Make OpenTelemetry-API Bridge and dependent instrumentations work with indy

[JonasKunz]

Ensures that the OpenTelemetry API bridge works with -PtestIndy=true. Part of #11457.
Effectively, this results in the bridge classes being loaded by the InstrumentationModuleClassloader instead of being injected into the application classloader. As a result, other instrumentations directly depending on those bridge classes (e.g. reactor) will need to be loaded by the same classloader.

This PR also adds a mechanism to hide packages present in the agent classloader for certain instrumentations.
We'll eventually need to use this when we get rid of shading: Then the non-shaded agent opentelemetry-API needs to be hidden from the API-bridge instrumentation in order to link against the application Otel-API.

@open-telemetry/java-instrumentation-approvers : Could someone please add the testIndy label to this PR?

[JonasKunz]

Currently waiting on #11582 to fix the independent test failures.

[SylvainJuge]

[minor] we need to cover this in https://github.com/open-telemetry/opentelemetry-java-instrumentation/pull/11546/files do you mind opening a PR on my PR to cover this as well ?

[JonasKunz]

I'd wait on approvals on this PR before doing the documentation (e.g. the method name might be changed). But I will update the docs afterwards

[trask]

not sure if it's worth it, but this reminds me of similar method we have which uses a fancy optimized Trie structure:

opentelemetry-java-instrumentation/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/ignore/IgnoredTypesBuilder.java

Line 29 in 9dd11ff

IgnoredTypesBuilder ignoreClass(String classNameOrPrefix);

[trask]

I didn't follow why this is only a problem for this particular instrumentation? thanks

[JonasKunz]

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

"io.opentelemetry.javaagent.instrumentation.opentelemetryapi.v1_27.ApplicationOpenTelemetry127");

The 1.0 instrumentation looks for a OpenTelemetry implementation class (=the helper) provided by the 1.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 version 1.26, this lookup would fail.

This check doesn't quite work that way for invokedynamic instrumentations. In that case, we have three classloaders:

• The application classloader containing the application OpenTelemetry API
• The agent classloader (containing all the .class resources of the instrumentations)
• The InstrumentationModuleClassloader which will have the application classloader and agent classloader both as parents

When now an instrumentation is applied, its advice code and helpers are loaded into the InstrumentationModuleClassloader. The module-grouping feature ensures that all opentelemetry-api instrumentations use the same InstrumentationModuleClassloader instance.

Going back to our example where the application ships the OpenTelemetry API in 1.26, the following would happen:

• The 1.0 instrumentation tries to lookup the helper provided by the 1.27 instrumentation
• This helper is not injected into the InstrumentationModuleClassloader, because the instrumentation wasn't applied
• However, the InstrumentationModuleClassloader 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.

[JonasKunz]

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.

[trask]

thanks, that helps me a lot, a couple of questions:

The helper will now be loaded by the agent classloader (because it contains the .class files of the helper and therefore can load them)

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)

Note that once we commit to invokedynamic and remove non-invokedynamic instrumentations, we can simplify this.

does that mean we can remove agentPackagesToHide() at that point?

I don't know if something like that is already in place for the current bridge implementation.

yes, this is handled by muzzle, check out #1193

[JonasKunz]

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)

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?

does that mean we can remove agentPackagesToHide() at that point?

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 bridge InstrumentationModuleClassloader: 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.

yes, this is handled by muzzle, check out #1193

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

[laurit]

A lot of this "trickery" is only required because right now when we inject helpers into application classloaders, those classes are also loaded immedatiely.

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

[JonasKunz]

I wasn't aware of that, thanks for pointing this out!

[laurit]

I think it is fine for now but we'll probably need to come back to this later and see whether we can improve the api.