Make more tests run with indy

[laurit]

Resolves #9552
Add an option to inject helper class into application class loader. This is useful for injecting helper classes that rely on being in the same runtime package as application classes. This will work only if these classes don't depend on any other helper classes (these are loaded into instrumentation class loader).
Make instrumentation class loader delegate to agent class loader only for classes in io.opentelemetry.javaagent. This will avoid linkage errors when instrumentation touches classes present in both application and the agent loader. I'd imagine that eventually we might need to make this use a configurable package list.
This pr also adds back muzzle matcher.
@JonasKunz could you review

[JonasKunz]

Great work, thanks for implementing this!
You can also link #9552 from this PR so that it closes when the PR is merged.

Make instrumentation class loader delegate to agent class loader only for classes in io.opentelemetry.javaagent. This will avoid linkage errors when instrumentation touches classes present in both application and the agent loader. I'd imagine that eventually we might need to make this use a configurable package list.

Sounds reasonable to me, we can add the config option later. We will certainly need it for the OpenTelemetry-API-Bridge instrumentation.

[JonasKunz]

I personally would have preferred to add a injector.copy(String classname).inject(InjectionMode.CLASS_ONLY) method to ClassInjector than a separate method.

This would allow to see all injected classes (proxies and copies) with a glance at the injectClasses method, allow the injection of the bytecode as a resource if needed and easier future extension.

[laurit]

I think we can change this later. I implemented most of this before the pr with ClassInjector was merged. My hope is that if we create enough of a mess with the apis then @mateuszrzeszutek will come and clean all of this up.

[JonasKunz]

Fine for me, hopefully for @mateuszrzeszutek too 😆

[JonasKunz]

If my understanding of AgentCachingPoolStrategy is correct, our TypeDescription-Cache ensures that we never hold a strong reference to classloaders, correct?

As a result, the caching won't really work here and it will also cause good entries to be evicted from the cache if it is fixed-size due to "flooding" the cache with temporary classloader.

I think we could avoid these problems and keep the muzzle-matching efficient by instead altering ReferenceMatcher.createTypePool:

// loader cannot be null, must pass "bootstrap proxy" instead of bootstrap class loader
  private TypePool createTypePool(ClassLoader loader) {
    // ok to use locationStrategy() without fallback bootstrap proxy here since loader is non-null
    TypePool instrumentedClPool = AgentTooling.poolStrategy()
        .typePool(AgentTooling.locationStrategy().classFileLocator(loader), loader);
    if(instrumentationModule.isIndy()) {
      TypePool instrumentedClPool = AgentTooling.poolStrategy()
          .typePool(AgentTooling.locationStrategy().classFileLocator(loader), loader);
      ClassLoader moduleOrAgentCl = instrumentationModule.getClass().getClassLoader();
      TypePool agentOrExtensionPool = AgentTooling.poolStrategy()
          .typePool(AgentTooling.locationStrategy().classFileLocator(moduleOrAgentCl), moduleOrAgentCl);
      return new TypePool() {
        @Override
        public Resolution describe(String name) {
          if(name.startsWith("io.opentelemetry.javaagent")) {
            return agentOrExtensionPool.describe(name);
          }
          return instrumentedClPool.describe(name);
        }

        @Override
        public void clear() {
          instrumentedClPool.clear();
          agentOrExtensionPool.clear();
        }
      };
    }
    return instrumentedClPool;
  }

[laurit]

This will require further investigation. As far as I understand we don't pass the temporary class loader here but the real instrumentation loader. Keep in mind that having to transform a class is really a special case, relatively few classes get transformed. It should be fine as long as loading classes that don't get transformed does minimal work.

[JonasKunz]

This will require further investigation. As far as I understand we don't pass the temporary class loader here but the real instrumentation loader.

Correct, but we make sure that we never hold a strong reference to that InstrumentationModuleClassloader, because that would result in a strong reference to the instrumented application classloader.

The InstrumentationModuleClassloader will only be strongly-referenced when the first invokedynamic-instruction is bootstrapped, therefore it may be GCed until this happens.

Keep in mind that having to transform a class is really a special case, relatively few classes get transformed. It should be fine as long as loading classes that don't get transformed does minimal work.

Makes sense, so we can keep it as is for now. I just thought that my proposed alternative with the custom TypePool might avoid these issues altogether, but we don't need to do this now if it is too much effort.

[laurit]

Would it make sense to insert a strong reference from application class loader to InstrumentationModuleClassloader so that InstrumentationModuleClassloader couldn't be gcd before the application class loader? Could do this be defining a class in application class loader that could keep the reference in a static field or map.

[JonasKunz]

I don't think that this is worth the effort and complexity now, because IINM the custom TypePool I suggested would still be better and imo less complex: When caching TypeDescriptions the corresponding classloader is used as part of the cache-key. If we use the InstrumentationModuleClassloader here, this will only allow the cache to be effective for classes from the same classloader when checking for the same InstrumentationModule.

If we instead use something like the custom type pool I suggested, the TypeDescriptions will instead be associated with either the application classloader or the agent/extension classloader, depending where they come from, allowing a better "reuse" in theory.

I think for now the "risk" of the InstrumentationModuleCL being GCed here is okay. If you want to, I can do a follow-up PR trying to implement that custom type pool.