Implement invokedynamic advice bootstrapping

[JonasKunz]

This PR finally makes InstrumentationModules with isIndyModule()=true functional. Part of #8999.

This PR does not include a safety mechanism for preventing very old bytecode from being instrumented.
In the elastic APM-agent we by default only instrument classes with bytecode level 1.4 and newer, which should cover the 99% case. We do however allow an opt-in for instrumenting very old bytecode via a config option.

We should discuss whether we want to take this approach for the Otel-agent as well, but this should be done in a different PR imo.

[laurit]

have you tried to find a way to make @Advice.Return(readOnly = false) work? I assume similarly @Advice.Argument(value = 0, readOnly = false) wouldn't work?

[JonasKunz]

Unfortunately I think @Advice.Return(readOnly = false) will never work without inlining due to the fact that the java language does not support output parameters.

The Advice is invoked via a standard method call. Therefor it does not have access to any local variables of the instrumented method, including arguments and return values. The "read" access is actually provided by copying those variables as call arguments when invoking the advice.

The only way I could think of making this work would be to wrap the arguments / return value with a modifiable object, e.g. @Advice.Return Assignable<String> ret and ret.setValue("foobar"). I feel like this solution would be overly complex, less efficient and not really more readable than @Advice.AssignReturned.

[laurit]

One could attempt transforming advice classes, add @Advice.AssignReturned.ToReturned, change parameter annotated with @Advice.Return to local variable and return its value from the advice. It definitely isn't easy, but I wouldn't go as far as calling it impossible.
The problem here is that people already have extensions that use the inlined advice, we need to figure out what to do with these (will we let them fail at runtime? will we prevent using old extensions? will we provide a migration guide? will we continue supporting both inlined and non-inlined advice?). There also seems to be much more content about using inlined advice (the default option) that wouldn't work with non-inlined advice. It is much easier to justify changes that are purely improvements without negative sides.

[JonasKunz]

One could attempt transforming advice classes, add @Advice.AssignReturned.ToReturned, change parameter annotated with @Advice.Return to local variable and return its value from the advice. It definitely isn't easy, but I wouldn't go as far as calling it impossible.

That's true, I wasn't thinking of runtime transforming advice code. However I'm a bit concerned that with such an approach we would reduce one of the main benefits of the invokedynamic approach: When debugging you would again debug different code than what is being executed, similar though not as severly changed as via inlining.

The problem here is that people already have extensions that use the inlined advice, we need to figure out what to do with these (will we let them fail at runtime? will we prevent using old extensions? will we provide a migration guide? will we continue supporting both inlined and non-inlined advice?)

This was one of our concerns as well when proposing the invokedynamic approach. If I recall correctly, we had a discussion with mainly @trask and @mateuszrzeszutek in the SIG meeting. I think the outcome was the following:

• It is okay to deprecate the inling-InstrumentationModules because they are not considered stable yet and not that widely used
• Eventually we want to get rid of inlining so that we can get rid of shading too and don't have to maintain two approaches
• The automatic migration (which is basically what you are suggesting I think) from inlined to invokedynamic InstrumentationModules was briefly discussed, but because the InstrumentationModule extension mechanism is not stable yet and apparently not that widely used it would be okay to just provide a migration guide instead

If I misremember or you don't agree here we can revive that discussion and rethink what the best approach would be.

There also seems to be much more content about using inlined advice (the default option) that wouldn't work with non-inlined advice

You are referring to content in terms of documentation, blogposts, etc, correct?
That's definitely true. I think a large part of the migration guide should be explaining on how to translate inlined advice techniques to invokedynamic ones and give reasoning. We should be able to recycle a good chunk of content from this blogpost for that purpose.

[laurit]

I was just curious whether you had attempted to make old advice code run, if nothing else it would show which of the instrumentations would be difficult to convert.

Eventually we want to get rid of inlining so that we can get rid of shading too and don't have to maintain two approaches

While getting rid of shading sounds awesome I think https://github.com/open-telemetry/opentelemetry-java-instrumentation/tree/main/instrumentation/opentelemetry-api/opentelemetry-api-1.0/javaagent would be difficult to implement without shading. I guess another option could be deleting that and similar instrumentations.

You are referring to content in terms of documentation, blogposts, etc, correct?
That's definitely true. I think a large part of the migration guide should be explaining on how to translate inlined advice techniques to invokedynamic ones and give reasoning. We should be able to recycle a good chunk of content from this blogpost for that purpose.

Good to hear that you have already considered this.

[JonasKunz]

While getting rid of shading sounds awesome I think https://github.com/open-telemetry/opentelemetry-java-instrumentation/tree/main/instrumentation/opentelemetry-api/opentelemetry-api-1.0/javaagent would be difficult to implement without shading.

The Otel-API bridge is truly the most difficult thing to realize without shading, but it is doable in a relatively clean way. I'll outline the idea below.

I guess another option could be deleting that and similar instrumentations.

I'm not sure what you mean here? Are you thinking of having the non-shaded SDK + API in the bootstrap instead to "shadow" Ote-API copies shipped with application classloaders? I think that easily could lead to version mismatches, which I think is one of the main motivations for having the Otel-API bridging instrumentation.

Anyway, adding an Otel-API instrumentation is possible with invokedynamic-advices and without publishing the agent's Otel API + SDK to the bootstrap. We actually implemented exactly that for Otel metrics support in the elastic APM Agent.

In the elastic APM Agent we have the following setup:

