Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/24269 )
Change subject: KUDU-3690: Add filtering to /metrics_prometheus ...................................................................... Patch Set 4: (11 comments) http://gerrit.cloudera.org:8080/#/c/24269/4/src/kudu/master/master-test.cc File src/kudu/master/master-test.cc: http://gerrit.cloudera.org:8080/#/c/24269/4/src/kudu/master/master-test.cc@4257 PS4, Line 4257: // Test that the Prometheus service discovery endpoint returns proper target groups Consider adding a scenario to verify that combining two different filters works as expected. The related critical code path is the same as for JSON metrics, but from the testing standpoint we should be looking at it as a "black box thing". In other words, make sure the metrics output for the query parameters such as "?level=info&types=server" is as expected. http://gerrit.cloudera.org:8080/#/c/24269/4/src/kudu/server/default_path_handlers.cc File src/kudu/server/default_path_handlers.cc: http://gerrit.cloudera.org:8080/#/c/24269/4/src/kudu/server/default_path_handlers.cc@514 PS4, Line 514: if (!ParseMetricFilters(req, resp, &opts.filters)) return; style/readability nit here and elsewhere: use scope braces for this one-liner as well, and maybe adding a comment on ParseMetricFilters() setting appropriate HTTP response code? http://gerrit.cloudera.org:8080/#/c/24269/4/src/kudu/tserver/tablet_server-test.cc File src/kudu/tserver/tablet_server-test.cc: http://gerrit.cloudera.org:8080/#/c/24269/4/src/kudu/tserver/tablet_server-test.cc@4621 PS4, Line 4621: } Consider adding a case to mix 'attributes' with 'ids', but specifying identifier for a tablet belonging to a different table. At least to make sure it doesn't barf but returns an empty metrics output: "?attributes=table_id,TestTable&ids=eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee" http://gerrit.cloudera.org:8080/#/c/24269/4/src/kudu/tserver/tablet_server-test.cc@4623 PS4, Line 4623: Similar to master-test.cc, consider adding a scenario to verify that combining two different filters works as expected. In other words, make sure it outputs the expected data for query parameters such as "?level=info&types=tablet" or similar. http://gerrit.cloudera.org:8080/#/c/24269/4/src/kudu/util/metrics-test.cc File src/kudu/util/metrics-test.cc: http://gerrit.cloudera.org:8080/#/c/24269/4/src/kudu/util/metrics-test.cc@1644 PS4, Line 1644: warn_m->Increment(); : info_m->Increment(); : debug_m->Increment(); I'd expect that counters with value of 0 would be present in the output. Is it a requirement to update the metrics to have them in the output? If not, then these two lines aren't consequential by any means; maybe, remove them? If yes, that's a bit strange for counters, at least. http://gerrit.cloudera.org:8080/#/c/24269/4/src/kudu/util/metrics-test.cc@1690 PS4, Line 1690: tablet_counter->Increment(); ditto: is it needed? http://gerrit.cloudera.org:8080/#/c/24269/4/src/kudu/util/metrics-test.cc@1694 PS4, Line 1694: table_counter->Increment(); ditto: is it needed? http://gerrit.cloudera.org:8080/#/c/24269/4/src/kudu/util/metrics-test.cc@1730 PS4, Line 1730: c_keep->IncrementBy(10); : c_drop->IncrementBy(20); ditto: is it needed? http://gerrit.cloudera.org:8080/#/c/24269/4/src/kudu/util/metrics-test.cc@1785 PS4, Line 1785: c_a->IncrementBy(1); : c_b->IncrementBy(2); ditto: is it needed? http://gerrit.cloudera.org:8080/#/c/24269/4/src/kudu/util/metrics-test.cc@1797 PS4, Line 1797: } There are some query parameter that are effective for JSON metrics format only, but not for the Prometheus format. Would it make sense to add extra test scenarios to cover that, at least to make sure nothing crashes if, for example, adding 'include_raw_histograms=1' into the query at /metrics_prometheus? Essentially, this is to explicitly tell that those parameters aren't applicable to the Prometheus format, and specifying them don't lead to any changes in the Prometheus metrics output. Probably, this has an intersection with the coverage mentioned in the next below. https://kudu.apache.org/docs/administration.html#_collecting_metrics_via_http http://gerrit.cloudera.org:8080/#/c/24269/4/src/kudu/util/metrics-test.cc@1798 PS4, Line 1798: Does it make sense to add a few test scenarios to verify how the new code handles wrong filtering input: * unrecognized filter keys * incorrect values for established/recognized keys (i.e., missing value from the key/value pair, etc.) -- To view, visit http://gerrit.cloudera.org:8080/24269 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5c0b23ae5c184bf9e33e453736cef5e7ce8ee2e1 Gerrit-Change-Number: 24269 Gerrit-PatchSet: 4 Gerrit-Owner: Marton Greber <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Ashwani Raina <[email protected]> Gerrit-Reviewer: Gabriella Lotz <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Reviewer: Yan-Daojiang <[email protected]> Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Reviewer: Zoltan Martonka <[email protected]> Gerrit-Comment-Date: Fri, 08 May 2026 00:26:18 +0000 Gerrit-HasComments: Yes
