-
Notifications
You must be signed in to change notification settings - Fork 873
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
Make OpenTelemetry-API Bridge and dependent instrumentations work with indy #11578
Merged
Merged
Changes from 8 commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
82c1a54
Add mechanism to hide packages from InstrumentationModuleClassLoader
JonasKunz 93dab94
Migrate OpenTelemetry API bridge to indy
JonasKunz dbe6eef
Add dependent instrumentations to same module group
JonasKunz 8e8fecc
spotless
JonasKunz 4af609d
Enable @Advice.AssignReturned for non-indy Advices
JonasKunz 0194c2d
spotless
JonasKunz 4e6d8d9
Merge remote-tracking branch 'otel/HEAD' into api-bridge-indy
JonasKunz f0cdc45
Added forgotten incubator instrumentation
JonasKunz 45ccc64
Merge remote-tracking branch 'otel/HEAD' into api-bridge-indy
JonasKunz cbbb215
Merge remote-tracking branch 'otel/main' into api-bridge-indy
JonasKunz 77e4299
fix javadoc
JonasKunz File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
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
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
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
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
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
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
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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't follow why this is only a problem for this particular instrumentation? thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like this instrumentation is the only one using this mechanism: All other api-bridge instrumentations seem to only provide the helpers, the
opentelemetry-api-1.0
instrumentation is the one glueing them to the application code.To give a bit more detail about the problem, here is an example lookup of such a helper class:
opentelemetry-java-instrumentation/instrumentation/opentelemetry-api/opentelemetry-api-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/opentelemetryapi/ApplicationOpenTelemetry.java
Line 61 in a453bc6
The
1.0
instrumentation looks for aOpenTelemetry
implementation class (=the helper) provided by the1.27
instrumentation to use that if available.So with non-indy, this helper class will only be available if the
1.27
instrumentation was actually applied, because only then this helper is injected. E.g. if the application ships the API in version1.26
, this lookup would fail.This check doesn't quite work that way for invokedynamic instrumentations. In that case, we have three classloaders:
InstrumentationModuleClassloader
which will have the application classloader and agent classloader both as parentsWhen now an instrumentation is applied, its advice code and helpers are loaded into the
InstrumentationModuleClassloader
. Themodule-grouping
feature ensures that all opentelemetry-api instrumentations use the sameInstrumentationModuleClassloader
instance.Going back to our example where the application ships the OpenTelemetry API in
1.26
, the following would happen:1.0
instrumentation tries to lookup the helper provided by the1.27
instrumentationInstrumentationModuleClassloader
, because the instrumentation wasn't appliedInstrumentationModuleClassloader
will now look in its parents for said helper. The helper will now be loaded by the agent classloader (because it contains the.class
files of the helper and therefore can load them)The mechanism of package hiding introduced in this PR prevents the last step from happening: We disallow the helper to be looked up from the agent classloader and therefore the class loading fails as it should.
Note that once we commit to invokedynamic and remove non-invokedynamic instrumentations, we can simplify this.
A lot of this "trickery" is only required because right now when we inject helpers into application classloaders, those classes are also loaded immedatiely. With invokedynamic and the
InstrumentationModuleClassloader
this follows the classic java model of the classloading knowing how to load its classes, but only actually loading them on demand.For the OpenTelemetry API bridge this means we can then just have the bridge for all versions included and simply at runtime (instead of instrumentation time) select the proper bridge implementation. We have actually done exactly that in the elastic-apm-agent for our OpenTelemetry Metrics API Bridge.
This means that the bridge code is actually completely independent of the agent/instrumentation and can be in my opinion be more easily maintained. I'm planning/proposing to take the same approach for the Otel-agent once we've dropped support for non-invokedynamic instrumentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI another neat thing about that bridge structure is that it allows to add a test for detecting unimplemented default methods:
https://github.com/elastic/apm-agent-java/blob/main/apm-agent-plugins/apm-opentelemetry/apm-opentelemetry-metrics-bridge-parent/apm-opentelemetry-metrics-bridge-latest/src/test/java/co/elastic/apm/agent/opentelemetry/metrics/bridge/BridgeExhaustivenessTest.java#L43
So that test would fail if we upgrade the SDK/API and the bridge is missing a newly added method. I don't know if something like that is already in place for the current bridge implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, that helps me a lot, a couple of questions:
would we ever want any helper classes to be loaded by agent classloader? (just wondering if we can do this universally somehow instead of per instrumentation module)
does that mean we can remove
agentPackagesToHide()
at that point?yes, this is handled by muzzle, check out #1193
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this should be the preferred mechanism for communicating across instrumentation modules.
The "module-group" feature where we place multiple instrumentations in the same classloader was mainly introduced to have an easy way of getting the existing instrumentations to work. They currently communicate by explicitly using classes which would have been injected into the app CL before.
Ideally instrumentation which offer a service to other instrumentations should rather do this by publishing an API to the agent-CL which will then be accessible to all other instrumentations, independent of their classloader.
E.g. if an instrumentation is responsible for wrapping some application types in order to preserve context, they could offer just an API for unwrapping instead of having all their implementation details and dependencies exposed to other instrumentation modules by sharing the classloader. This should make instrumentations and their interactions easier to understand, because then there are clear borders between them.
At the moment, classes can be explicitly loaded into the agent CL by returnign false for those via the
InstrumentationModule.isHelperClass
, which might look a bit confusing due to the old naming.That said, what we could do is ensure that no class for which
isHelperClass
is true is ever loaded from the agent-CL for a given module-group. Do you want me to try that maybe?agentPackagesToHide
will actually be required when we remove shading: As soon as we do that, there will be two opentelemetry-APIs with the same name visible to the bridgeInstrumentationModuleClassloader
: The opentelemetry-api from the agent and from the app.Normally, classes from the agent take precedence (so that application dependencies don't accidentally replace stuff we ship in the agent). In this case, we have the edge case that we actually do want to link against the application opentelemetry-api instead of the one from the agent. Therefore we need to hide the one from the agent and call it via some proxy-api (classes injected into the agent-cl delegating to the agent opentelemetry-API) instead.
IINM that one only covers abstract methods, inherited default methods are not considered as abstract. But let's not go down that rabbit hole, that is a topic for the future
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI we used to do this, but now we only use this for the boot loader. See https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation/internal/internal-class-loader/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/classloader/LoadInjectedClassInstrumentation.java
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't aware of that, thanks for pointing this out!