Spring Integration javaagent instrumentation by mateuszrzeszutek · Pull Request #3295 · open-telemetry/opentelemetry-java-instrumentation

mateuszrzeszutek · 2021-06-14T09:09:36Z

No description provided.

mateuszrzeszutek · 2021-06-14T09:10:33Z

instrumentation/spring/spring-integration-4.1/javaagent/spring-integration-4.1-javaagent.gradle

+ext {
+  // context "leak" here is intentional: spring-integration instrumentation will always override
+  // "local" span context with one extracted from the incoming message
+  doNotFailOnContextLeak = true
+}


This is a consequence of always using the context extracted from the incoming message; we completely ignore the "local" current context, but that's fine in that case.

Hmm... our debugContextLeakIfEnabled should not be called at all in case of CONSUMER spans. This is not spring integration specific problem.

It's called for all spans, span kind does not matter.

Yes, but the logic of not failing the test should be the same for all CONSUMER spans.

...pring/spring-integration-4.1/javaagent/src/test/groovy/SpringIntegrationAndRabbitTest.groovy

.../spring/spring-integration-4.1/testing/src/main/groovy/AbstractComplexPropagationTest.groovy

gradle/instrumentation.gradle

iNikem · 2021-06-14T14:06:23Z

instrumentation/spring/spring-integration-4.1/javaagent/spring-integration-4.1-javaagent.gradle

+ext {
+  // context "leak" here is intentional: spring-integration instrumentation will always override
+  // "local" span context with one extracted from the incoming message
+  doNotFailOnContextLeak = true
+}


Hmm... our debugContextLeakIfEnabled should not be called at all in case of CONSUMER spans. This is not spring integration specific problem.

...pring/spring-integration-4.1/javaagent/src/test/groovy/SpringIntegrationAndRabbitTest.groovy

mateuszrzeszutek · 2021-06-15T13:16:09Z

After discussing this with @iNikem I've changed the way spans are created by the spring-integration instrumentation:

If there's an ongoing CONSUMER (or SERVER, as it's also a top-level span) this instrumentation will not attempt to extract context from the incoming message, it will only propagate/inject the current context to the message. That is because if there's already a CONSUMER span then some other messaging instrumentation must have created it (and if there's a SERVER span, then most likely it means that spring-integration is not used in a messaging scenario)
If there's no ongoing CONSUMER (or SERVER) span, this instrumentation will extract the parent span context from the incoming message and create a new CONSUMER span, and propagate it by injecting the context to the message. This situation should occur when spring-integration code is the first one to receive a message produced in some other process, and there's no other messaging instrumentation.

mateuszrzeszutek · 2021-06-15T13:19:43Z

...pring/spring-integration-4.1/javaagent/src/test/groovy/SpringIntegrationAndRabbitTest.groovy

+        // spring-cloud-stream-binder-rabbit listener puts all messages into a BlockingQueue immediately after receiving
+        // that's why the rabbitmq CONSUMER span will never have any child span (and propagate context, actually)
+        // and that's why spring-integration creates another CONSUMER span
+        span(4) {
+          name ~/testTopic.anonymous.[-\w]+ process/
+          childOf span(3)
+          kind CONSUMER
+        }
+        span(5) {
+          name "testConsumer.input"
+          childOf span(3)
+          kind CONSUMER
+        }


Two CONSUMER spans here are a necessity: turns out spring-cloud-stream for rabbitmq uses a listener that puts all incoming messages into a BlockingQueue immediately after receiving them; there's no way to propagate context through that... (BTW, this explains why @RabbitListener did not work for some people)

So when spring-integration instrumentation kicks in, it sees no CONSUMER span, so it creates a new one - a child of the remote PRODUCER span.

mateuszrzeszutek · 2021-06-15T13:23:05Z

...tation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/Instrumenter.java

      case SERVER:
-        suppressed = ServerSpan.fromContextOrNull(parentContext) != null;
+      case CONSUMER:
+        suppressed = ServerSpan.exists(parentContext) || ConsumerSpan.exists(parentContext);


SERVER and CONSUMER spans are now both treated as "local-root" type of spans; by default, you can't have both of them in a single process, in a single trace, since they both represent the receiving side.
This is a pretty significant change, please tell me WDYT.

trask · 2021-06-16T05:22:36Z

...pring/spring-integration-4.1/javaagent/src/test/groovy/SpringIntegrationAndRabbitTest.groovy

+          }
+        }
+        span(5) {
+          name "testConsumer.input"


I think by message semantic conventions this should be testConsumer.input process

trask · 2021-06-16T06:16:48Z

...pring/spring-integration-4.1/javaagent/src/test/groovy/SpringIntegrationAndRabbitTest.groovy

+          }
+        }
+        span(3) {
+          name "testTopic -> testTopic send"


I think messaging semantic conventions suggest this should be just testTopic send

That span comes from rabbit instrumentation, do you mind if I create an issue for this?

It seems that we should review rabbitmq integration's usage of semantic convention anyway. There are some other strange things here

trask · 2021-06-16T06:18:04Z

...pring/spring-integration-4.1/javaagent/src/test/groovy/SpringIntegrationAndRabbitTest.groovy

+          attributes {}
+        }
+        span(2) {
+          name "exchange.declare"


can you comment in the test which spans are from spring integration instrumentation and which are from rabbitmq instrumentation?

Good point, will do.

mateuszrzeszutek requested review from anuraaga, iNikem, jkwatson, laurit, pavolloffay, trask and tylerbenson as code owners June 14, 2021 09:09

mateuszrzeszutek commented Jun 14, 2021

View reviewed changes

...pring/spring-integration-4.1/javaagent/src/test/groovy/SpringIntegrationAndRabbitTest.groovy Show resolved Hide resolved

mateuszrzeszutek commented Jun 14, 2021

View reviewed changes

.../spring/spring-integration-4.1/testing/src/main/groovy/AbstractComplexPropagationTest.groovy Show resolved Hide resolved

iNikem reviewed Jun 14, 2021

View reviewed changes

iNikem approved these changes Jun 14, 2021

View reviewed changes

trask reviewed Jun 15, 2021

View reviewed changes

...pring/spring-integration-4.1/javaagent/src/test/groovy/SpringIntegrationAndRabbitTest.groovy Show resolved Hide resolved

mateuszrzeszutek commented Jun 15, 2021

View reviewed changes

Mateusz Rzeszutek added 4 commits June 15, 2021 15:23

Spring Integration javaagent instrumentation

66f5835

codenarc

4e4de18

inline gradle property

5bfe159

Conditionally create CONSUMER spans instead of always INTERNAL

84bfb99

mateuszrzeszutek force-pushed the spring-messaging-javaagent branch from 9434914 to 84bfb99 Compare June 15, 2021 13:24

Mateusz Rzeszutek added 4 commits June 15, 2021 15:26

merge fix

f9b2577

Add attributes assertions to rabbitMq test

1e2803b

minor fix

b5fa99f

fix javadocs

5045e54

iNikem approved these changes Jun 15, 2021

View reviewed changes

trask approved these changes Jun 16, 2021

View reviewed changes

trask reviewed Jun 16, 2021

View reviewed changes

Code review follow-up

3bf3fbc

mateuszrzeszutek mentioned this pull request Jun 16, 2021

Review RabbitMQ instrumentation's semantic conventions usage #3337

Closed

iNikem merged commit 3e28b01 into open-telemetry:main Jun 16, 2021

mateuszrzeszutek deleted the spring-messaging-javaagent branch June 16, 2021 14:24

mateuszrzeszutek mentioned this pull request Jun 16, 2021

Trace ID/Context is not propagated into spring-cloud-stream-binder-rabbit backed by org.springframework.integration.channel.FluxMessageChannel #3096

Open

Conversation

mateuszrzeszutek commented Jun 14, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mateuszrzeszutek commented Jun 15, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trask Jun 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

trask Jun 16, 2021 •

edited

Loading