Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13580 )
Change subject: [metrics] Merge metrics by the same attribute ...................................................................... Patch Set 4: (8 comments) Didn't review the whole thing yet, but left some nits and questions/suggestions about the approach. http://gerrit.cloudera.org:8080/#/c/13580/4/src/kudu/server/default_path_handlers.cc File src/kudu/server/default_path_handlers.cc: http://gerrit.cloudera.org:8080/#/c/13580/4/src/kudu/server/default_path_handlers.cc@347 PS4, Line 347: opts.merge_by_attribute = ParseBool(req.parsed_args, "merge"); If the point of this is to be pretty generic, would it make sense to use something like a `entities_to_merge` list instead? Perhaps something like: vector<string> entities_to_merge = ParseArray(req.parsed_args, "entities_to_merge"); for (const auto& e : entities_to_merge) { if (e == "tablet") { EmplaceIfNotPresent(&opts.entity_to_merge_attributes, "tablet", { "table", "table_name" }); } } ? Then MetricJsonOptions would be generic enough to merge by more than a single attribute. By a similar token, would it make sense to use a `unmerged_entities_to_include` list instead of `include_origin`? Or `unmerged_entities_to_exclude` if we want to keep the default behavior (in this case, an empty list) consistent with what exists today. http://gerrit.cloudera.org:8080/#/c/13580/4/src/kudu/util/metrics.h File src/kudu/util/metrics.h: http://gerrit.cloudera.org:8080/#/c/13580/4/src/kudu/util/metrics.h@444 PS4, Line 444: bool merge_by_attribute; Is this/Should this be affected by 'include_entity_attributes'? nit: How about about naming this 'merge_entities_by_attribute' so it's clear what is being merged? http://gerrit.cloudera.org:8080/#/c/13580/4/src/kudu/util/metrics.h@449 PS4, Line 449: include_origin nit: How about naming 'include_origin' to 'include_unmerged_entities'? "origin"/"original" is a bit vague in my opinion. http://gerrit.cloudera.org:8080/#/c/13580/4/src/kudu/util/metrics.h@442 PS4, Line 442: : // Whether to merge original metrics by some attribute. : bool merge_by_attribute; : : // Whether to include original metrics. : // We can reduce output size by set 'include_origin' to false and set : // 'merge_by_attribute' to true. : bool include_origin; What happens if these are both true or both false? I think: - both false: no metrics get output - both true: both the aggregate _and_ the original get output Is the both false case desirable? If not, would it make sense for `include_origin` to be `include_unmerged_if_merged` and have the both false case still yield unmerged metrics? http://gerrit.cloudera.org:8080/#/c/13580/4/src/kudu/util/metrics.h@451 PS4, Line 451: // Metrics in this prototype will be merged. : std::string prototype_name_merge_from; : : // Metrics will be merged to this prototype name. : std::string prototype_name_merge_to; : : // Metrics with the same attribute name will be merge together. : std::string attribute_for_merge; How about encapsulating these into a struct like: struct MergeAttributes { std::string from; std::string to; std::string merge_attribute; }; And then using an optional<MergeAttributes>? Then we could rely on the option rather than using merge_by_attribute. Or, if we're trying to be very generic and decide to aggregate multiple entities, would it make sense to use a map here? Something like: struct MergeAttributes { std::string merge_to; std::string attribute_to_merge_by; }; std::unordered_map<std::string, MergeAttributes> entity_to_merge_attributes; WDYT? http://gerrit.cloudera.org:8080/#/c/13580/4/src/kudu/util/metrics.h@673 PS4, Line 673: Status GetMetricsAndAttrs(const MetricJsonOptions& opts, Can you add docs to this, including that this will only ever return either OK or Status::NotFound()? http://gerrit.cloudera.org:8080/#/c/13580/4/src/kudu/util/metrics.h@699 PS4, Line 699: virtual scoped_refptr<Metric> clone() const = 0; Could you add docs for this and why it might be useful to clone? http://gerrit.cloudera.org:8080/#/c/13580/4/src/kudu/util/metrics.h@727 PS4, Line 727: virtual bool Merge(const scoped_refptr<Metric>& other) = 0; nit: Could you add docs for this too, including what the return value is? -- To view, visit http://gerrit.cloudera.org:8080/13580 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8db3d082ae847eb1d83b9e4aee57d5e4bf13e1b5 Gerrit-Change-Number: 13580 Gerrit-PatchSet: 4 Gerrit-Owner: Yingchun Lai <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Comment-Date: Tue, 18 Jun 2019 03:30:58 +0000 Gerrit-HasComments: Yes
