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

Agent context storage wrapper should not override other wrappers #7355

Merged
merged 2 commits into from
Dec 12, 2022

Conversation

laurit
Copy link
Contributor

@laurit laurit commented Dec 2, 2022

Resolves #7344

@laurit laurit requested a review from a team December 2, 2022 12:06
@oleborne
Copy link

oleborne commented Dec 2, 2022

Thanks @laurit for the fix. I was about to make the same proposal.

However this leaves indeed the possibility for the wrapper to break the agent by actually replacing the underlying ContextStorage. Such a wrapper would break the javaagent.

If the code owners are ok with this possible issue, I would recommend to also update the Javadoc of ContextStorageWrappersInstrumentation to explicitly state that Wrappers are still allowed: only providers are bypassed.

@laurit
Copy link
Contributor Author

laurit commented Dec 2, 2022

However this leaves indeed the possibility for the wrapper to break the agent by actually replacing the underlying ContextStorage. Such a wrapper would break the javaagent.

In my opinion this is not a big issue. If user has built such a wrapper then user can also modify it if there is desire to run with javaagent. If such a wrapper is used by a framework then agent can try to instrument it.

@oleborne
Copy link

oleborne commented Dec 5, 2022

If user has built such a wrapper then user can also modify it if there is desire to run with javaagent.

Totally agree, hence my remark about updating the Javadoc to 1. warn people about the possibility and 2. clarify how to fix it should you be the author of such a wrapper.

@trask trask added this to the v1.21.0 milestone Dec 11, 2022
…ent/src/main/java/io/opentelemetry/javaagent/instrumentation/opentelemetryapi/ContextStorageWrappersInstrumentation.java

Co-authored-by: Trask Stalnaker <[email protected]>
@trask trask merged commit d6ff481 into open-telemetry:main Dec 12, 2022
@laurit laurit deleted the agent-wrapper-order branch July 6, 2023 17:37
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.

ContextStorage.addWrapper and javaagent incompatibility
4 participants