Marton Greber has posted comments on this change. ( http://gerrit.cloudera.org:8080/24096 )
Change subject: [WIP] Export entity attributes as Prometheus labels ...................................................................... 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. 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: const string labels = "type=\"" + type_label + "\","; Unlike tablet/table entities where BuildPrometheusLabels always emits both type= and id=, server entities only get type=. In a multi-master setup there's no way to distinguish which master a metric came from. Consider including id_ (or the hostname) as an id= label here for consistency with other entities. Another thing here: should we use EscapePrometheusLabelValue here for consistency? http://gerrit.cloudera.org:8080/#/c/24096/1/src/kudu/util/metrics.cc@440 PS1, Line 440: WriteMetricsPrometheus(writer, metrics, "", labels); Both paths (here and below on WriteMetricsPrometheus) pass prefix="". This drops the kudu_ prefix entirely from all metric names. Prometheus recommends an application-level prefix. Suggest passing "kudu_" as prefix here instead of "". http://gerrit.cloudera.org:8080/#/c/24096/1/src/kudu/util/metrics.cc@1041 PS1, Line 1041: SubstituteAndAppend(&out, kFmt, prefix, name, "", labels, unit, value()); : SubstituteAndAppend(&out, kFmt, prefix, name, "_count", labels, unit, total_count()); : SubstituteAndAppend(&out, kFmt, prefix, name, "_sum", labels, unit, total_sum()); <metric>_count and <metric>_sum value lines are emitted with no preceding # HELP/# TYPE. The new CheckPrometheusOutput in test_util.cc has a special exemption for _sum/_count suffixes specifically to paper over this. Prometheus will generate warnings for value lines without metadata. This is pre-existing, but the new test helper explicitly works around it — worth a follow-up ticket or a comment explaining why it's acceptable. 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: partition IIUC partition comes from partition_schema().PartitionDebugString(...). This can be very long. Partition as an alerting rule can be brittle, do we really want to include this here? What do you think? http://gerrit.cloudera.org:8080/#/c/24096/1/src/kudu/util/prometheus_writer.cc@64 PS1, Line 64: return labels; The returned string always ends with ,. All format strings rely on this to produce valid output (e.g. {$2unit_type="bytes"} only works if $2 ends with ,). This silent contract will produce malformed output if any caller passes a labels string without the trailing comma. Please add a comment on BuildPrometheusLabels's return contract, or restructure the format strings so the separator is explicit. -- 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: 1 Gerrit-Owner: Yan-Daojiang <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Comment-Date: Fri, 13 Mar 2026 13:00:12 +0000 Gerrit-HasComments: Yes
