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

CONTEXT_*_SPAN_KEY need to be moved to bootstrap and accessed through bridging? #1726

Closed
anuraaga opened this issue Nov 21, 2020 · 3 comments · Fixed by #3911
Closed

CONTEXT_*_SPAN_KEY need to be moved to bootstrap and accessed through bridging? #1726

anuraaga opened this issue Nov 21, 2020 · 3 comments · Fixed by #3911
Labels
bug Something isn't working

Comments

@anuraaga
Copy link
Contributor

anuraaga commented Nov 21, 2020

If I'm not mistaken, these keys will not function correctly when using both the agent and library instrumentation together since the key will be shaded in one and not compatible. I think these need to be moved to a location in the bootstrap classloader, with methods to get / set these. The opentelemetry-api instrumentation will need to also instrument these methods to perform bridging for these operations in the app classloader since they accept a Span.

@anuraaga anuraaga added the bug Something isn't working label Nov 21, 2020
@anuraaga anuraaga changed the title CONTEXT_*_SPAN_KEY need to be moved to an unshaded location? CONTEXT_*_SPAN_KEY need to be moved to bootstrap and accessed through bridging? Nov 21, 2020
@trask
Copy link
Member

trask commented Nov 22, 2020

I think these need to be moved to a location in the bootstrap classloader, with methods to get / set these.

these are in instrumentation-api which is (shaded) in the bootstrap classloader already

The opentelemetry-api instrumentation will need to also instrument these methods to perform bridging for these operations in the app classloader since they accept a Span.

👍

@trask
Copy link
Member

trask commented Nov 23, 2020

I think this is needed for a common use case for end users too: adding attributes to the SERVER span, but finding yourself under a spring controller INTERNAL span so you can't use Span.current().

Maybe ServerSpan.current() or something similarly friendly?

@mateuszrzeszutek
Copy link
Member

As an additional improvement, we could also set the library Config so that it contains exactly the same configuration as the javaagent: config file & SPI included.

mateuszrzeszutek pushed a commit to mateuszrzeszutek/opentelemetry-java-instrumentation that referenced this issue Mar 2, 2021
mateuszrzeszutek pushed a commit that referenced this issue Mar 3, 2021
* Move SERVER and CLIENT span context keys out of BaseTracer

Inspired by #1726 (comment)

* Rename context keys

Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>

Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants