Add OpenTelemetry support. #556

alesj · 2021-11-07T11:25:28Z

Signed-off-by: Ales Justin ales.justin@gmail.com

config/application.properties

ppatierno · 2022-01-18T14:02:37Z

pom.xml

@@ -106,6 +106,9 @@
 		<jaeger.version>1.6.0</jaeger.version>
 		<opentracing.version>0.33.0</opentracing.version>
 		<opentracing-kafka-client.version>0.1.15</opentracing-kafka-client.version>
+		<opentelemetry.version>1.7.0-alpha</opentelemetry.version>
+		<opentelemetry-stable.version>1.7.0</opentelemetry-stable.version>


why there are two opentelemetry version properties (one with -stable and the other seems to be an alpha) using both in different places to import dependencies?

"My" OpenTelemetry artifacts did yet made it to "stable" version, where Jaeger artifcat alfready did.
Hence the 2 diff versions.
But we agreed it's OK to use alpha for those -- as they are the initial ones, and haven't been changed.
I'll check if there is a stable version, but shouldn't make a diff.
(if I change it again now, I need to re-run the whole cycle, for the N-th time ...)

Did you check if there is a stable version? This "alpha" here bothers me a little bit :-)

Let me check ...

https://search.maven.org/artifact/io.opentelemetry.instrumentation/opentelemetry-kafka-clients-common

All alphas ... but that's OK, since, afaik / imho, the 1st didn't change at all ...

ppatierno · 2022-01-18T14:03:55Z

src/main/java/io/strimzi/kafka/bridge/Application.java

@@ -62,12 +61,13 @@
    private static final int DEFAULT_EMBEDDED_HTTP_SERVER_PORT = 8080;

    @SuppressWarnings({"checkstyle:NPathComplexity"})
+    @SuppressFBWarnings("REC_CATCH_EXCEPTION")


why this one?

When I ran mvn clean install -DskipTests spotbugs:check it spit out this warning / error ...

this is not the reason, this is the detection of the problem ... I would like to know why there is now a problem that needs this suppress warning that wasn't there before.

No idea, I was just following spotbugs error report.
Perhaps checkstyle plugin upgrade?

I would try to understand what's the error and then at least opening an issue with the reason to track and doing a change later to avoid doing it in the current PR. If spotbugs is raising this maybe we don't see a potential bug ... it's worth investigating.

I think it's the issue of "eating" the exception:

https://github.com/archived-codacy/codacy-findbugs/blob/master/src/main/resources/docs/description/REC_CATCH_EXCEPTION.md

Which in our case that's not (completely) the case -- we log and exit the app ...

The link you pasted seems to suggest that we should not catch just Exception as the main does but RuntimeException or being more specific. Would it be the solution instead of suppress the warning?

Imho, we can leave it as is ... as that's from main() and we log and exit.

ppatierno · 2022-01-18T14:04:25Z

src/main/java/io/strimzi/kafka/bridge/MetricsReporter.java

@@ -24,6 +25,7 @@
     * @param jmxCollectorRegistry JmxCollectorRegistry instance for scraping metrics from JMX endpoints
     * @param meterRegistry MeterRegistry instance for scraping metrics exposed through Vert.x
     */
+    @SuppressFBWarnings("SIC_INNER_SHOULD_BE_STATIC_ANON")


why this one?

Same here, checkstyle / spotbugs complained ...

ditto as above

Same ... as you can see, I didn't change anything else in this class.

src/main/java/io/strimzi/kafka/bridge/tracing/OpenTelemetryHandle.java

scholzj

Did you test the backwards compatibility? I.e. that the old OpenTracing tracing works?

config/application.properties

pom.xml

src/main/java/io/strimzi/kafka/bridge/Application.java

src/main/java/io/strimzi/kafka/bridge/MetricsReporter.java

src/main/java/io/strimzi/kafka/bridge/converter/DefaultDeserializer.java

src/main/java/io/strimzi/kafka/bridge/tracing/OpenTracingHandle.java

src/main/java/io/strimzi/kafka/bridge/tracing/TracingUtil.java

