Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/24779 )
Change subject: [metrics] Support merge_rules aggregation in /metrics_prometheus ...................................................................... Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/24779/1/src/kudu/server/default_path_handlers.cc File src/kudu/server/default_path_handlers.cc: http://gerrit.cloudera.org:8080/#/c/24779/1/src/kudu/server/default_path_handlers.cc@523 PS1, Line 523: // Malformed rules (i.e. not exactly three '|'-separated parts) are ignored, : // mirroring the JSON endpoint's lenient behaviour. Does it make sense at least to report on invalid input for the setting provided with the startup flags? In other words, consider adding a validator for the newly introduced --metrics_prometheus_default_merge_rules flag, issuing a warning if the provided string is malformed. http://gerrit.cloudera.org:8080/#/c/24779/1/src/kudu/server/default_path_handlers.cc@552 PS1, Line 552: merge_rules nit: maybe, create a static/constexpr constant and use it here and elsewhere instead of the "merge_rules" string literal? http://gerrit.cloudera.org:8080/#/c/24779/1/src/kudu/util/metrics-test.cc File src/kudu/util/metrics-test.cc: PS1: I couldn't find scenarios that would involve taking aggregation rules from --metrics_prometheus_default_merge_rules. Consider adding scenarios to cover the following: * provide a valid setting with --metrics_prometheus_default_merge_rules and make sure it's applied when no aggregation rules are sent with a request * provide a valid setting with --metrics_prometheus_default_merge_rules and make sure it's overridden by the rules provided with a request * provide a invalid setting with --metrics_prometheus_default_merge_rules, and make sure there isn't any aggregation attempted on the invalid input http://gerrit.cloudera.org:8080/#/c/24779/1/src/kudu/util/metrics-test.cc@417 PS1, Line 417: Is there any interest in adding test for aggregation based on the 'partition' attribute? For example, 'HASH (col_x) PARTITION 3' for a hash-partitioned table? http://gerrit.cloudera.org:8080/#/c/24779/1/src/kudu/util/metrics.cc File src/kudu/util/metrics.cc: http://gerrit.cloudera.org:8080/#/c/24779/1/src/kudu/util/metrics.cc@131 PS1, Line 131: ",hostname=\"" + EscapePrometheusLabelValue(opts.hostname) + "\"" nit: maybe, explicitly pre-compute this string outside of the enclosing 'for(...)' cycle? -- To view, visit http://gerrit.cloudera.org:8080/24779 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I76a365de3b328f2beefd460626cc0c896537e713 Gerrit-Change-Number: 24779 Gerrit-PatchSet: 1 Gerrit-Owner: Yan-Daojiang <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Reviewer: Yan-Daojiang <[email protected]> Gerrit-Comment-Date: Thu, 03 Sep 2026 00:02:13 +0000 Gerrit-HasComments: Yes
