Yan-Daojiang has posted comments on this change. ( http://gerrit.cloudera.org:8080/24096 )
Change subject: KUDU-3692 Export entity attributes as Prometheus labels ...................................................................... Patch Set 2: (5 comments) > Patch Set 1: > > (5 comments) > > Thank you very much for preparing this patch, this is really useful to adopt > Prometheus conventions! > I tested this out with local Prometheus running, looks okay. Thanks for the thorough review! All comments have been addressed in the patch set 2. PTAL when you get a chance. Thanks! http://gerrit.cloudera.org:8080/#/c/24096/1/src/kudu/util/metrics.cc File src/kudu/util/metrics.cc: http://gerrit.cloudera.org:8080/#/c/24096/1/src/kudu/util/metrics.cc@439 PS1, Line 439: } else if (id_ == kIdTabletServer) { > Unlike tablet/table entities where BuildPrometheusLabels always emits both Done http://gerrit.cloudera.org:8080/#/c/24096/1/src/kudu/util/metrics.cc@440 PS1, Line 440: prefix = "kudu_tserver_"; > Both paths (here and below on WriteMetricsPrometheus) pass prefix="". This DONE. http://gerrit.cloudera.org:8080/#/c/24096/1/src/kudu/util/metrics.cc@1041 PS1, Line 1041: : const string full_sum_name = Substitute("$0$1_sum", prefix, name); : if (writer->ShouldWriteHelpAndType(full_sum_name)) { > <metric>_count and <metric>_sum value lines are emitted with no preceding # Good catch. I've addressed this in patch set 2: For Histogram: Added dedicated # HELP and # TYPE lines for <metric>_sum and <metric>_count, treating them as independent counter-type metrics. The description appends (sum) or (count) suffix to the original description. For MeanGauge: Similarly added HELP/TYPE for its _sum/_count variants, using gauge type since these values may reset. Test validation strengthened: Removed the _sum/_count suffix workaround in CheckPrometheusOutput(). The test helper now strictly requires every value line to have a corresponding HELP entry — no special exemptions. Both new and legacy formats now emit proper metadata for _sum/_count lines. This ensures Prometheus won't generate warnings about missing metadata during scrapes. http://gerrit.cloudera.org:8080/#/c/24096/1/src/kudu/util/prometheus_writer.cc File src/kudu/util/prometheus_writer.cc: http://gerrit.cloudera.org:8080/#/c/24096/1/src/kudu/util/prometheus_writer.cc@56 PS1, Line 56: > IIUC partition comes from partition_schema().PartitionDebugString(...). Thi Yes, I agree with you. The "partition" labels can become very long, increasing the size of the label values ??and potentially causing issues with Prometheus metric storage and queries. I initially included this in the implementation because I saw Beat Fuellemann at https://issues.apache.org/jira/browse/KUDU-3692 wanting all this information. I've now removed the partitions in Patch set 2. http://gerrit.cloudera.org:8080/#/c/24096/1/src/kudu/util/prometheus_writer.cc@64 PS1, Line 64: std::string type_label; > The returned string always ends with ,. All format strings rely on this to Addressed. I restructured the approach: 1. BuildPrometheusLabels() now explicitly returns without trailing comma (documented in comment) 2. Added PrometheusLabelPrefixForInjection() helper that callers must use to get proper comma-separated prefix 3. All call sites updated to use this helper This makes the separator handling explicit at both producer and consumer sides. -- To view, visit http://gerrit.cloudera.org:8080/24096 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I690918d06f19c393369b8fc43c3ec90dd2231d3d Gerrit-Change-Number: 24096 Gerrit-PatchSet: 2 Gerrit-Owner: Yan-Daojiang <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Reviewer: Yan-Daojiang <[email protected]> Gerrit-Comment-Date: Mon, 16 Mar 2026 14:10:01 +0000 Gerrit-HasComments: Yes
