JMX Scraper: Kafka server, producer and consumer YAMLs and integration tests added #1670

robsunday · 2025-01-24T12:37:48Z

Three target systems connected to Kafka have been added:

Server
Producer
Consumer

To fully test Kafka metrics there is a need to spin multiple containers, so these three components can correctly interact and metrics can be correctly generated.
Base integration test class TargetSystemIntegrationTest was modified to support initialization of multiple containers.

There are still some TODOs left that will be fixed in subsequent PRs

Part of #1362

robsunday · 2025-01-24T12:39:01Z

...est/java/io/opentelemetry/contrib/jmxscraper/target_systems/TargetSystemIntegrationTest.java

@@ -86,12 +88,23 @@ static void afterAll() {

  @AfterEach
  void afterEach() {
+    if (scraper != null && scraper.isRunning()) {


[for reviewer] I rearranged containers shutdown sequence to be in reverse order than starting to avoid unnecessary errors in the logs

robsunday · 2025-01-24T12:40:25Z

...est/java/io/opentelemetry/contrib/jmxscraper/target_systems/TargetSystemIntegrationTest.java

  }

  protected void verifyMetrics() {
    MetricsVerifier metricsVerifier = createMetricsVerifier();
    await()
        .atMost(Duration.ofSeconds(60))
+        .pollInterval(Duration.ofSeconds(1))


[for reviewer] Just to decrease CPU usage a bit if we do not have all metrics available immediately.

...a/io/opentelemetry/contrib/jmxscraper/target_systems/kafka/KafkaConsumerIntegrationTest.java

robsunday · 2025-01-24T12:42:32Z

...a/io/opentelemetry/contrib/jmxscraper/target_systems/kafka/KafkaConsumerIntegrationTest.java

+                metric
+                    .hasDescription(
+                        "The average number of bytes consumed for all topics per second")
+                    .hasUnit("By")


[for reviewer] Changed from 'by' to follow semconv

robsunday · 2025-01-24T12:44:03Z

...a/io/opentelemetry/contrib/jmxscraper/target_systems/kafka/KafkaConsumerIntegrationTest.java

+                metric
+                    .hasDescription(
+                        "The average number of records consumed for all topics per second")
+                    .hasUnit("{record}")


[for reviewer] Multiple units in this file have been changed from "1" to annotated form to follow semconv

robsunday · 2025-01-24T12:45:05Z

...est/java/io/opentelemetry/contrib/jmxscraper/target_systems/kafka/KafkaContainerFactory.java

+            "sh",
+            "-c",
+            "echo 'Sending messages to test-topic-1'; "
+                + "i=1; while true; do echo \"Message $i\"; sleep .25; i=$((i+1)); done | /opt/bitnami/kafka/bin/kafka-console-producer.sh --bootstrap-server "


[for reviewer] Topic is automatically created when messages are sent to it - no need to manually create it

...Test/java/io/opentelemetry/contrib/jmxscraper/target_systems/kafka/KafkaIntegrationTest.java

robsunday · 2025-01-24T12:47:36Z

...a/io/opentelemetry/contrib/jmxscraper/target_systems/kafka/KafkaProducerIntegrationTest.java

+        .withEnv("JMX_PORT", Integer.toString(jmxPort))
+        .withExposedPorts(jmxPort)
+        .waitingFor(Wait.forListeningPorts(jmxPort));
+    //        .waitingFor(Wait.forLogMessage(".*Sending messages to test-topic-1.*", 1));


[for reviewer] Will be cleaned

robsunday · 2025-01-24T12:49:59Z

jmx-scraper/src/main/resources/kafka.yaml

+        metric: kafka.purgatory.size
+        type: updowncounter
+        desc: The number of requests waiting in purgatory
+        unit: "{request}"


[for reviewer] Please verify all annotations of units. In the original files they were either in plular form, or were missing (unit: "1" was used instead)

jmx-scraper/src/main/resources/kafka-consumer.yaml

robsunday · 2025-01-24T12:50:56Z

jmx-scraper/src/main/resources/kafka-producer.yaml

+        unit: ms
+      request-rate:
+        desc: The average number of requests sent per second
+        unit: '{request}'


[for reviewer] Please verify all annotations of units. In the original files they were either in plular form, or were missing (unit: "1" was used instead)

...est/java/io/opentelemetry/contrib/jmxscraper/target_systems/kafka/KafkaContainerFactory.java

SylvainJuge · 2025-01-27T13:52:14Z

jmx-scraper/src/main/resources/kafka-consumer.yaml

+      bytes-consumed-rate:
+        metric: total.bytes-consumed-rate
+        desc: The average number of bytes consumed for all topics per second
+        unit: 'By'


[minor] we can remove quotes here on the unit for simplicity/consistency.

Suggested change

unit: 'By'

unit: By

jmx-scraper/src/main/resources/kafka-consumer.yaml

jmx-scraper/src/main/resources/kafka.yaml

SylvainJuge · 2025-01-27T14:18:03Z

jmx-scraper/src/main/resources/kafka-producer.yaml

+      record-error-rate:
+        desc: The average per-second number of record sends that resulted in errors for a topic
+      record-retry-rate:
+        desc: The average per-second number of retried record sends for a topic
+      record-send-rate:
+        desc: The average number of records sent per second for a topic


[minor] because the unit is different for a few metrics means it's probably simpler to always describe the unit at the same level instead of only overriding when it's different.

Yes, in this case I agree with your opinion - it is easier to grasp

jmx-scraper/src/main/resources/kafka-consumer.yaml

...a/io/opentelemetry/contrib/jmxscraper/target_systems/kafka/KafkaConsumerIntegrationTest.java

Code cleanups Other code review related changes

breedx-splk · 2025-02-07T23:01:45Z

...Test/java/io/opentelemetry/contrib/jmxscraper/target_systems/kafka/KafkaIntegrationTest.java

+    // TODO: Find out how to force Kafka to generate these metrics
+    //        .add(
+    //            "kafka.logs.flush.count",
+    //            metric ->
+    //                metric
+    //                    .hasDescription("Log flush count")
+    //                    .hasUnit("{flush}")
+    //                    .isCounter()
+    //                    .hasDataPointsWithoutAttributes())
+    //        .add(
+    //            "kafka.logs.flush.time.median",
+    //            metric ->
+    //                metric
+    //                    .hasDescription("Log flush time - 50th percentile")
+    //                    .hasUnit("ms")
+    //                    .isGauge()
+    //                    .hasDataPointsWithoutAttributes())
+    //        .add(
+    //            "kafka.logs.flush.time.99p",
+    //            metric ->
+    //                metric
+    //                    .hasDescription("Log flush time - 99th percentile")
+    //                    .hasUnit("ms")


A tracking issue might be more visible than these comments. I think you should open an issue.

Done: #1726

breedx-splk

Looks good to me. Wowza that's a lot of metrics. :) Makes one wish that kafka had native otel support ootb... 🤔

robsunday · 2025-02-10T16:24:54Z

Looks good to me. Wowza that's a lot of metrics. :) Makes one wish that kafka had native otel support ootb... 🤔

Yes, and newer Kafka versions have surely even more.

trask · 2025-02-10T18:19:59Z

@robsunday can you merge main into your PR branch? that will fix the required gradle-wrapper-validation check, thanks!

JMX Metrics: metric descriptions updated to match common convention

…emetry-java-contrib into integration-test-kafka

…n tests added (open-telemetry#1670)

Kafka broker, producer and consumer YAMLs and tests added

65b4f3b

github-actions bot assigned breedx-splk, robsunday and SylvainJuge Jan 24, 2025

github-actions bot requested review from breedx-splk and SylvainJuge January 24, 2025 12:38

robsunday commented Jan 24, 2025

View reviewed changes

...a/io/opentelemetry/contrib/jmxscraper/target_systems/kafka/KafkaConsumerIntegrationTest.java Outdated Show resolved Hide resolved

robsunday commented Jan 24, 2025

View reviewed changes

...Test/java/io/opentelemetry/contrib/jmxscraper/target_systems/kafka/KafkaIntegrationTest.java Outdated Show resolved Hide resolved

robsunday commented Jan 24, 2025

View reviewed changes

jmx-scraper/src/main/resources/kafka-consumer.yaml Show resolved Hide resolved

robsunday commented Jan 24, 2025

View reviewed changes

robsunday marked this pull request as ready for review January 24, 2025 12:58

robsunday requested a review from a team as a code owner January 24, 2025 12:58

Merge branch 'open-telemetry:main' into integration-test-kafka

1cf8e5b

SylvainJuge mentioned this pull request Jan 27, 2025

Refactor & merge JMX gatherer & insight implementations #1362

Open

24 tasks

SylvainJuge reviewed Jan 27, 2025

View reviewed changes

Merge branch 'open-telemetry:main' into integration-test-kafka

40299c9

github-actions bot requested a review from SylvainJuge February 4, 2025 12:13

Units updated in groovy files

31c4fc4

Code cleanups Other code review related changes

SylvainJuge approved these changes Feb 5, 2025

View reviewed changes

breedx-splk reviewed Feb 7, 2025

View reviewed changes

breedx-splk approved these changes Feb 7, 2025

View reviewed changes

robsunday mentioned this pull request Feb 10, 2025

[jmx-scraper] Add support for Kafka logging metrics in the integration test #1726

Open

JMX Scraper: reverted some metric name changes

665c390

JMX Metrics: metric descriptions updated to match common convention

github-actions bot requested a review from SylvainJuge February 13, 2025 16:14

robsunday requested a review from breedx-splk February 13, 2025 16:31

robsunday and others added 3 commits February 14, 2025 14:47

Merge branch 'open-telemetry:main' into integration-test-kafka

b8b228d

Merge branch 'main' into integration-test-kafka

8575c9a

Merge branch 'integration-test-kafka' of github.com:robsunday/opentel…

f6a4f18

…emetry-java-contrib into integration-test-kafka

trask merged commit b92a465 into open-telemetry:main Feb 14, 2025
17 checks passed

breedx-splk pushed a commit to breedx-splk/opentelemetry-java-contrib that referenced this pull request Feb 14, 2025

JMX Scraper: Kafka server, producer and consumer YAMLs and integratio…

24d191b

…n tests added (open-telemetry#1670)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JMX Scraper: Kafka server, producer and consumer YAMLs and integration tests added #1670

JMX Scraper: Kafka server, producer and consumer YAMLs and integration tests added #1670

robsunday commented Jan 24, 2025 •

edited

Loading

robsunday Jan 24, 2025

robsunday Jan 24, 2025

robsunday Jan 24, 2025

robsunday Jan 24, 2025

robsunday Jan 24, 2025

robsunday Jan 24, 2025

robsunday Jan 24, 2025

robsunday Jan 24, 2025

SylvainJuge Jan 27, 2025

SylvainJuge Jan 27, 2025

robsunday Feb 4, 2025

breedx-splk Feb 7, 2025

robsunday Feb 10, 2025

breedx-splk left a comment

robsunday commented Feb 10, 2025

trask commented Feb 10, 2025

JMX Scraper: Kafka server, producer and consumer YAMLs and integration tests added #1670

JMX Scraper: Kafka server, producer and consumer YAMLs and integration tests added #1670

Conversation

robsunday commented Jan 24, 2025 • 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

breedx-splk left a comment

Choose a reason for hiding this comment

robsunday commented Feb 10, 2025

trask commented Feb 10, 2025

robsunday commented Jan 24, 2025 •

edited

Loading