Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13376 )
Change subject: [util] Support for entity type/id/attrs/metrics filtering in metrics ...................................................................... Patch Set 3: (10 comments) ttr http://gerrit.cloudera.org:8080/#/c/13376/3/src/kudu/server/default_path_handlers.cc File src/kudu/server/default_path_handlers.cc: http://gerrit.cloudera.org:8080/#/c/13376/3/src/kudu/server/default_path_handlers.cc@342 PS3, Line 342: // 'type' : { : const string* arg = FindOrNull(req.parsed_args, "type"); : if (arg != nullptr) { : SplitStringUsing(*arg, ",", &opts.entity_types); : } : } Could you decompose this into a helper function to avoid repeating the same few lines of code? http://gerrit.cloudera.org:8080/#/c/13376/3/src/kudu/util/metrics-test.cc File src/kudu/util/metrics-test.cc: http://gerrit.cloudera.org:8080/#/c/13376/3/src/kudu/util/metrics-test.cc@407 PS3, Line 407: METRIC_DEFINE_entity(tablet); : METRIC_DEFINE_counter(tablet, rows_inserted, "Rows Inserted", MetricUnit::kRows, : "Number of rows inserted into this tablet since service start"); : METRIC_DEFINE_counter(tablet, rows_upserted, "Rows Upserted", kudu::MetricUnit::kRows, : "Number of rows upserted into this tablet since service start"); Could you reuse the entity/counter already defined by this test? (L55, L70, L84, and L98). I think you could assign whatever IDs/attributes to that entity that you want. http://gerrit.cloudera.org:8080/#/c/13376/3/src/kudu/util/metrics-test.cc@419 PS3, Line 419: const auto GenMetrics = [&] () { GenMetrics and CheckMetrics are only called once each, so why bother wrapping them in lambdas? http://gerrit.cloudera.org:8080/#/c/13376/3/src/kudu/util/metrics-test.cc@443 PS3, Line 443: { : const string entity_type = "not_exist_type"; : std::ostringstream out; : MetricJsonOptions opts; : opts.entity_types.emplace_back(entity_type); : JsonWriter w(&out, JsonWriter::PRETTY); : ASSERT_OK(registry_.WriteAsJson(&w, opts)); : ASSERT_EQ("[]", out.str()); : } Given how often this block is repeated, if we can reduce some of the lines of code via lambda, we should. http://gerrit.cloudera.org:8080/#/c/13376/3/src/kudu/util/metrics-test.cc@559 PS3, Line 559: opts.entity_attrs.emplace_back(entity_attr_key1); : opts.entity_attrs.emplace_back(entity_attr_val1); : opts.entity_attrs.emplace_back(entity_attr_key2); : opts.entity_attrs.emplace_back(entity_attr_val2); : opts.entity_attrs.emplace_back(entity_attr_key3); : opts.entity_attrs.emplace_back(entity_attr_val3); You should be able to do something like: opts.entity_attrs = { "...", "...", "..." } http://gerrit.cloudera.org:8080/#/c/13376/3/src/kudu/util/metrics.h File src/kudu/util/metrics.h: http://gerrit.cloudera.org:8080/#/c/13376/3/src/kudu/util/metrics.h@445 PS3, Line 445: '*' Do we still need to support this? Seems like 'empty' is a simpler way to express matching all, and I don't believe we ever exposed '*' in a wire protocol. http://gerrit.cloudera.org:8080/#/c/13376/3/src/kudu/util/metrics.h@452 PS3, Line 452: std::vector<std::string> entity_attrs; This is more special than the others. Please doc how it works. http://gerrit.cloudera.org:8080/#/c/13376/3/src/kudu/util/metrics.cc File src/kudu/util/metrics.cc: http://gerrit.cloudera.org:8080/#/c/13376/3/src/kudu/util/metrics.cc@187 PS3, Line 187: bool MatchMetricInList(const string& metric_name, This should probably be renamed (and maybe rename some of its variables/comments) to reflect its more generic nature. That is, we're not just using it to match metrics; we're now using it to match arbitrary strings. http://gerrit.cloudera.org:8080/#/c/13376/3/src/kudu/util/metrics.cc@225 PS3, Line 225: else if (opts.entity_attrs.size() % 2 != 0) { : return Status::OK(); : } Why do we exit early if there's an odd number of attribute filters? Oh, it looks like that's because of the special meaning of entity_attrs: the number of entries should always be even because each pair represents a key and a value. Please doc this and check for it upfront, in MetricRegistry::WriteAsJSON. Should probably return an error or something if it's invalid. http://gerrit.cloudera.org:8080/#/c/13376/3/src/kudu/util/metrics.cc@249 PS3, Line 249: vector<string> attr_val; No need for this to be a vector; there's always just one entry. On L254 you can do { attr_val } to get a temporary vector for MatchMetricInList. -- To view, visit http://gerrit.cloudera.org:8080/13376 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I45ca55bd1f0f3d4107cb208f6f855d24121823ee Gerrit-Change-Number: 13376 Gerrit-PatchSet: 3 Gerrit-Owner: helifu <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: helifu <[email protected]> Gerrit-Comment-Date: Wed, 22 May 2019 18:27:33 +0000 Gerrit-HasComments: Yes
