Next steps to making the invokedynamic instrumentation approach the default

[JonasKunz]

We want to make the invokedynamic-based instrumentation approach the default one eventually.
This issue is intended to document the next steps towards this goal.

Per discussion in the SIG-Meeting, we'll do the following things first before switching the default of otel.javaagent.experimental.indy from false to true:

• Rewrite advice source code of included instrumentations to not require runtime-transformation (see below section for details)
• Ensure VirtualFields work efficiently in indy-mode
  • Enhance muzzle to pick up VirtualField.find calls from static initializers of helpers and advice classes
  • Replace all direct usages of VirtualField.find in advice classes with static fields in helpers

After that is done, we should be able to default otel.javaagent.experimental.indy to true. Based on our confidence in the feature, we'll have to decide whether to do this in a minor or major release.
Inline advice will still be technically supported by overriding that config option back to false. This leaves us an escape hatch in case we find bugs without impacting users much (they can immediately switch back to false to avoid the bugs).
Then, in a later major version we'll remove support for inlined-advice and then can start gaining the benefits, such as removing shading.

In addition, we should clean up the APIs added to ExperimentalInstrumentationModule (see the API feature to (not) promote sections below). This is however not considered a blocker for defaulting otel.javaagent.experimental.indy to true.

Advice source code migration

For instrumentations to work with indy, the advice code needs to be written slightly differently, as described in the guide. This is currently done automatically for most advices at runtime via a runtime transformation of the advice bytecode. This of course is not ideal from a maintenance perspective, as the source-code doesn't match what is actually executed.

I'd suggest to do the following:

• Add a new marker method boolean requiresAdviceRewritingForIndy() to ExperimentalInstrumentationModule and only apply the runtime-transformation accordingly
• Rewrite all the instrumentation advice code to no require the runtime transformation anymore (aka apply the guide)
  • Only exception: Keep inlined = true (which is the default): All other changes are backwards compatible and allow us to still run with otel.javaagent.experimental.indy=false
  • Once we remove support for inlined advice, we can add inlined = false, which is an easy, IDE-supported change (e.g. IntelliJ structural replace feature)
  • I'll try to provide an automated tool to do the more complex sourcecode-transformation (e.g. instering @Advice.AssignReturned) based on javaparser a tool for AST-based, structural code modification with a lexically preserving printer (e.g. original comments and whitespace are preserved)
• We can then disable the AdviceTransformer for instrumentations shipped with the agent. For external instrumentations, we can keep it around for some grace period and print a warning (e.g. Please migrate your advices, here is a link to our guide).

API features to promote

With the completion #11457, we'll have all our instrumentation capable of working with invokedynamic. We need to make sure to also have a clean plugin interface with indy-support when making it the default for external instrumentations. Currently, those features live in the ExperimentalInstrumentationModule class.
We'll need to clean up and migrate the relevant features to InstrumentationModule:
In order to make the

• Promote injectClasses(ClassInjector)
• Replace injectedClassNames with a method on ClassInjector instead
• Promote the module-opening mechanism added via make rmi instrumentation indy-compatible + add module opener #12585

The following two features I'm not sure about whether we want to actually have them in the public API or whether we'll keep them for internal modules only:

API features not to promote

getModuleGroup

getModuleGroup(): This feature was added to ease the conversion of some instrumentations which are tightly coupled by accessing classes from each other directly. A cleaner approach for this requirement is to have:

• A shared API-project for those instrumentations which is loaded by the agent-classloader (=non-helper classes)
• Have the individual instrumentations communicate through that API-layer instead

This would involve a very big refactor for our existing instrumentations, but I don't feel like this is the case for external instrumentations. Therefore I think it would be better in terms of "best-practices" to not have this method part of the public API to promote the correct way of doing things. Maybe we could rewrite one of the internal modules (after inlining is not supported anymore) to showcase how to do this.

agentPackagesToHide

agentPackagesToHide(): This method is used when trying to instrument classes which are also present in the agent-classloader: The problem is that for linking, classes found in the agent classloader take precedence over classes from the instrumented classloader. This means that if the classes are present in both, the instrumentation won't be able to actually use/instrument them. I feel like this is an esoteric edge which should not be exposed to the public api in order to keep it simpler, as it should be very rare to stumble across.

An example where this is needed internally is the instrumentation of the opentelemetry-api: The instrumentation needs to bridge application opentelemetry-APIs, while at the same timing bringing it's own opentelemetry-API version (eventually unshaded).

[laurit]

To get there, a first step will be to change the default of otel.javaagent.experimental.indy from false to true. This will allow us to evaluate the approach in a safe (opt-out) manner in the field and fixing bugs before fully removing inlined-advice to gain the full benefits, including the removal of shading from the agent.

Do I understand correctly that you wish to do this before the next major release? If so, you may need to raise this at the SIG meeting for discussion. I'm afraid that introducing this change in the stable version, even if it can be opted out, to field test and fix bugs does not align with our stability goals.

API features to promote

Some of the new experimental APIs are rarely needed so even if we don't manage to make them stable it should not be a problem.

getModuleGroup

I don't like this API either.

Advice source code migration
I'll try to provide an automated tool to do the more complex sourcecode-transformation (e.g. instering @Advice.AssignReturned) based on javaparser a tool for AST-based, structural code modification with a lexically preserving printer (e.g. original comments and whitespace are preserved)

The problem is that current inline advice code is generally more readable than than the non-inline advice. It is possible that advice code may need to be changed during conversion by introducing helper classes etc. to keep the same level of readability. I'm not sure whether automated conversion can help with this.
I would suggest that you start by introducing a way to tag instrumentations that need conversion (or don't need it) so you could disable automated rewriting. This should give a better picture of how many instrumentations still need to be converted. Additionally this would avoid the situation where new changes to instrumentations introduce changes that don't work without the automated rewriting.
Another issue that needs to be tackled is the removal of VirtualField usage from the advice. We may need to introduce logic that checks for the VirtualField in advice and emits a warning similarly to what we currently have for rewriting the VirtualField usages.

[JonasKunz]

@laurit I've changed the issue description based on your remarks and the discussion in yesterdays SIG meeting. Let me know if anything is missing or you disagree with any points.

The problem is that current inline advice code is generally more readable than than the non-inline advice. It is possible that advice code may need to be changed during conversion by introducing helper classes etc. to keep the same level of readability. I'm not sure whether automated conversion can help with this.

The main concern here is when multiple @Advice.Locals are involved, correct?. For that I'd like to introduce the following "pattern":

• add a static class to the Advice as container for locals:

   private static class Locals {
      Scope spanCope;
      Foo bar;
  }

• In the enter-method, start with Locals locals = new Locals()
• Replace the assignments to the @Advice.Local parameters with assignments to local.*
• Ensure that locals is returned on every exit path
  Imo, this isn't a lot worse than how locals are dealt with previously. Of course, if multiple @AssignReturned are involved, we'll have to pack things up in Object[] on return, which isn't the nicest. However IMO the benefit of these kind of advices is that it removes the fact that assignments to method parameters suddenly have side effects, which can be confusing for instrumentation beginners.

I'm not intending to have the automatic conversion as a fire and forget tool. Rather that you run the tool, review and potentially adjust the results and then open a PR. It is intended to save us from the repetitive manual and therefore easily error prone tasks of applying the same rewrite patterns over and over again.