Added support for opentelemetry #83

OwenCorrigan76 · 2022-09-27T14:21:57Z

Incorporates Opentelemetry alongside OpenTracing to read traces on Producer, Consumer and Streams.

Added OpenTelemetry dependencies as instructed here: [1]

Required Environmental Variables:

TRACING_SYSTEM - String for activating tracing. accepted values are `jaeger`,  `opentelemetry`.

[1] https://github.com/ppatierno/kafka-opentelemetry

kyguy

Looks great! Just one nit!

java/kafka/streams/pom.xml

java/kafka/consumer/src/main/java/KafkaConsumerExample.java

mimaison · 2022-09-27T14:55:05Z

java/kafka/consumer/src/main/java/KafkaConsumerConfig.java

@@ -34,12 +34,14 @@ public class KafkaConsumerConfig {
    private final String oauthTokenEndpointUri;
    private final String additionalConfig;
    private final String saslLoginCallbackClass = "io.strimzi.kafka.oauth.client.JaasClientOauthLoginCallbackHandler";
+    private final String tracingSystem;
+    private static final String DEFAULT_TRACING_SYSTEM = null;


Strings are null by default so you can remove = null

Without initialising to null, I'm getting the following error - Variable 'DEFAULT_TRACING_SYSTEM' might not have been initialized

mimaison · 2022-09-27T15:00:03Z

java/kafka/consumer/src/main/java/KafkaConsumerConfig.java

@@ -89,14 +93,14 @@ public static Properties createProperties(KafkaConsumerConfig config) {
        props.put(ConsumerConfig.KEY_DESERIALIZER_CLASS_CONFIG, "org.apache.kafka.common.serialization.StringDeserializer");
        props.put(ConsumerConfig.VALUE_DESERIALIZER_CLASS_CONFIG, "org.apache.kafka.common.serialization.StringDeserializer");

-        if (config.getSslTruststoreCertificates() != null)   {
+        if (config.getSslTruststoreCertificates() != null)  {


Nit: If we're removing extra spaces, let's remove them all! Otherwise undo this change.
Same below

mimaison · 2022-09-27T15:01:34Z

java/kafka/consumer/src/main/java/KafkaConsumerExample.java

-            Tracer tracer = Configuration.fromEnv().getTracer();
-            GlobalTracer.registerIfAbsent(tracer);
+        TracingSystem tracingSystem;
+        tracingSystem = TracingSystem.forValue(config.getTracingSystem());


Can we merge these 2 lines?

mimaison · 2022-09-27T15:02:46Z

java/kafka/consumer/src/main/java/KafkaConsumerExample.java


-            props.put(ConsumerConfig.INTERCEPTOR_CLASSES_CONFIG, TracingConsumerInterceptor.class.getName());
+                props.put(ConsumerConfig.INTERCEPTOR_CLASSES_CONFIG, io.opentracing.contrib.kafka.TracingConsumerInterceptor.class.getName());
+            } else if (tracingSystem == KafkaConsumerExample.TracingSystem.OPENTELEMETRY) {


I think you should be able to drop the KafkaConsumerExample. prefix here

Dropped without any issues.

mimaison · 2022-09-27T15:03:10Z

java/kafka/consumer/src/main/java/KafkaConsumerExample.java

@@ -62,4 +87,4 @@ public static void main(String[] args) {
        }
        log.info("Received {} messages", receivedMsgs);
    }
-}
+}


I think we can keep the newline. Same in a few other files below

Do you mean an extra empty line at the bottom? If so, added.

mimaison · 2022-09-27T15:04:14Z

java/kafka/deployment-tracing.yaml

@@ -95,6 +97,8 @@ spec:
              value: const
            - name: JAEGER_SAMPLER_PARAM
              value: "1"
+            - name: TRACING_SYSTEM
+              value: "jaeger"


Do we need the quotes?

Quotes removed

mimaison · 2022-09-27T15:05:12Z

java/kafka/consumer/src/main/java/KafkaConsumerExample.java


-            props.put(ConsumerConfig.INTERCEPTOR_CLASSES_CONFIG, TracingConsumerInterceptor.class.getName());
+                props.put(ConsumerConfig.INTERCEPTOR_CLASSES_CONFIG, io.opentracing.contrib.kafka.TracingConsumerInterceptor.class.getName());


We should keep the import and use the short name instead of this.

The reason the import prefix is added here instead of keeping the import is: Jaeger (opentracing) and opentelemetry use the same class name and the only way i could differencieate between the two is to add the import in the lines of code. Is there another way to do this? Thanks. See below:
props.put(ConsumerConfig.INTERCEPTOR_CLASSES_CONFIG,io.opentracing.contrib.kafka.TracingConsumerInterceptor.class.getName());
props.put(ConsumerConfig.INTERCEPTOR_CLASSES_CONFIG,io.opentelemetry.instrumentation.kafkaclients.TracingConsumerInterceptor.class.getName());

java/kafka/consumer/src/main/java/KafkaConsumerExample.java

java/kafka/pom.xml

java/kafka/producer/src/main/java/KafkaProducerExample.java

java/kafka/streams/src/main/java/KafkaStreamsExample.java

java/kafka/producer/pom.xml

mimaison · 2022-09-27T15:05:49Z

java/kafka/producer/src/main/java/KafkaProducerExample.java

@@ -26,6 +25,21 @@
 public class KafkaProducerExample {
    private static final Logger log = LogManager.getLogger(KafkaProducerExample.class);

+    public enum TracingSystem {


Instead of duplicating this logic, can we extract it into its own class?

java/kafka/consumer/pom.xml

Signed-off-by: Owen <owencorrigan76@gmail.com>

java/kafka/common/src/main/java/TracingSystem.java

java/kafka/consumer/src/main/java/KafkaConsumerExample.java

java/kafka/producer/src/main/java/KafkaProducerExample.java

java/kafka/streams/src/main/java/KafkaStreamsExample.java

java/kafka/streams/src/main/java/TracingKafkaClientSupplier.java

Signed-off-by: Owen <owencorrigan76@gmail.com>

java/kafka/consumer/src/main/java/KafkaConsumerExample.java

Signed-off-by: Owen <owencorrigan76@gmail.com>

java/kafka/consumer/src/main/java/KafkaConsumerExample.java

java/kafka/producer/src/main/java/KafkaProducerExample.java

java/kafka/consumer/src/main/java/KafkaConsumerExample.java

Signed-off-by: Owen <owencorrigan76@gmail.com>

ppatierno

LGTM. Thanks for this work!

scholzj

Pro-tip: If you resolve the addressed comments, the PR will be much more readable.

java/kafka/common/src/main/java/TracingSystem.java

java/kafka/consumer/src/main/java/KafkaConsumerExample.java

java/kafka/deployment-tracing.yaml

Signed-off-by: Owen <owencorrigan76@gmail.com>

scholzj

LGTM. Thanks for the PR.

@ppatierno @mimaison Do you need another pass through it? Or can it be merged?

ppatierno · 2022-10-18T05:00:53Z

It's fine with me

* Add OpenTelemetry support Signed-off-by: Owen <owencorrigan76@gmail.com>

kyguy requested review from ppatierno, mimaison and kyguy September 27, 2022 14:22

kyguy approved these changes Sep 27, 2022

View reviewed changes

java/kafka/streams/pom.xml Outdated Show resolved Hide resolved

kyguy requested a review from scholzj September 27, 2022 14:36

scholzj reviewed Sep 27, 2022

View reviewed changes

java/kafka/consumer/src/main/java/KafkaConsumerExample.java Outdated Show resolved Hide resolved

java/kafka/consumer/src/main/java/KafkaConsumerExample.java Outdated Show resolved Hide resolved

mimaison reviewed Sep 27, 2022

View reviewed changes

ppatierno requested changes Sep 27, 2022

View reviewed changes

mimaison reviewed Sep 27, 2022

View reviewed changes

ppatierno reviewed Sep 29, 2022

View reviewed changes

java/kafka/consumer/pom.xml Outdated Show resolved Hide resolved

OwenCorrigan76 force-pushed the strimzi-client-examples-opentracing-and-opentelemetry branch from c9adda3 to f82401c Compare October 7, 2022 15:47

Add OpenTelemetry support

c167192

Signed-off-by: Owen <owencorrigan76@gmail.com>

OwenCorrigan76 force-pushed the strimzi-client-examples-opentracing-and-opentelemetry branch from f82401c to c167192 Compare October 7, 2022 15:57

Nits addressed

66475db

Signed-off-by: Owen <owencorrigan76@gmail.com>

OwenCorrigan76 requested review from mimaison, ppatierno, kyguy and scholzj and removed request for mimaison, ppatierno, kyguy and scholzj October 10, 2022 08:46

OpenTelemetry Support

6e4587d

Signed-off-by: Owen <owencorrigan76@gmail.com>

OpenTelemetry Support fix build error

7aec81b

Signed-off-by: Owen <owencorrigan76@gmail.com>

ppatierno requested changes Oct 10, 2022

View reviewed changes

Addressing Paolo's comments

af66c8d

Signed-off-by: Owen <owencorrigan76@gmail.com>

OwenCorrigan76 requested a review from ppatierno October 10, 2022 15:49

ppatierno requested changes Oct 11, 2022

View reviewed changes

java/kafka/consumer/src/main/java/KafkaConsumerExample.java Outdated Show resolved Hide resolved

java/kafka/consumer/src/main/java/KafkaConsumerExample.java Outdated Show resolved Hide resolved

Addressing Indentation

244262d

Signed-off-by: Owen <owencorrigan76@gmail.com>

kyguy reviewed Oct 11, 2022

View reviewed changes

Opentelemetry SDK put back, StreamConfig Refactored

00e9cad

Signed-off-by: Owen <owencorrigan76@gmail.com>

OwenCorrigan76 requested a review from ppatierno October 12, 2022 12:03

ppatierno approved these changes Oct 12, 2022

View reviewed changes

OwenCorrigan76 requested a review from scholzj October 12, 2022 15:42

scholzj reviewed Oct 13, 2022

View reviewed changes

OpenTelemetry Support for Client-Examples

eb8f4f6

Signed-off-by: Owen <owencorrigan76@gmail.com>

OwenCorrigan76 requested a review from scholzj October 17, 2022 14:39

scholzj approved these changes Oct 17, 2022

View reviewed changes

OwenCorrigan76 requested a review from mimaison October 18, 2022 08:06

scholzj merged commit 205d594 into strimzi:main Oct 18, 2022

OwenCorrigan76 added a commit to OwenCorrigan76/client-examples that referenced this pull request Jan 30, 2024

Added support for opentelemetry (strimzi#83)

78069aa

* Add OpenTelemetry support Signed-off-by: Owen <owencorrigan76@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for opentelemetry #83

Added support for opentelemetry #83

OwenCorrigan76 commented Sep 27, 2022

kyguy left a comment

mimaison Sep 27, 2022

OwenCorrigan76 Sep 29, 2022

mimaison Sep 27, 2022 •

edited

Loading

OwenCorrigan76 Sep 29, 2022

mimaison Sep 27, 2022

OwenCorrigan76 Sep 29, 2022

mimaison Sep 27, 2022

OwenCorrigan76 Sep 29, 2022

mimaison Sep 27, 2022 •

edited

Loading

OwenCorrigan76 Sep 29, 2022

mimaison Sep 27, 2022

OwenCorrigan76 Sep 29, 2022

mimaison Sep 27, 2022

OwenCorrigan76 Sep 29, 2022 •

edited

Loading

mimaison Sep 27, 2022

ppatierno left a comment

scholzj left a comment

scholzj left a comment

ppatierno commented Oct 18, 2022


		props.put(ConsumerConfig.INTERCEPTOR_CLASSES_CONFIG, TracingConsumerInterceptor.class.getName());
		props.put(ConsumerConfig.INTERCEPTOR_CLASSES_CONFIG, io.opentracing.contrib.kafka.TracingConsumerInterceptor.class.getName());

Added support for opentelemetry #83

Added support for opentelemetry #83

Conversation

OwenCorrigan76 commented Sep 27, 2022

kyguy 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 Sep 27, 2022 • 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

mimaison Sep 27, 2022 • 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

OwenCorrigan76 Sep 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ppatierno left a comment

Choose a reason for hiding this comment

scholzj left a comment

Choose a reason for hiding this comment

scholzj left a comment

Choose a reason for hiding this comment

ppatierno commented Oct 18, 2022

mimaison Sep 27, 2022 •

edited

Loading

mimaison Sep 27, 2022 •

edited

Loading

OwenCorrigan76 Sep 29, 2022 •

edited

Loading