Add OpenTelemetry support #633

alesj · 2022-06-29T14:04:49Z

This PR adds OpenTelemetry support.
It abstract previous OpenTracing / Jaeger usage -- adding thin abstraction layer to hide actual implementation usage.
Most of the old code was used as it is, only poll or consume was changed away from interceptor usage - as we couldn't model trace/spans as expected.
Async send needed some extra work for OTel, due to its ThreadLocal current span propagation.

Current bridge.tracing property must be set to opentelemetry to use OTel tracing.

config/application.properties

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/tracing/SpanHandle.java

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

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

tombentley · 2022-07-01T13:50:41Z

@alesj please could you provide a description for this PR, and an issue reference if there is one? Thanks.

alesj · 2022-07-05T12:52:16Z

@alesj please could you provide a description for this PR, and an issue reference if there is one? Thanks.

Short description provided.
No issue ref afaik.

ppatierno · 2022-07-06T13:12:05Z

I created a corresponding issue for tracking #636

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

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

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

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

tombentley

Thanks @alesj! I left a few comments and questions.

pom.xml

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

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

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

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

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

src/main/java/io/strimzi/kafka/bridge/http/HttpSourceBridgeEndpoint.java

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

tombentley · 2022-07-07T15:31:24Z

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

+    @BeforeEach
+    public void setUp() {
+        assumeServer(String.format("http://%s:%s", Urls.BRIDGE_HOST, Urls.BRIDGE_PORT)); // bridge
+        assumeServer("http://localhost:16686"); // jaeger


I think it would be better if we could use testcontainers to spin up a server. I admit I've no idea how much work that would really, but maybe it's not so much?

With testcontainers it's chicken-n-egg problem ... you would first need to build latest Bridge image, only to be used in tests then.
Perhaps I can easily make it work with running Application::main with the right args ...

Ah, now I know why I added that service.name property to Bridge config ... otherwise you can only enable OpenTracing via env vars ... which makes it hard (if not impossible) to test.

Something like this is then required - custom generic containers:

https://github.com/alesj/test-clients/tree/tests1/testsuite/src/test/java/io/strimzi/testclients/test/support

I can give it a shot if it's really needed ...

Looking at Ales's branch I'm inclined to thiat bridge support for the test-clients would be a great addition. @ppatierno @scholzj wdyt?

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

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

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

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

ppatierno · 2022-07-13T08:54:43Z

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

+    protected abstract TracingOptions tracingOptions();
+
+    @Test
+    public void testSmoke(VertxTestContext context) throws Exception {


I am not sure what we are testing here. Actually we are using the vertx-tracing support just for enabling tracing on the client side (because the Vertx instance we are creating is used for the web client part) but then we are sending/receiving messages but when actually the tracing part is tested? Tbh I don't think we had anything before as well, but this test is not adding much more.

Yeah, atm the 'test' only adds simple automation of send+receive -- so you don't have to do it completely manually (via curl, postman, etc).

@tombentley do you think we should keep it anyway or just remove? Maybe opening an issue about adding proper tracing tests later?

Maybe opening an issue about adding proper tracing tests later?

+1

ppatierno · 2022-07-15T11:00:09Z

With the current status of the PR we are avoiding the hack we had before about tracking spans with a UUID for using the right tracing context in the Kafka producer interceptor due to the async nature of Vert.x and different threads usage.
We are now using the VertxContextStorageProvider which an implementation of the context storage for OpenTelemetry in the Vert.x land which takes into account bringing OpenTelemetry context via Vert.x context.

Unluckily the current solution has a problem when an HTTP client uses the "keep-alive" connection enabled.
In this scenario, when the bridge is used to send a message, on the first request the HTTP send span and the underneath Kafka send message span are related correctly (the latter is CHILD OF the former).
All the subsequent HTTP send requests are not traced correctly because the above two spans are totally independent.
After a long investigation, it turned out that it is due to the Vert.x Kafka client producer on the bridge being the same and reused on all HTTP requests when "keep-alive" connection is enabled.
The issue is that the Vert.x Kafka client is taking the initial Vert.x context when it's created for the first time (on the first HTTP request and connection established), and then it uses this one for executing any other blocking call (see the Kafka send).
Of course, each incoming HTTP request has its own context (bringing tracing) instead.
So the first time everything works as expected because the two contexts are actually the same. From the second request, the Kafka client uses the first "old" context (with no tracing anymore) and not the right "current" context (bringing tracing).

Of course with no "keep-alive" everything works fine each time, because a new Kafka client is created for every HTTP request and the context is always the "right" one.
We are in touch with Vert.x people to get advice and about the behaviour of the Vert.x Kafka client.

Meanwhile we already have the "workaround" which is actually a middle solution (pushed by Ales during these days to avoid the above mentioned hack) about using an executor service which is wrapped through the Context (in OpenTelemetry) in order to use the right context in Vert.x.
This solution was suggested by OpenTelemetry people and it seems to be the "official" one when you don't have the right context storage (which we have but driving to the above problem).

This is more or less the status of the art.

If there is no trick for the Vert.x Kafka client or we would need a change on the client so waiting for a new release, I would move forward with this PR by using the last "workaround" and then tracking an issue to come back using the storage later.

Last consideration is that Vert.x seems to have an official support for tracing now, both for OpenTelemetry and OpenTracing but seems to be still in TP and anyway using it would need a more bigger refactoring on the bridge because such support handle the HTTP part of the tracing for us our of box, so we should remove our code. I am against this solution in the near future.

I would like to know what the other maintainers think about the overall picture of this. @tombentley @scholzj ?

bin/kafka_bridge_run.sh

ppatierno · 2022-07-21T13:18:05Z

Thanks to Julien from the Vert.x team, we have got a fix in the Vert.x Kafka client (vert-x3/vertx-kafka-client#226) which I tested and seems to work. Of course, this will be available in the next Vert.x 4.3.3 release. An ETA seems to be August for having this release.
At this point we have two choices: updating this PR with the workaround of wrapping the Vert.x executor context while waiting for the Vert.x 4.3.3 release with a proper fix or ... waiting for the Vert.x 4.3.3 release and not merging this PR yet.
I have no strong opinion. Waiting for the Vert.x release could mean even another month, but I don't think we are going to have another bridge release soon.

@tombentley @scholzj what's your feeling on this?

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

Remove OTel span propagation hack. Simplify executor service adapt. Signed-off-by: Ales Justin <ales.justin@gmail.com>

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

pom.xml

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

ppatierno · 2022-08-09T19:06:23Z

Finally Vert.x 4.3.3 is out! @alesj could you update the PR by using this release and test again that everything works fine please?
I am ok with the PR but I will have another final pass.
@scholzj @tombentley could you review please, it would be great to finally close this addition after so long time :-)

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

ppatierno

@alesj I left some comments mostly around dependencies that can be safely removed

pom.xml

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

src/main/java/io/strimzi/kafka/bridge/http/HttpSinkBridgeEndpoint.java

src/main/java/io/strimzi/kafka/bridge/http/HttpSourceBridgeEndpoint.java

tombentley · 2022-08-10T10:59:12Z

src/main/java/io/strimzi/kafka/bridge/http/HttpSourceBridgeEndpoint.java

@@ -96,70 +85,45 @@ public void handle(Endpoint<?> endpoint) {

        boolean isAsync = Boolean.parseBoolean(routingContext.queryParams().get("async"));

-        Tracer tracer = GlobalTracer.get();


Not for this PR I guess, but this handle() method is pretty complex. We should try to refactor it into smaller methods.

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

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

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

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

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

src/main/java/io/strimzi/kafka/bridge/http/HttpSourceBridgeEndpoint.java

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

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

ppatierno

LGTM! @alesj thanks for the effort!

src/main/java/io/strimzi/kafka/bridge/http/HttpSourceBridgeEndpoint.java

tombentley

Thanks Ales!

alesj requested a review from ppatierno June 29, 2022 14:04

alesj mentioned this pull request Jun 29, 2022

Add OpenTelemetry support. #556

Closed

alesj force-pushed the otel_refs_fix branch from cd28a4e to 4a7720a Compare June 29, 2022 14:15

ppatierno changed the title ~~Fix how send/poll and its messages are referenced~~ Add OpenTelemetry support Jun 30, 2022

ppatierno reviewed Jun 30, 2022

View reviewed changes

alesj force-pushed the otel_refs_fix branch from 4a7720a to 703bff5 Compare July 5, 2022 12:45

alesj force-pushed the otel_refs_fix branch from 703bff5 to 0f4b7b9 Compare July 5, 2022 13:01

ppatierno added this to the 0.22.0 milestone Jul 6, 2022

ppatierno reviewed Jul 6, 2022

View reviewed changes

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

ppatierno reviewed Jul 6, 2022

View reviewed changes

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

scholzj modified the milestones: 0.21.6, 0.22.0 Jul 6, 2022

ppatierno reviewed Jul 6, 2022

View reviewed changes

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

alesj force-pushed the otel_refs_fix branch from 0f4b7b9 to 9aff074 Compare July 7, 2022 11:33

tombentley reviewed Jul 7, 2022

View reviewed changes

alesj force-pushed the otel_refs_fix branch 2 times, most recently from 7eee00a to 36240b5 Compare July 8, 2022 14:15

ppatierno requested changes Jul 9, 2022

View reviewed changes

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

alesj force-pushed the otel_refs_fix branch from e20ddb5 to 7b27704 Compare July 11, 2022 12:45

ppatierno requested changes Jul 11, 2022

View reviewed changes

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

ppatierno reviewed Jul 11, 2022

View reviewed changes

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

alesj force-pushed the otel_refs_fix branch 2 times, most recently from 9bd61e1 to 65d6e10 Compare July 12, 2022 09:05

ppatierno requested changes Jul 12, 2022

View reviewed changes

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

ppatierno reviewed Jul 13, 2022

View reviewed changes

ppatierno reviewed Jul 18, 2022

View reviewed changes

bin/kafka_bridge_run.sh Show resolved Hide resolved

ppatierno mentioned this pull request Jul 21, 2022

KafkaWriteStream uses the caller context to execute internal worker tasks. vert-x3/vertx-kafka-client#226

Merged

alesj added 6 commits July 26, 2022 10:25

Add OpenTelemetry support.

f271e89

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

Fix async scope/context handling in OTel.

0300225

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

Fix relationship handling.

9ce78c6

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

Upgrade libs, and consume test

d656f53

Remove OTel span propagation hack. Simplify executor service adapt. Signed-off-by: Ales Justin <ales.justin@gmail.com>

Remove OTel executor service workaround.

15988db

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

Add javadoc

7c7d759

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

alesj force-pushed the otel_refs_fix branch from f976d5b to 7c7d759 Compare July 26, 2022 08:26

Remove code setup of required config via env vars / sys props.

576cdaa

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

ppatierno requested changes Aug 1, 2022

View reviewed changes

pom.xml Outdated Show resolved Hide resolved

Remove gRPC impl dependency - not needed.

656dd20

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

alesj force-pushed the otel_refs_fix branch from 1aa6bfa to 656dd20 Compare August 2, 2022 09:09

Upgrade Vert.x to 4.3.3.

6b62e57

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

ppatierno requested changes Aug 10, 2022

View reviewed changes

pom.xml Outdated Show resolved Hide resolved

pom.xml Outdated Show resolved Hide resolved

pom.xml Show resolved Hide resolved

pom.xml Outdated Show resolved Hide resolved

pom.xml Outdated Show resolved Hide resolved

pom.xml Outdated Show resolved Hide resolved

alesj added 2 commits August 10, 2022 12:40

Update OTel version, remove unused configuration.

d5b6e9e

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

Return dependency -- it's needed after all.

34f785d

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

tombentley reviewed Aug 10, 2022

View reviewed changes

alesj added 2 commits August 10, 2022 14:14

Reduce span builder usage.

6cd4dbb

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

Ignore OTel metrics in OpenTracing test.

b1c9448

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

ppatierno requested changes Aug 10, 2022

View reviewed changes

src/main/java/io/strimzi/kafka/bridge/http/HttpSourceBridgeEndpoint.java Outdated Show resolved Hide resolved

src/main/java/io/strimzi/kafka/bridge/http/HttpSourceBridgeEndpoint.java Show resolved Hide resolved

ppatierno requested changes Aug 10, 2022

View reviewed changes

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

Revert some changes, add more docs.

8f4310e

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

ppatierno approved these changes Aug 11, 2022

View reviewed changes

src/main/java/io/strimzi/kafka/bridge/http/HttpSourceBridgeEndpoint.java Show resolved Hide resolved

tombentley approved these changes Aug 11, 2022

View reviewed changes

ppatierno merged commit ad42485 into strimzi:main Aug 11, 2022

ppatierno mentioned this pull request Aug 13, 2022

Add support for OpenTelemetry #636

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add OpenTelemetry support #633

Add OpenTelemetry support #633

alesj commented Jun 29, 2022 •

edited

Loading

tombentley commented Jul 1, 2022

alesj commented Jul 5, 2022

ppatierno commented Jul 6, 2022

tombentley left a comment

tombentley Jul 7, 2022

alesj Jul 8, 2022

alesj Jul 8, 2022

alesj Jul 8, 2022 •

edited

Loading

tombentley Jul 8, 2022

ppatierno Jul 13, 2022

alesj Jul 13, 2022

ppatierno Jul 13, 2022

alesj Jul 13, 2022

ppatierno commented Jul 15, 2022

ppatierno commented Jul 21, 2022

ppatierno commented Aug 9, 2022

ppatierno left a comment

tombentley Aug 10, 2022

ppatierno left a comment

tombentley left a comment

		@@ -96,70 +85,45 @@ public void handle(Endpoint<?> endpoint) {

		boolean isAsync = Boolean.parseBoolean(routingContext.queryParams().get("async"));

		Tracer tracer = GlobalTracer.get();

Add OpenTelemetry support #633

Add OpenTelemetry support #633

Conversation

alesj commented Jun 29, 2022 • edited Loading

tombentley commented Jul 1, 2022

alesj commented Jul 5, 2022

ppatierno commented Jul 6, 2022

tombentley 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

alesj Jul 8, 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

ppatierno commented Jul 15, 2022

ppatierno commented Jul 21, 2022

ppatierno commented Aug 9, 2022

ppatierno left a comment

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

tombentley left a comment

Choose a reason for hiding this comment

alesj commented Jun 29, 2022 •

edited

Loading

alesj Jul 8, 2022 •

edited

Loading