Fix interference between extension and Pyroscope SDK #13
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
korniltsev
approved these changes
Dec 31, 2024
| @Override | ||
| public void close() { | ||
| try { | ||
| Method closeMethod = ctx.getClass().getDeclaredMethod("close"); |
There was a problem hiding this comment.
nit: I'm a bit worried about performance of using reflection here. If this becomes a problem we can try generate a java class at runtime
There was a problem hiding this comment.
Agreed, I'll keep an eye. Also, we need to add some test automation to this repo, I'll create an issue for this.
aleks-p
force-pushed
the
fix/extension-sdk-interference
branch
from
e4622a1 to
827fbaf
Compare
This was referenced Jan 2, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Provides a solution for #1, #10 and #12. This is just a proposal, there are perhaps other alternatives that could work as well.
Background
When building the extension jar, we bundle a relocated Pyroscope SDK and async profiler binaries. We do this so that profiling itself and the tracing-profiling correlation can be achieved with no user-level code modifications. A user simply adds the open telemetry java agent and registers the pyroscope-otel jar as an extension:
Limitations
If an application already depends on the Pyroscope SDK (e.g., to add custom labels), and we want to correlate traces and profiles, we are faced with a problem. We effectively have 2 self-contained Pyroscope jars in the system, one from the SDK itself and one for the otel extension. Furthermore, the otel extension is loaded via an isolated class loader, which means we can't easily detect whether the Pyroscope SDK is present or not.
In some cases this will result in an exception like in #1 and #10. in other cases (#12, depending on used versions) we will end up with 2 profilers running and an undefined profiler output.
Proposed solution
We introduce a
ProfilerApiin the Pyroscope SDK, exposing basic profiling functionalities needed by this otel extension. When the extension runs, it will try to load an implementation of this API using the system class loader. If it succeeds, we use it for all future profiler interactions. If it fails for any reason, we do everything as we do today.Depends on grafana/pyroscope-java#176.
Caveats
The extension runs early in the boot process. For it to be able to "see" the
ProfilerApiimplementation, we need to run Pyroscope in agent mode and the agent needs to be loaded before the open telemetry one:Users that don't explicitly need the Pyroscope SDK in their applications can omit this and run the extension like they do today: