Next attempt to extract common logic from instrumentations.

[iNikem]

I tried to reduce code repetition in DB client instrumentations, jdbc and mongo ones.

@trask please take a look at io.opentelemetry.auto.instrumentation.jdbc.JDBCDecorator#startSpan method. I currently don't see a good way (without repetitive code) to avoid returning null from it.

[trask]

I think it's safer for the CallDepth thread local management to be more explicit, for similar reasons as for the scope thread local management (that it's easy to miss decrementing/closing under all conditions).

Also this would keep the CallDepth story consistent whether it's sync or async (as opposed to resetting the CallDepth inside of TRACER.end(span) which only works if it's sync).

What do you think about the template below for advice that needs to track CallDepth?

The template makes sure we always increment/decrement the CallDepth first thing, making sure we never miss that.

It also passes CallDepth between @OnMethodEnter/@OnMethodExit which saves a thread local lookup in @OnMethodExit which is nice.

It would require adding a few new methods to the CallDepthThreadLocalMap API, but the existing methods could stay and we could migrate over slowly.

The point of TRACER.getCallDepth() below is just to encapsulate the call depth key inside of the Tracer, instead of calling CallDepthThreadLocalMap.getCallDepth(Statement.class) directly from the advice.

  @Advice.OnMethodEnter
  public static CallDepth onEnter(
      ...
      @Advice.Local("otelSpan") Span span,
      @Advice.Local("otelScope") Scope scope) {

    CallDepth callDepth = TRACER.getCallDepth();
    if (callDepth.increment() && TRACER.shouldCreateSpan(...)) {
      span = TRACER.startSpan(...);
      scope = TRACER.withSpan(span);
    }
    return callDepth;
  }

  @Advice.OnMethodExit
  public static void onExit(
      ...
      @Advice.Thrown Throwable throwable,
      @Advice.Local("otelSpan") Span span,
      @Advice.Local("otelScope") Scope scope,
      @Advice.Enter CallDepth callDepth) {

    if (depth.decrement() && scope != null) {
      scope.close();
      if (throwable == null) {
        TRACER.end(...);
      } else {
        TRACER.endExceptionally(...);
      }
    }
  }

This doesn't solve the problem in this PR of returning null from startSpan, but i'm realizing that doesn't bother me so much, and i can see the value in this PR, in which case maybe we should change the general @OnMethodEnter template above to remove the shouldCreateSpan method and integrate that decision into startSpan, e.g.

  @Advice.OnMethodEnter
  public static CallDepth onEnter(
      ...
      @Advice.Local("otelSpan") Span span,
      @Advice.Local("otelScope") Scope scope) {

    CallDepth callDepth = TRACER.getCallDepth();
    if (callDepth.increment()) {
      span = TRACER.startSpan(...);
      if (span != null) {
        scope = TRACER.withSpan(span);
      }
    }
    return callDepth;
  }

[iNikem]

First, how does returning null not bother you any more? :) This essentially rollbacks your #499 .

Second, it is an open question for me, btw, which instrumentations require this CallDepth management? Looks like we have 2 similar mechanism: CallDepth and the one used by HttpServer instrumentations (attaching span to requests). Is there some hidden abstraction?

Third, what is the difference between span, scope and callDepth? Why we store two of them in local variables but return the third?

Forth, 😭 for so much copy-paste.

Fifth, yeah, that may work :)

[trask]

First, how does returning null not bother you any more? :) This essentially rollbacks your #499 .

I didn't say it doesn't bother me at all 😂

Second, it is an open question for me, btw, which instrumentations require this CallDepth management? Looks like we have 2 similar mechanism: CallDepth and the one used by HttpServer instrumentations (attaching span to requests). Is there some hidden abstraction?

CallDepth is primarily needed when dealing with overloaded methods that delegate to each other, in order to prevent duplicate spans. It doesn't work for HttpServer instrumentation b/c in that case the calls are across different threads.

We can probably reduce the number of places that CallDepth tracking is needed once we implement #465.

Third, what is the difference between span, scope and callDepth? Why we store two of them in local variables but return the third?

Good question, let's make them all @Advice.Local parameters.

Forth, 😭 for so much copy-paste.

I think it's a good compromise, but let's see how it looks as we start spreading it across various instrumentation, we can always revisit.

Fifth, yeah, that may work :)

👍

[trask]

i like this, minor suggestions below

[trask]

👍