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

Propagate client span context in doOnRequest #6621

Merged
merged 4 commits into from
Sep 21, 2022

Conversation

jamesmartinpp
Copy link
Contributor

Copying the changes from Mike W. This fixes the problem where the jaxrs and http client request spans are siblings instead of parent/child.

@mateuszrzeszutek please review

@jamesmartinpp jamesmartinpp requested a review from a team September 14, 2022 16:05
Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

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

Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

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

jaxrs and http client request spans are siblings instead of parent/child

@jamesmartinpp which jaxrs library/version and which http client library/version is this? we may be able to update the related jaxrs client test to include the http client instrumentation to provide test coverage for this

@jamesmartinpp
Copy link
Contributor Author

jaxrs and http client request spans are siblings instead of parent/child

@jamesmartinpp which jaxrs library/version and which http client library/version is this? we may be able to update the related jaxrs client test to include the http client instrumentation to provide test coverage for this

The client is reactor-netty and the jaxrs framework is Resteasy.

@trask
Copy link
Member

trask commented Sep 14, 2022

@jamesmartinpp what versions of reactory-netty and resteasy?

@trask
Copy link
Member

trask commented Sep 14, 2022

This fixes the problem where the jaxrs and http client request spans are siblings instead of parent/child.

I thought I understood this, but I'm not able to repro it, is this a jaxrs CLIENT span or a jaxrs INTERNAL span? could you include a bit more detail how to reproduce the issue? thx!

@jamesmartinpp
Copy link
Contributor Author

@jamesmartinpp what versions of reactory-netty and resteasy?

reactor-netty v1.0.17
resteasy v3.5.0

@jamesmartinpp
Copy link
Contributor Author

This fixes the problem where the jaxrs and http client request spans are siblings instead of parent/child.

I thought I understood this, but I'm not able to repro it, is this a jaxrs CLIENT span or a jaxrs INTERNAL span? could you include a bit more detail how to reproduce the issue? thx!

I believe this is a jaxrs CLIENT span. I'm pretty sure the relationship to the jaxrs service span is correct. I will check MIke's notes and get back to you.

@jamesmartinpp
Copy link
Contributor Author

jamesmartinpp commented Sep 15, 2022

@trask

I believe this is a jaxrs CLIENT span. I'm pretty sure the relationship to the jaxrs service span is correct. I will check MIke's notes and get back to you.

Sorry, I was wrong. Prior to this change we are propagating the upstream service's jax-rs request spanid as the parent. So the downstream jaxrs request will use the upstream server's spanId as its parent. Making this change will propagate the http get spanId instead.

So before this change:

server1
    |
    --- client 1
    |
    --- server 2

After this change:
server1
    |
    --- client 1
            |
            --- server 2

@mateuszrzeszutek
Copy link
Member

Just to make things a bit more clear: you're using a custom propagation method and you need doOnRequest() to have the CLIENT span context available to be able to propagate all the necessary data, right?

@jamesmartinpp
Copy link
Contributor Author

Just to make things a bit more clear: you're using a custom propagation method and you need doOnRequest() to have the CLIENT span context available to be able to propagate all the necessary data, right?

Yes, that appears to be correct. Please let me know if this is a problem.

@trask
Copy link
Member

trask commented Sep 20, 2022

@jamesmartinpp makes sense, thx

jamesmartinpp and others added 3 commits September 20, 2022 12:30
…ent/src/main/java/io/opentelemetry/javaagent/instrumentation/reactornetty/v1_0/HttpClientInstrumentation.java


comment added to explain the change.

Co-authored-by: Trask Stalnaker <[email protected]>
@trask trask merged commit 2cfaf2d into open-telemetry:main Sep 21, 2022
LironKS pushed a commit to helios/opentelemetry-java-instrumentation that referenced this pull request Oct 23, 2022
Copying the changes from Mike W. This fixes the problem where the jaxrs
and http client request spans are siblings instead of parent/child.

@mateuszrzeszutek please review

Co-authored-by: Trask Stalnaker <[email protected]>
LironKS pushed a commit to helios/opentelemetry-java-instrumentation that referenced this pull request Oct 31, 2022
Copying the changes from Mike W. This fixes the problem where the jaxrs
and http client request spans are siblings instead of parent/child.

@mateuszrzeszutek please review

Co-authored-by: Trask Stalnaker <[email protected]>
LironKS pushed a commit to helios/opentelemetry-java-instrumentation that referenced this pull request Dec 4, 2022
Copying the changes from Mike W. This fixes the problem where the jaxrs
and http client request spans are siblings instead of parent/child.

@mateuszrzeszutek please review

Co-authored-by: Trask Stalnaker <[email protected]>
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.

3 participants