Marton Greber has posted comments on this change. ( http://gerrit.cloudera.org:8080/24269 )
Change subject: KUDU-3690: Add filtering to /metrics_prometheus ...................................................................... Patch Set 5: (12 comments) http://gerrit.cloudera.org:8080/#/c/24269/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/24269/4//COMMIT_MSG@18 PS4, Line 18: Malformed ?attributes= (odd number of values) returns HTTP 400, > could you add a test to metrics-test.cc to cover this scenario? good point, added: TEST_F(MasterTest, MetricsOddAttributesReturnsBadRequest) TEST_F(TabletServerTest, MetricsOddAttributesReturnsBadRequest) 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: > Consider adding a scenario to verify that combining two different filters w yes makes sense, added: TEST_F(MasterTest, PrometheusMetricsCombinedFilters) TEST_F(TabletServerTest, PrometheusMetricsCombinedFilters) 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: // ParseMetricFilters() sets resp->status_code to HTTP 400 and returns false > style/readability nit here and elsewhere: use scope braces for this one-lin Done 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 ident ah yea good point, added: TEST_F(TabletServerTest, PrometheusMetricsMixedAttributesAndIds) 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 combin Done 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" level: on > I'd expect that counters with value of 0 would be present in the output. they are not needed indeed, removed them. http://gerrit.cloudera.org:8080/#/c/24269/4/src/kudu/util/metrics-test.cc@1690 PS4, Line 1690: > ditto: is it needed? Done http://gerrit.cloudera.org:8080/#/c/24269/4/src/kudu/util/metrics-test.cc@1694 PS4, Line 1694: PrometheusWriter writer(& > ditto: is it needed? Done http://gerrit.cloudera.org:8080/#/c/24269/4/src/kudu/util/metrics-test.cc@1730 PS4, Line 1730: ASSERT_OK(registry.WriteAsPrometheus(&writer, opts)); : const auto& str = out.st > ditto: is it needed? Done http://gerrit.cloudera.org:8080/#/c/24269/4/src/kudu/util/metrics-test.cc@1785 PS4, Line 1785: ASSERT_STR_CONTAINS(str, "id=\"tablet-a\""); : ASSERT_STR_NOT_CONTA > ditto: is it needed? Done http://gerrit.cloudera.org:8080/#/c/24269/4/src/kudu/util/metrics-test.cc@1797 PS4, Line 1797: auto entity = METRIC_ENTITY_test_entity.Instantiate(®istry, "level-test"); > There are some query parameter that are effective for JSON metrics format o yes, added: TEST_F(MasterTest, PrometheusMetricsJsonOnlyParamsIgnored) TEST_F(TabletServerTest, PrometheusMetricsJsonOnlyParamsIgnored) http://gerrit.cloudera.org:8080/#/c/24269/4/src/kudu/util/metrics-test.cc@1798 PS4, Line 1798: METRIC_warn_counter.Instantiate(entity); > Does it make sense to add a few test scenarios to verify how the new code h Done -- 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: 5 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 12:32:25 +0000 Gerrit-HasComments: Yes
