Refactor out camel ContextWithScope into a bootstrap package so it can be referenced by multiple ClassLoaders.

[evanderkoogh]

As mentioned in #14238 there was an issue when using the Apache Camel instrumentation inside a blueprint container where some of the instrumentation classes were loaded by different classloaders at different times. This led to a loss of context, and many orphaned spans on a trace.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: evanderkoogh / name: Erwin van der Koogh (7640875, d4dce67, 886f841, 41c5552, 4097d39)
• ✅ login: otelbot[bot] (335daa4)

[otelbot-java-instrumentation]

🔧 The result from spotlessApply was committed to the PR branch.

[laurit]

Imo this isn't equivalent to the previous code. Previously instrumenter().end() was skipped when scope field in ContextWithScope was null, now it is always run. You could probably get this done with less changes by keeping the request field in ContextWithScope, but changing the type to Object. Then you would only need to modify ActiveContextManager.deactivate method.

[evanderkoogh]

But if I keep Request field and make it an Object, I would still need to take interfaces like Exchange to the bootstrap package (and thus have a dependency on camel in the bootstrap package) right?

[laurit]

No, you don't. It is similar to storing anything in ArrayList, which is in boot loader. Data is kept in Object[] and contain any kind of object, including the ones whose class is not in boot loader. When you read back the object you cast it to the desired type.

[evanderkoogh]

Oh you mean something like this?

/*
 * Copyright The OpenTelemetry Authors
 * SPDX-License-Identifier: Apache-2.0
 */

package io.opentelemetry.javaagent.instrumentation.apachecamel;

import com.google.auto.value.AutoValue;
import io.opentelemetry.api.trace.SpanKind;

@AutoValue
abstract class CamelRequest {

  public static CamelRequest create(
      Object spanDecorator,
      Object exchange,
      Object endpoint,
      CamelDirection camelDirection,
      SpanKind spanKind) {
    return new AutoValue_CamelRequest(spanDecorator, exchange, endpoint, camelDirection, spanKind);
  }

  public abstract Object getSpanDecorator();

  public abstract Object getExchange();

  public abstract Object getEndpoint();

  public abstract CamelDirection getCamelDirection();

  public abstract SpanKind getSpanKind();
}

[evanderkoogh]

@laurit Apologies about the delay.. been rethinking about how to solve this..
Another issue with the double classloader path is that we end up with duplicate spans.. each one started by the instrumentation in the different class loaders.

But the actual Camel Exchange object used by both of these class loaders is (and could only ever be) from one of those ClassLoaders.. So if we suppress the creation of a span if the ClassLoader of the instrumentation code != the ClassLoader of the Exchange object, we solve two birds with one stone.

That way we don't need to go through the trouble of having to load certain things in the bootstrap ClassLoader and we don't end up with duplicate spans. But that is slightly more cumbersome that I had hoped it would be? The InstrumenterBuilder is used, but there seems to be no good way to override the shouldStart method?

Is that a possible avenue to explore?

[laurit]

Is that a possible avenue to explore?

definitely. I don't know what exactly is causing the issue for you, but lets assume that the problem is that the instrumentation in CamelContextInstrumentation instruments all implementations of CamelContext and you have one implementation that extends another and that the the start method is called for both. That could cause CamelTracingService to be added twice. You could resolve this by excluding one of the classes in the typeMatcher method or use CallDepth like in

opentelemetry-java-instrumentation/instrumentation/jdbc/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jdbc/StatementInstrumentation.java

Line 82 in 09e2ff0

callDepth = CallDepth.forClass(Statement.class);

to suppress the instrumentation in the nested call. Other possibility could be using classLoaderMatcher in InstrumentationModule if you can recognize the class loader where you don't wish to run the instrumentation.

[evanderkoogh]

Ohh.. that looks very promising indeed. I will have a play with those in the next few days to see what I can come up with.. cheers!

[laurit]

No, I meant

  private static class ContextWithScope {
    @Nullable private final ContextWithScope parent;
    @Nullable private final Context context;
    private final Object request;
    @Nullable private final Scope scope;

[laurit]

On second thought that might not work. If CamelRequest is in multiple class loaders then could also trigger a ClassCastException. Your version would work though.