Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Always create a JMS consumer span #10604

Merged
merged 5 commits into from
Feb 22, 2024
Merged

Conversation

laurit
Copy link
Contributor

@laurit laurit commented Feb 19, 2024

Currently when using MessageConsumer#receive() with receive-telemetry disabled (default) we don't create any spans on the consumer side. This PR changes this behavior so that a receive span is created that is the child of the producer.

@laurit laurit requested a review from a team as a code owner February 19, 2024 16:21
@breedx-splk
Copy link
Contributor

I've seen users get tripped up by the lack of parenting for messaging/jms/kafka for sure....but do we have spec around this? Is this going to surprise some folks?

Copy link
Contributor

@breedx-splk breedx-splk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't see anything troubling in the implementation, but I'm not sure I understand how the test traces shake out. Hope to get some clarification.

equalTo(SemanticAttributes.MESSAGING_OPERATION, "receive"),
equalTo(SemanticAttributes.MESSAGING_MESSAGE_ID, messageId))),
trace ->
trace.hasSpansSatisfyingExactly(span -> span.hasName("consumer parent").hasNoParent()));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something is weird about this to me. We create two traces, just like we did before, but the innermost span of the second consumer trace is now part of the first trace? It seems weird now that we have two consumer spans, one rooted in a publisher parent context and another one that seems lonely ("consumer parent"). It's also no longer true that "consumer parent" is the parent of the consumer.

What am I missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for looking into this or messaging instrumentation is a bit of a mess so and not the easiest to understand. We'd like all of our messaging instrumentations to follow the same rules, but unfortunately this is not the case. Currently I think kafka and sqs instrumentations are probably in the best shape.
The telemetry that the messaging instrumentations create depends on the otel.instrumentation.messaging.experimental.receive-telemetry.enabled flag, which by default is false. This is tested in the Jms3SuppressReceiveSpansTest, true is tested in Jms3InstrumentationTest. When this flag is true we should create telemetry where producer and consumer are in different traces that are connected via a span link. Having consumer span for receive operation being parent of process span is for example described in https://github.com/open-telemetry/semantic-conventions/blob/v1.21.0/docs/messaging/messaging-spans.md#batch-receiving Spec has been using span links for a while and at one point we even turned this on by default, but had to revert because at that time most backends did not support the span links. Our default behavior is to place producer and consume in the same trace so that producer is the parent of the consumer and there are no span links. When using this strategy we usually don't create consumer spans for receive operation and only have process spans (this motivated the receive-telemetry in the flag name). If there is no process span then there will be no consumer spans at all, which is what this PR tries to address (or initially tried to address, had to make more changes to get spring jms tests passing too).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for clarifying, appreciate it.

Copy link
Contributor

@breedx-splk breedx-splk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like a step forward to me.

@laurit laurit merged commit b5bbc62 into open-telemetry:main Feb 22, 2024
47 checks passed
@laurit laurit deleted the jms-receive branch February 22, 2024 07:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants