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

Cross test javaagent instrumentation against other instrumentations targeted at the same library? #5261

Open
trask opened this issue Jan 28, 2022 · 6 comments

Comments

@trask
Copy link
Member

trask commented Jan 28, 2022

E.g. netty-3.8 instrumentation is tested with netty-4.0 and netty-4.1 instrumentations applied.

Should we apply this pattern to all instrumentations with multiple versions, or decide that the muzzle range checks are all we need and abandon this practice?

@iNikem
Copy link
Contributor

iNikem commented Jan 28, 2022

I already expressed my opinion, that we should test with the whole agent, not partial agents prepared specifically for a given module :)

@mateuszrzeszutek
Copy link
Member

I already expressed my opinion, that we should test with the whole agent, not partial agents prepared specifically for a given module :)

That indeed sounds like a good idea; especially in light of our recent Armeria vs. Netty discussion 😄
And if we ever need to test a particular instrumentation in isolation, we can create a test suite/set that disables unwanted instrumentations.

@trask
Copy link
Member Author

trask commented Jan 31, 2022

we should test with the whole agent, not partial agents prepared specifically for a given module

this sounds good to me, any thoughts how to implement?

@iNikem
Copy link
Contributor

iNikem commented Feb 1, 2022

we should test with the whole agent, not partial agents prepared specifically for a given module

this sounds good to me, any thoughts how to implement?

io.opentelemetry.instrumentation.javaagent-testing.gradle.kts uses a minimal agent-for-testing and adds module-specific extension. It may depend on the full javaagent instead.

@iNikem
Copy link
Contributor

iNikem commented Feb 1, 2022

@anuraaga was against this idea in the past because allegedly building the full agent will take too much time.

@trask
Copy link
Member Author

trask commented Feb 1, 2022

sounds worth giving it a try and seeing what the perf impact is

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants