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 key shading: multiple instances #4666

Closed
lmolkova opened this issue Nov 18, 2021 · 4 comments
Closed

Context key shading: multiple instances #4666

lmolkova opened this issue Nov 18, 2021 · 4 comments
Labels
enhancement New feature or request

Comments

@lmolkova
Copy link
Contributor

lmolkova commented Nov 18, 2021

What I want to achieve:

Libraries that are not shaded into agent should be able to use instrumentation-api and other instrumentation libraries directly and it should work seamlessly with the agent.

Scenario 1:

  • library instrumentation does not ship with agent
  • library wants to pass spans in Reactor context using ContextPropagationOperator.store... so that nested instrumentations would pick up parent context.
  • I'm trying to make it work along with agent instrumentation (i.e. nested instrumentation correlates properly)

Scenario 2:
Same as 1, but I want to set SpanKey.ALL_CLIENTS to suppress nested CLIENTS

Update: SpanKeys just work because of context key bridging

What doesn't work: shaded context keys

Interaction between an external library and everything the agent is hard and fragile because of shaded context keys and context.

E.g. io.opentelemetry.instrumentation.reactor.ContextPropagationOperator from opentelemetry-reactor-3.1 is different from what agent uses io.opentelemetry.javaagent.shaded.instrumentation.reactor.ContextPropagationOperator, i.e. static context key is another instance

Here's an example for reactor that shows the problem (need to run with agent)

Describe the solution you'd like

It looks like context keys must not be shaded to avoid multiple instances of them. Maybe there is some transformation that can keep them or perhaps we can move them to classes that are never relocated. But it doesn't solve another issue: I can get a shaded or unshaded context anytime I read it, i.e. Context must not be shaded as well.

Describe alternatives you've considered

I can definitely use reflection against io.opentelemetry.javaagent.shaded.instrumentation.reactor.ContextPropagationOperator or shaded SpanKeysas mitigation. It doesn't look great 🙉

https://github.com/Azure/azure-sdk-for-java/blob/main/sdk/core/azure-core-tracing-opentelemetry/src/main/java/com/azure/core/tracing/opentelemetry/implementation/OpenTelemetrySpanSuppressionHelper.java

@lmolkova lmolkova added the enhancement New feature or request label Nov 18, 2021
@mateuszrzeszutek
Copy link
Member

It looks like context keys must not be shaded to avoid multiple instances of them.

I think it's the other way - they must be shaded and we have to implement a bridge instrumentation; the application uses io.opentelemetry.context.Context, while the agent uses io.opentelemetry.javaagent.shaded.io.opentelemetry.context.Context, we need to convert one to another to make it possible for library & javaagent instrumentation to communicate.

@lmolkova
Copy link
Contributor Author

lmolkova commented Nov 18, 2021

they must be shaded and we have to implement a bridge instrumentation;

I think we can't ask libraries to do context bridge and also be aware of a shaded key. It should be the agent responsibility to check for non-shaded context key and non-shaded context.
Would it be a reflection-based approach or there is some better way?

@mateuszrzeszutek
Copy link
Member

I think we can't ask libraries to do context bridge and also be aware of a shaded key. It should be the agent responsibility to check for non-shaded context key and non-shaded context.

Yes, most definitely! I wasn't precise enough in my previous comment - "a bridge instrumentation" referred to an additional javaagent instrumentation (like the opentelemtry-api one) that introduces the agent<->library translation.

@lmolkova
Copy link
Contributor Author

I'll play with reflection in ContextPropagationOperator and see how it goes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants