Skip to content

Conversation

laurit
Copy link
Contributor

@laurit laurit commented Sep 23, 2025

Resolves #14733
Currently we start tracing with the the first jdbc class that is called. This PR moves the tracing to the first non-wrapper. Wrapper is detected by attempting to unwrap the input object, if it unwraps to a different instance. Note that shardingspere, that was used in the issue, types can't be unwrapped but it is fine because these types are already excluded via a ignored types configurer. What affects the problem described in the issue is skipping the hikari cp wrapper that is on top of the shardingsphere.

@laurit laurit marked this pull request as ready for review September 24, 2025 05:04
@laurit laurit requested a review from a team as a code owner September 24, 2025 05:04
Copy link
Member

@zeitlinger zeitlinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure if this deserves a dedicated test or not

@laurit laurit added this to the v2.21.0 milestone Sep 24, 2025
Copy link
Contributor

@breedx-splk breedx-splk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree that a test would be nice here, but could be in a follow-up effort.

return DATASOURCE_INSTRUMENTER;
}

private static final Cache<Class<?>, Boolean> wrapperClassCache = Cache.weak();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does ClassValue work here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't work in the standard way because we need the actual object to decide whether it can be unwrapped. I think the most straightforward way we could use class value would be to use

  private static final ClassValue<IsWrapper> wrapperClassCache =
      new ClassValue<IsWrapper>() {
        @Override
        protected IsWrapper computeValue(Class<?> type) {
          return new IsWrapper();
        }
      };
  
  private static class IsWrapper {
    volatile boolean valueSet;
    volatile boolean value;
    
    boolean getValue(Wrapper object, Class<? extends Wrapper> clazz) {
      if (valueSet) {
        return value;
      }
      value = isWrapperInternal(object, clazz);
      valueSet = true;
      return true;
    }
  }

  /**
   * Returns true if the given object is a wrapper and shouldn't be instrumented. We'll instrument
   * the underlying object called by the wrapper instead.
   */
  public static <T extends Wrapper> boolean isWrapper(T object, Class<T> clazz) {
    IsWrapper isWrapper = wrapperClassCache.get(clazz);
    return isWrapper.getValue(object, clazz);
  }

  private static boolean isWrapperInternal(Wrapper object, Class<? extends Wrapper> clazz) {
    try {
      // we are dealing with a wrapper when the object unwraps to a different instance
      if (object.isWrapperFor(clazz)) {
        Wrapper unwrapped = object.unwrap(clazz);
        if (object != unwrapped) {
          return true;
        }
      }
    } catch (SQLException | AbstractMethodError e) {
      // ignore
    }
    return false;
  }

or

  private static final int UNDEFINED = 0;
  private static final int WRAPPER = 1;
  private static final int NOT_WRAPPER = -1;
  private static final ClassValue<AtomicInteger> wrapperClassCache =
      new ClassValue<AtomicInteger>() {
        @Override
        protected AtomicInteger computeValue(Class<?> type) {
          return new AtomicInteger();
        }
      };

  /**
   * Returns true if the given object is a wrapper and shouldn't be instrumented. We'll instrument
   * the underlying object called by the wrapper instead.
   */
  public static <T extends Wrapper> boolean isWrapper(T object, Class<T> clazz) {
    AtomicInteger isWrapper = wrapperClassCache.get(clazz);
    int value = isWrapper.get();
    if (value != UNDEFINED) {
      return value == WRAPPER;
    }
    boolean result = isWrapperInternal(object, clazz);
    isWrapper.set(result ? WRAPPER : NOT_WRAPPER);
    return result;
  }

  private static <T extends Wrapper> boolean isWrapperInternal(T object, Class<T> clazz) {
    try {
      // we are dealing with a wrapper when the object unwraps to a different instance
      if (object.isWrapperFor(clazz)) {
        Wrapper unwrapped = object.unwrap(clazz);
        if (object != unwrapped) {
          return true;
        }
      }
    } catch (SQLException | AbstractMethodError e) {
      // ignore
    }
    return false;
  }

which is slightly more code than the weak cache, but not too bad really.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't work in the standard way because we need the actual object to decide whether it can be unwrapped

ah, makes sense

I like the IsWrapper version better than the AtomicInteger version, but I also like your existing implementation given that it's not really a standard usage of ClassValue anyways

@laurit laurit merged commit 62dd98a into open-telemetry:main Sep 26, 2025
89 checks passed
@laurit laurit deleted the jdbc-wrapper branch September 26, 2025 04:47
mznet pushed a commit to mznet/opentelemetry-java-instrumentation that referenced this pull request Sep 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Some JDBC Span Attributes are Wrong When Using Shardingsphere and HikariCP

5 participants