helifu 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) 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 Done 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, Done 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 wrappi Done 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 Done 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: Done 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 ex I have no idea whether we exposed '*' in a wire protocol, but in the code before the change, '*' is supported, please look at L353/356 in default_path_handlers.cc. Right now after the change, there are 4 cases: (1) empty/none; (2) filter=* (3) filter=a,b,c,d (4) filter= case2 is the same as case3, but will match all. case4 is special in the original code, the metrics of every entity is empty when we call "http://x.x.x.x:8050/metrics?metrics=";. That means metrics are omitted. So, i will keep this design if you don't mind. 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. Done 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/com Done 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? Done 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 Done -- 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: Thu, 23 May 2019 03:11:05 +0000 Gerrit-HasComments: Yes