src/test/java/io/strimzi/kafka/bridge/tracing/OpenTelemetryTest.java

src/main/java/io/strimzi/kafka/bridge/tracing/TracingConstants.java

src/main/java/io/strimzi/kafka/bridge/tracing/TracingHandle.java

scholzj

The good news ... the OpenTracing still seems to work fine :-). But I didn't managed to get anything using OTel. When I try to follow the docs PR from @PaulRMellor, it always fails to connect to my Jaeger. Do you have some example configurations for Jaeger Operator and for the agent to play with?

scholzj · 2022-02-02T20:20:34Z

bin/kafka_bridge_run.sh

+# enabling OpenTelemetry with Jaeger
+if [ -n "$OTEL_SERVICE_NAME" ]; then
+  export OTEL_TRACES_EXPORTER="jaeger"
+fi
+


Why should this be in this script and not in bin/docker/kafka_bridge_run.sh? That script is supposed to be used by users running the bridge on their own. they would not configure OTEL_SERVICE_NAME but they would edit the configuration file. Should this be configured in the config file instead?

scholzj · 2022-02-02T20:22:33Z

config/application.properties

+#bridge.tracing=jaeger-otel
+#bridge.tracing.service-name=strimzi-kafka-bridge


Apart from the exporter configuration I commented abotu above => should this also configure the address of the Jaeger agent etc.? Or maybe not => we didn't do anything special for OpenTracing configuration. I guess the bridge.tracing.service-name is confusing. Why do we have it here and the others not?

Dunno. @ppatierno ? ^

@alesj but the current bridge doesn't have such field in the properties file. It has just bridge.tracing=jaeger that you changed into bridge.tracing=jaeger-otel. You added bridge.tracing.service-name, why?

So you could customize your spans / traces names.
It can be fixed to strimzi-kafka-bridge.

I guess the point is, how does it differ from the environment variable and from the system property?

I think we follow the same order -- env, sys, config.
I'm happy to drop any or all of them.

The same order as what?

As how OTel lookups up config -- env, sys, custom config

So ... the bridge is using the Vert.x Config for getting the configuration.
As you can see here we provides two stores for configuration: file (the application.properties) and env vars. Any env var value overrides the same parameter specified in the properties file (because how stores are added matters and in our case is fileStore first and then envStore as explained here).

OK, I'll remove sys prop lookup.
Where I think the lookup order is then OK -- env first, if no env, then config prop.

scholzj · 2022-02-02T20:45:26Z

config/application.properties

 #bridge.tracing=jaeger
+# OpenTelemetry support
+#bridge.tracing=jaeger-otel


This might be my misunderstanding. But do we still need the jaeger part here? Shouldn't the type be just something like opentracing or otel? I thought OpenTelemetry should abstract from the implementations. Not necessarily saying it is wrong, just curious if it is still needed.

Not needed of course.
It was jaeger for some unknown reason -- as if there was ever any other option? -- so @ppatierno ? ^^
... and I just followed that unknown reason ...

Let me rephrase the question ... what it the exact relationship between OTel and Jaeger and how do things like Zipkin fit in?

They don't. :-)
Jaeger is a fixed choice, so imho not a good choice of config value.

What does it mean that it is a fixed choice?

Yeah ... at the end it's that exporter config + right deps on the classpath.
I can add Zipkin, etc ... if you want.

You do not have to add Zipkin. But this clearly impacts the configuration and the initial question.

the reason was this

strimzi-kafka-bridge/src/main/java/io/strimzi/kafka/bridge/Application.java

Line 138 in f96cb3f

if ("jaeger".equals(bridgeConfig.getTracing())) {

... this bridge.tracing is used to enable tracing and we used jaeger thinking about potential future value with other tracing systems. It's even reflected from the CO when configuring the tracing on the bridge through the STRIMZI_TRACING env var here https://github.com/strimzi/strimzi-kafka-bridge/blob/main/bin/docker/kafka_bridge_config_generator.sh#L4

Ah, OK ... this needs to be fixed then as well.

So I guess `jaeger-otel' is not a good name?
Or, actually, we need a new differentiator? By tracing type not impl ...

Ah, OK ... this needs to be fixed then as well.

OK, this is fine, it takes the env var ...

scholzj · 2022-02-02T20:56:17Z

pom.xml

+		<dependency>
+			<groupId>io.grpc</groupId>
+			<artifactId>grpc-netty-shaded</artifactId>
+			<version>${grpc.version}</version>
+		</dependency>


Getting back to this ... are we actually using GRPC? The OpenTracing IMHO used Thrift.

https://github.com/open-telemetry/opentelemetry-java/tree/main/sdk-extensions/autoconfigure#jaeger-exporter

The [Jaeger](https://www.jaegertracing.io/docs/1.21/apis/#protobuf-via-grpc-stable) exporter. This exporter uses gRPC for its communications protocol.

scholzj · 2022-02-02T21:05:11Z

src/main/java/io/strimzi/kafka/bridge/tracing/OpenTelemetryHandle.java

+    public String serviceName(BridgeConfig config) {
+        String serviceName = System.getenv(envName());
+        if (serviceName == null) {
+            serviceName = System.getProperty(OPENTELEMETRY_SERVICE_NAME_PROPERTY_KEY);


What is the reason for getting it from the properties suddenly? Do we have 3 way s how to configure the service? In envvar., system propeerty and in the config file?

That's how OTel uses this -- env, sys -- but it can be left out.
@ppatierno ^

What's the question for me? The tracing with Jaeger used this way in the main application
https://github.com/strimzi/strimzi-kafka-bridge/blob/main/src/main/java/io/strimzi/kafka/bridge/Application.java#L139

Do we go with all 3 checks or not? Or just exact config property?

@alesj I didn't get the question for me? Isn't it the code you added. If you want to know how bridge configuration works, take a look at the previous comment about using Vert.x Config so in our case (due to stores order) env vars first and application.properties file ... but no system properties at all.

OK, as "said" just before, I'll remove sys prop lookup.

alesj · 2022-02-03T10:43:12Z

The good news ... the OpenTracing still seems to work fine :-). But I didn't managed to get anything using OTel. When I try to follow the docs PR from @PaulRMellor, it always fails to connect to my Jaeger. Do you have some example configurations for Jaeger Operator and for the agent to play with?

This is the env vars I have (plus enabled jaeger-otel tracing in config)

OTEL_TRACES_EXPORTER=jaeger;JAEGER_SERVICE_NAME=strimzi-kafka-bridge

and it works for me from IDE.

scholzj · 2022-02-03T13:01:40Z

This is the env vars I have (plus enabled jaeger-otel tracing in config)

OTEL_TRACES_EXPORTER=jaeger;JAEGER_SERVICE_NAME=strimzi-kafka-bridge

and it works for me from IDE.

Ok, so how do you do it on Kube with Jaeger operator?

alesj · 2022-02-03T14:57:37Z

This is the env vars I have (plus enabled jaeger-otel tracing in config)
OTEL_TRACES_EXPORTER=jaeger;JAEGER_SERVICE_NAME=strimzi-kafka-bridge
and it works for me from IDE.

Ok, so how do you do it on Kube with Jaeger operator?

I think the only thing missing here is the OTEL_EXPORTER_JAEGER_ENDPOINT in k8s.

src/main/java/io/strimzi/kafka/bridge/tracing/OpenTelemetryHandle.java

config/application.properties

scholzj · 2022-02-23T14:08:22Z

bin/docker/kafka_bridge_run.sh

+# enabling OpenTelemetry with Jaeger by default
+if [ -n "$OTEL_SERVICE_NAME" ] && [ -z "$OTEL_TRACES_EXPORTER" ]; then
+  export OTEL_TRACES_EXPORTER="jaeger"
+fi


Should this be gated by STRIMZI_TRACING as well? That is what enabled the tracing in the first place.

Imo no need for STRIMZI_TRACING gates.
I'll remove this snippet, and just leave similar one in bin/kafka_bridge_run.sh

bin/kafka_bridge_run.sh

pom.xml

Signed-off-by: Ales Justin <ales.justin@gmail.com>

alesj · 2022-06-29T14:05:30Z

Closed in favor of a new PR: #633

alesj requested a review from ppatierno November 7, 2021 11:25

alesj force-pushed the otel1 branch from 11db677 to 8244df0 Compare November 7, 2021 11:42

scholzj requested a review from sknot-rh November 8, 2021 10:14

scholzj added this to the 0.21.0 milestone Nov 8, 2021

alesj force-pushed the otel1 branch 2 times, most recently from 3e066fe to 0929c8c Compare November 22, 2021 14:16

alesj force-pushed the otel1 branch from 0929c8c to 6d29db8 Compare December 6, 2021 09:56

scholzj modified the milestones: 0.21.0, 0.22.0 Dec 7, 2021

alesj force-pushed the otel1 branch from 6d29db8 to 82414a7 Compare December 27, 2021 00:47

alesj force-pushed the otel1 branch from 82414a7 to 6f407d7 Compare January 5, 2022 08:52

alesj force-pushed the otel1 branch from 6f407d7 to e7ba509 Compare January 12, 2022 09:01

ppatierno reviewed Jan 18, 2022

View reviewed changes

alesj force-pushed the otel1 branch 2 times, most recently from 6ca8160 to cea0565 Compare January 19, 2022 09:37

ppatierno approved these changes Jan 25, 2022

View reviewed changes

scholzj reviewed Jan 27, 2022

View reviewed changes

alesj force-pushed the otel1 branch from cea0565 to e5878f8 Compare January 31, 2022 12:23

scholzj reviewed Jan 31, 2022

View reviewed changes

src/main/java/io/strimzi/kafka/bridge/tracing/TracingConstants.java Outdated Show resolved Hide resolved

src/main/java/io/strimzi/kafka/bridge/tracing/TracingHandle.java Outdated Show resolved Hide resolved

alesj force-pushed the otel1 branch 2 times, most recently from 452827f to 494dc8b Compare January 31, 2022 19:17

scholzj reviewed Feb 2, 2022

View reviewed changes

alesj force-pushed the otel1 branch from 494dc8b to 828eb17 Compare February 3, 2022 10:01

scholzj reviewed Feb 3, 2022

View reviewed changes

src/main/java/io/strimzi/kafka/bridge/tracing/OpenTelemetryHandle.java Outdated Show resolved Hide resolved

alesj force-pushed the otel1 branch 3 times, most recently from 5229002 to 9e909ca Compare February 7, 2022 11:16

scholzj modified the milestones: 0.22.0, 0.23.0 Feb 7, 2022

alesj force-pushed the otel1 branch from 9e909ca to 8220c85 Compare February 11, 2022 13:44

alesj force-pushed the otel1 branch 3 times, most recently from 0689fa7 to 52d88be Compare February 23, 2022 10:35

scholzj reviewed Feb 23, 2022

View reviewed changes

alesj force-pushed the otel1 branch from 52d88be to f172533 Compare February 23, 2022 18:36

Add OpenTelemetry support.

de79017

Signed-off-by: Ales Justin <ales.justin@gmail.com>

alesj force-pushed the otel1 branch from f172533 to de79017 Compare February 24, 2022 14:20

ppatierno self-requested a review April 5, 2022 15:13

scholzj modified the milestones: 0.21.5, 0.22.0 Apr 20, 2022

alesj closed this Jun 29, 2022

		#bridge.tracing=jaeger-otel
		#bridge.tracing.service-name=strimzi-kafka-bridge

Add OpenTelemetry support. #556

Add OpenTelemetry support. #556

Conversation

alesj commented Nov 7, 2021

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

alesj Jan 21, 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

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

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

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

Choose a reason for hiding this comment

alesj commented Feb 3, 2022

scholzj commented Feb 3, 2022

alesj commented Feb 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alesj commented Jun 29, 2022

alesj Jan 21, 2022 •

edited

Loading