• An (unshaded) Otel-API and Otel-Metrics-SDK are loaded by the Agent-classloader
• An instrumentation instruments user application GlobalOpenTelemetrys and bridges them to the SDK which lives inside the agent classloader

The main problem is that the InstrumentationModuleClassloader now sees two Otel-APIs: The one from the user application being instrumented and the one in the agent-classloader.

One option to resolve this conflict is to shade the Otel-API which is shipped with the agent. But we don't want shading for known reasons. The alternative we implement in the elastic APM Agent is to create a Proxy-API which does nothing but delegating to the Otel-API. This Proxy-API is loaded by the agent-classloader.

Now the Otel-API bridge instrumentation can simply access the Proxy-API when it wants to actually access the API/SDK shipped with the agent. The way I implemented the proxy API here has the downside of increasing allocations due to wrapping everything. In hindsight I'd rather use static adapter methods operating on Objects to avoid that wrapping.

[laurit]

Proxy api is a nice idea. Another instrumentation that could prove challenging is

opentelemetry-java-instrumentation/instrumentation/aws-sdk/aws-sdk-2.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/awssdk/v2_2/AwsSdkInstrumentationModule.java

Line 26 in f89fa74

helperResourceBuilder.register("software/amazon/awssdk/global/handlers/execution.interceptors");

that injects a resource that contains a class name that application code will try to loader. That class should be injected into application class loader. That instrumentation also uses multiple instrumentation modules in a weird way to handle optional dependencies.

[JonasKunz]

I've already spent some braintime looking at all the instrumentations using the resource-injection mechanism of InstrumentationModules. I did this because imo if we want to get rid of shading we'll also need to find ways of implementing those instrumentation modules without the resource injection. My chain of thought here is as follows:

• Resource injection is currently used to inject references to or the bytecode sources of classes which are also injected into the target classloader
• To do their thing those injected classes require support from the agent-sdk, which therefor needs to be in the bootstrap
• if the agent SDK is in the bootstrap, parts of it must be shaded

Therefor I came to the conclusion that if we want to get rid of shading, we'll need to find ways for those InstrumentationModules to work correctly without injecting resources.

The use-cases I saw for resource injection are the following:

• Injected the .class files of injected classes because the user application does bytecode inspection
• Injection of standard SPI services
• Injection of services with non-standard serviceloading mechanism

The injection of .class-resources shouldn't be required anymore: The InstrumentationModuleClassloader already takes care of providing those resources correctly.

The injection of services loaded via the ServiceLoader is currently used by quite a few modules. Therefor I would think that it would make sense to lift this into a capability of InstrumentationModules in general:

public class MyInstrumentationModule extends InstrumentationModule {

@Override
public void registerServices(ServiceRegistry registry) {
   registry.register("the.service.Interface", "my.instrumentation.ServiceImpl");
}

}

The agent could provide this functionality by itself implementing an Instrumentation of the ServiceLoader.
This instrumentation would detect when a lookup for a service interface is made for which an InstrumentationModule registered a service (e.g. the.service.Interface in this case). It would then create a my.instrumentation.ServiceImpl instance (loaded by the InstrumentationModuleClassloader) and return that in addition.

Your linked AWS instrumentation falls under the last category:

• Injection of services with non-standard serviceloading mechanism

Fortunately for this specific instrumentation this should be easily solveable. We should be able to instrument ClasspathInterceptorChainFactory to achive the same behaviour as if we would inject the interceptor via a resource (except that the interceptor is now nicely loaded via the InstrumentationModuleClassloader)

I'm more concerned about instrumentations injecting SpringBoot autoconfigurations:

opentelemetry-java-instrumentation/instrumentation/spring/spring-boot-actuator-autoconfigure-2.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/spring/actuator/v2_0/SpringBootActuatorInstrumentationModule.java

Line 37 in f89fa74

public void registerHelperResources(HelperResourceBuilder helperResourceBuilder) {

But I'm confident that we'll find a clean solution there aswell.

[laurit]

alternatively we could introduce a concept of proxy classes that are defined in the application class loader and just delegate to the same class in the instrumentation class loader.

[JonasKunz]

Yes, that would also resolve the problems with the injected SpringBoot autoconfigurations I think.

[SylvainJuge]

[minor] It could probably added later if (and only if) that proves to be an issue in practice. Having to require explicit opt-in for older bytecode means that instrumentation modules for old libraries would be disabled by default. One good example is the Apache httpclient3 library which is compiled with 1.1 bytecode.

The only issue I see with bytecode version upgrade is when the agent is unable to resolve the types that are in the stack map frames, for example when the classloader is not "cooperative enough" and prevents us from loading the class bytecode from resources.

[mateuszrzeszutek]

Nit: perhaps we could think of ignoring CanIgnoreReturnValueSuggester for classes that end with ...Advice; we're already doing a similar thing for the PrivateConstructorForUtilityClass check

See

opentelemetry-java-instrumentation/conventions/src/main/kotlin/otel.errorprone-conventions.gradle.kts

Lines 128 to 131 in f89fa74

	if (project.path.endsWith(":testing") || name.contains("Test")) {
	// This check causes too many failures, ignore the ones in tests
	disable("CanIgnoreReturnValueSuggester")
	}

and https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/custom-checks/src/main/java/io/opentelemetry/javaagent/customchecks/OtelPrivateConstructorForUtilityClass.java

[JonasKunz]

I've implemented an Otel variant of the check now. It is slightly more compex than the OtelPrivateConstructorForUtilityClass because it is a method-level check. Therefore I need to find the declaring class manually in the check.

If you know any better solution here let me know.

[trask]

🎉