Rewrite to use the new Prometheus Java client #25

mimaison · 2024-06-25T16:53:09Z

This introduces some changes:

Quantiles names are now serialized without trailing zeros. So we now have labels like quantile="0.5". Previously the label was quantile="0.50".
I've added sum series to Summary metrics. They now have quantiles, count and sum. Sum series have the _sum suffix.
Removed _count suffix from Counter metrics. As per the Counter documentation, Counter metrics should use the _total suffix. Previously we were injecting _count and Prometheus was adding _total. So now we have kafka_server_kafka_server_brokertopicmetrics_bytesinpersec_total while before we had kafka_server_kafka_server_brokertopicmetrics_bytesinpersec_count_total.
The new client has a dedicated metric type of non-numeric metrics, InfoSnapshot. Under the covers this still emits a Gauge metric with a value set to 1, like we do today. This is because Prometheus does not support non-numeric metrics. It's preferable to use the dedicated type, however this new type uses the _info suffix. So now we have kafka_server_kafka_server_kafkaserver_clusterid_info{ClusterId="mHGu-1meQOSC_RpQHdCpOw"} while before we had kafka_server_kafka_server_kafkaserver_clusterid{ClusterId="mHGu-1meQOSC_RpQHdCpOw"}. The suffix helps identify metrics whose values should not be graphed (as they are constant) and where the useful information is in the labels instead.
Gauge metrics don't have a suffix anymore, as per the OpenMetrics specification. The previous Prometheus client was injecting a _total suffix. We now have kafka_server_txn_marker_channel_metrics_response, while previously we had kafka_server_txn_marker_channel_metrics_request_total.
The new client does not insert a trailing comma after the last label. We now have kafka_server_alter_partition_metrics_connection_close_rate{BrokerId="0"}, while we previously had kafka_server_alter_partition_metrics_connection_close_rate{BrokerId="0",}.

The new client allows exposing metrics in the OpenMetrics format and serialized with Protobuf. When Prometheus scrapes the endpoint, it negotiates the format using the Accept HTTP header. You can check different formats using the debug query parameter on the GET /metrics endpoint. For example:

/metrics?debug=openmetrics: View OpenMetrics text format.
/metrics?debug=text: View Prometheus text format.
/metrics?debug=prometheus-protobuf: View a text representation of the Prometheus protobuf format.

Related resources:

mimaison · 2024-06-26T10:40:59Z

src/main/java/io/strimzi/kafka/metrics/KafkaMetricsCollector.java

+     */
+    @Override
+    public MetricSnapshots collect() {
+        Map<String, GaugeSnapshot.Builder> gaugeBuilders = new HashMap<>();


Unfortunately the MetricSnapshotBuilder interface does not have a build() method. I opened a PR against prometheus/client_java to change that.

In the meantime, I think we have to keep a collection per metric type.

mimaison · 2024-06-26T10:44:39Z

src/main/java/io/strimzi/kafka/metrics/DataPointSnapshotBuilder.java

+     * @param labels Labels associated with the datapoint
+     * @return The {@link CounterSnapshot.CounterDataPointSnapshot} datapoint
+     */
+    public static CounterSnapshot.CounterDataPointSnapshot convert(Number number, Labels labels) {


I'm not super happy with the various methods especially as convert(double value, Labels labels) and convert(Number number, Labels labels) shadow each other. Ideally I wanted to keep this file "Yammer free" but I ended up having to use Timer and Histogram.

So maybe we should tweak some of these convert() methods to accept the Yammer and Kafka metric types.

if you had sum and count parameters that were Suppliers and a value Function, you could hide yammerness from this class.

I shuffled a few things around so this class does not depend on Yammer anymore.

mimaison · 2024-06-26T10:47:07Z

src/main/java/io/strimzi/kafka/metrics/KafkaMetricsCollector.java

+    }
+
+    private String metricName(MetricName metricName) {
+        return PrometheusNaming


I'm not sure I fully understand how to use the various methods in PrometheusNaming especially sanitizeMetricName() and prometheusName().

mimaison · 2024-06-26T10:51:02Z

src/main/java/io/strimzi/kafka/metrics/KafkaPrometheusMetricsReporter.java

+    }
+
+    // for testing
+    KafkaPrometheusMetricsReporter(PrometheusRegistry registry) {


PrometheusRegistry has no methods to clear it. For this reason, I introduced constructors to inject a registry instance. This simplifies testing.
Same in YammerPrometheusMetricsReporter

mimaison · 2024-06-26T10:52:11Z

src/main/java/io/strimzi/kafka/metrics/PrometheusMetricsReporterConfig.java

     */
-    public PrometheusMetricsReporterConfig(Map<?, ?> props) {
+    public PrometheusMetricsReporterConfig(Map<?, ?> props, PrometheusRegistry registry) {


It's not ideal to have a registry field here. But it's required when start the HTTP server.

mimaison · 2024-06-26T11:24:02Z

src/test/java/io/strimzi/kafka/metrics/KafkaMetricsCollectorTest.java


    @BeforeEach
    public void setup() {
-        labels = new HashMap<>();
-        labels.put("key", "value");
+        tagsMap = new LinkedHashMap<>();


The setup of the 2 Collector tests is still a bit wonky with the labels/tagsMap fields. I'll see if I can tidy this up a little bit.

mimaison · 2024-06-26T12:54:45Z

pom.xml

@@ -309,6 +309,23 @@
          </archive>
        </configuration>
      </plugin>
+      <!-- Temporarily re-add the assembly plugin before we fix the packaging -->


This will be addressed in #24

k-wall · 2024-06-26T13:37:57Z

pom.xml

@@ -169,17 +169,17 @@

    <dependency>
      <groupId>io.prometheus</groupId>
-      <artifactId>simpleclient</artifactId>
+      <artifactId>prometheus-metrics-model</artifactId>


Should we just import the BOM https://central.sonatype.com/artifact/io.prometheus/prometheus-metrics-bom

We try to avoid that. It makes it hard to override dependencies etc.

k-wall · 2024-06-26T13:40:22Z

src/main/java/io/strimzi/kafka/metrics/DataPointSnapshotBuilder.java

+
+    private static Quantiles quantiles(Sampling samplingMetric) {
+        Quantiles.Builder quantilesBuilder = Quantiles.builder();
+        for (double quantile : Arrays.asList(0.50, 0.75, 0.95, 0.98, 0.99, 0.999)) {


extract constant

k-wall · 2024-06-26T13:49:14Z

src/main/java/io/strimzi/kafka/metrics/DataPointSnapshotBuilder.java

+     * @return The {@link InfoSnapshot.InfoDataPointSnapshot} datapoint
+     */
+    public static InfoSnapshot.InfoDataPointSnapshot convert(Object value, Labels labels, String metricName) {
+        labels = labels.add(PrometheusNaming.sanitizeLabelName(metricName), String.valueOf(value));


do we want to mutate the caller's labels?

Labels.add() returns a new instance, but yeah we don't want to reassign labels. Fixed.

k-wall

lgtm

scholzj · 2024-06-27T15:30:06Z

.checkstyle/suppressions.xml

+  <suppress checks="CyclomaticComplexity" files="YammerMetricsCollector.java"/>
+  <suppress checks="NPathComplexity" files="YammerMetricsCollector.java"/>
+  <suppress checks="JavaNCSS" files="YammerMetricsCollector.java"/>
+  <suppress checks="ClassFanOutComplexity" files="YammerMetricsCollector.java"/>


Should you use annotations to suppress these instead of suppressing it here? That is what we normally do -> it is better coupled with the code and easier to see and maintain.

Right, if it's what Strimzi usually does, yes let's use annotations. Done

Signed-off-by: Mickael Maison <[email protected]>

mimaison · 2024-06-27T19:07:14Z

I rebased on main and removed my pom.xml changes related to packaging as it's been resolved in #27

mimaison force-pushed the new-client branch 2 times, most recently from a9e3cc9 to c1b97e5 Compare June 26, 2024 10:30

mimaison marked this pull request as ready for review June 26, 2024 10:37

mimaison commented Jun 26, 2024

View reviewed changes

mimaison force-pushed the new-client branch from c1b97e5 to 87cb058 Compare June 26, 2024 10:56

mimaison requested review from k-wall and OwenCorrigan76 June 26, 2024 10:57

mimaison commented Jun 26, 2024

View reviewed changes

k-wall reviewed Jun 26, 2024

View reviewed changes

k-wall approved these changes Jun 27, 2024

View reviewed changes

scholzj reviewed Jun 27, 2024

View reviewed changes

scholzj added this to the 0.1.0 milestone Jun 27, 2024

scholzj approved these changes Jun 27, 2024

View reviewed changes

mimaison added 3 commits June 27, 2024 20:59

Rewrite to use the new Prometheus Java client

a8b4b4a

Signed-off-by: Mickael Maison <[email protected]>

Address feedback

5641518

Signed-off-by: Mickael Maison <[email protected]>

Use annotations for checkstyle suppressions

85d9c4c

Signed-off-by: Mickael Maison <[email protected]>

mimaison force-pushed the new-client branch from fa93fd7 to 85d9c4c Compare June 27, 2024 19:04

mimaison merged commit 4c1fd9e into strimzi:main Jun 27, 2024
7 checks passed

mimaison deleted the new-client branch June 27, 2024 19:15

mimaison mentioned this pull request Jun 27, 2024

Update Prometheus dependency #23

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite to use the new Prometheus Java client #25

Rewrite to use the new Prometheus Java client #25

mimaison commented Jun 25, 2024 •

edited

Loading

mimaison Jun 26, 2024

mimaison Jun 26, 2024

k-wall Jun 26, 2024

mimaison Jun 26, 2024

mimaison Jun 26, 2024

mimaison Jun 26, 2024

mimaison Jun 26, 2024

mimaison Jun 26, 2024

mimaison Jun 26, 2024

k-wall Jun 26, 2024

scholzj Jun 26, 2024

k-wall Jun 26, 2024

k-wall Jun 26, 2024

mimaison Jun 26, 2024

k-wall left a comment

scholzj Jun 27, 2024

mimaison Jun 27, 2024

mimaison commented Jun 27, 2024

Rewrite to use the new Prometheus Java client #25

Rewrite to use the new Prometheus Java client #25

Conversation

mimaison commented Jun 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k-wall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mimaison commented Jun 27, 2024

mimaison commented Jun 25, 2024 •

edited

Loading