Review comments: Replace un-needed instrumentation by just propagating scope not spans
MODIFY
dd-java-agent/instrumentation/aws-java-sdk-2.2/src/main/java/datadog/trace/instrumentation/aws/v2/AbstractAwsClientInstrumentation.java
dd-java-agent/instrumentation/aws-java-sdk-2.2/src/main/java/datadog/trace/instrumentation/aws/v2/AbstractAwsClientInstrumentation.java
@@ -6,7 +6,7 @@ public abstract class AbstractAwsClientInstrumentation extends Instrumenter.Defa
private static final String INSTRUMENTATION_NAME = "aws-sdk";
public AbstractAwsClientInstrumentation() {
- super("hibernate-core");
+ super(INSTRUMENTATION_NAME);
}
@Override

MODIFY
dd-java-agent/instrumentation/hibernate/hibernate.gradle
dd-java-agent/instrumentation/hibernate/hibernate.gradle
@@ -9,7 +9,7 @@ muzzle {
pass {
group = "org.hibernate"
module = "hibernate-core"
- versions = "[5.0.0.Final, 5.+]"
+ versions = "[5.0.0.Final,)"
}
}
@@ -44,6 +44,6 @@ dependencies {
testCompile "com.sun.xml.bind:jaxb-impl:2.2.11"
testCompile "javax.activation:activation:1.1.1"
- latestDepTestCompile group: 'org.hibernate', name: 'hibernate-core', version: '5.+'
+ latestDepTestCompile group: 'org.hibernate', name: 'hibernate-core', version: '+'
latestDepTestCompile group: 'com.h2database', name: 'h2', version: '1.4.197'
}

MODIFY
dd-java-agent/instrumentation/hibernate/src/main/java/datadog/trace/instrumentation/hibernate/CriteriaInstrumentation.java
dd-java-agent/instrumentation/hibernate/src/main/java/datadog/trace/instrumentation/hibernate/CriteriaInstrumentation.java
@@ -34,8 +34,8 @@ public class CriteriaInstrumentation extends Instrumenter.Default {
@Override
public String[] helperClassNames() {
return new String[] {
- "datadog.trace.instrumentation.hibernate.SessionMethodUtils",
- "datadog.trace.instrumentation.hibernate.SessionState",
+ packageName + ".SessionMethodUtils",
+ packageName + ".SessionState",
"datadog.trace.agent.decorator.BaseDecorator",
"datadog.trace.agent.decorator.ClientDecorator",
"datadog.trace.agent.decorator.DatabaseClientDecorator",
@@ -66,7 +66,7 @@ public class CriteriaInstrumentation extends Instrumenter.Default {
InstrumentationContext.get(Criteria.class, SessionState.class);
return SessionMethodUtils.startScopeFrom(
- contextStore, criteria, "hibernate.criteria." + name, null);
+ contextStore, criteria, "hibernate.criteria." + name, null, true);
}
@Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class)

DELETE
dd-java-agent/instrumentation/hibernate/src/main/java/datadog/trace/instrumentation/hibernate/IteratorInstrumentation.java
-package datadog.trace.instrumentation.hibernate;
-
-import static datadog.trace.agent.tooling.ByteBuddyElementMatchers.safeHasSuperType;
-import static java.util.Collections.singletonMap;
-import static net.bytebuddy.matcher.ElementMatchers.isInterface;
-import static net.bytebuddy.matcher.ElementMatchers.isMethod;
-import static net.bytebuddy.matcher.ElementMatchers.named;
-import static net.bytebuddy.matcher.ElementMatchers.not;
-
-import com.google.auto.service.AutoService;
-import datadog.trace.agent.tooling.Instrumenter;
-import datadog.trace.bootstrap.ContextStore;
-import datadog.trace.bootstrap.InstrumentationContext;
-import java.util.Map;
-import net.bytebuddy.asm.Advice;
-import net.bytebuddy.description.method.MethodDescription;
-import net.bytebuddy.description.type.TypeDescription;
-import net.bytebuddy.implementation.bytecode.assign.Assigner;
-import net.bytebuddy.matcher.ElementMatcher;
-import org.hibernate.engine.HibernateIterator;
-
-@AutoService(Instrumenter.class)
-public class IteratorInstrumentation extends Instrumenter.Default {
-
- public IteratorInstrumentation() {
- super("hibernate-core");
- }
-
- @Override
- public Map<String, String> contextStore() {
- return singletonMap("org.hibernate.engine.HibernateIterator", SessionState.class.getName());
- }
-
- @Override
- public String[] helperClassNames() {
- return new String[] {
- "datadog.trace.instrumentation.hibernate.SessionMethodUtils",
- "datadog.trace.instrumentation.hibernate.SessionState",
- "datadog.trace.agent.decorator.BaseDecorator",
- "datadog.trace.agent.decorator.ClientDecorator",
- "datadog.trace.agent.decorator.DatabaseClientDecorator",
- "datadog.trace.agent.decorator.OrmClientDecorator",
- packageName + ".HibernateDecorator",
- };
- }
-
- @Override
- public ElementMatcher<TypeDescription> typeMatcher() {
- return not(isInterface())
- .and(safeHasSuperType(named("org.hibernate.engine.HibernateIterator")));
- }
-
- @Override
- public Map<? extends ElementMatcher<? super MethodDescription>, String> transformers() {
- return singletonMap(
- isMethod().and(named("next").or(named("remove"))), IteratorAdvice.class.getName());
- }
-
- public static class IteratorAdvice {
-
- @Advice.OnMethodEnter(suppress = Throwable.class)
- public static SessionState startMethod(
- @Advice.This final HibernateIterator iterator, @Advice.Origin("#m") final String name) {
-
- final ContextStore<HibernateIterator, SessionState> contextStore =
- InstrumentationContext.get(HibernateIterator.class, SessionState.class);
-
- return SessionMethodUtils.startScopeFrom(
- contextStore, iterator, "hibernate.iterator." + name, null);
- }
-
- @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class)
- public static void endMethod(
- @Advice.Enter final SessionState state,
- @Advice.Thrown final Throwable throwable,
- @Advice.Return(typing = Assigner.Typing.DYNAMIC) final Object entity) {
-
- SessionMethodUtils.closeScope(state, throwable, entity);
- }
- }
-}

MODIFY
dd-java-agent/instrumentation/hibernate/src/main/java/datadog/trace/instrumentation/hibernate/ProcedureCallInstrumentation.java
dd-java-agent/instrumentation/hibernate/src/main/java/datadog/trace/instrumentation/hibernate/ProcedureCallInstrumentation.java
@@ -33,8 +33,8 @@ public class ProcedureCallInstrumentation extends Instrumenter.Default {
@Override
public String[] helperClassNames() {
return new String[] {
- "datadog.trace.instrumentation.hibernate.SessionMethodUtils",
- "datadog.trace.instrumentation.hibernate.SessionState",
+ packageName + ".SessionMethodUtils",
+ packageName + ".SessionState",
"datadog.trace.agent.decorator.BaseDecorator",
"datadog.trace.agent.decorator.ClientDecorator",
"datadog.trace.agent.decorator.DatabaseClientDecorator",
@@ -65,7 +65,7 @@ public class ProcedureCallInstrumentation extends Instrumenter.Default {
final SessionState state =
SessionMethodUtils.startScopeFrom(
- contextStore, call, "hibernate.procedure." + name, call.getProcedureName());
+ contextStore, call, "hibernate.procedure." + name, call.getProcedureName(), true);
return state;
}

MODIFY
dd-java-agent/instrumentation/hibernate/src/main/java/datadog/trace/instrumentation/hibernate/QueryInstrumentation.java
dd-java-agent/instrumentation/hibernate/src/main/java/datadog/trace/instrumentation/hibernate/QueryInstrumentation.java
@@ -36,8 +36,8 @@ public class QueryInstrumentation extends Instrumenter.Default {
@Override
public String[] helperClassNames() {
return new String[] {
- "datadog.trace.instrumentation.hibernate.SessionMethodUtils",
- "datadog.trace.instrumentation.hibernate.SessionState",
+ packageName + ".SessionMethodUtils",
+ packageName + ".SessionState",
"datadog.trace.agent.decorator.BaseDecorator",
"datadog.trace.agent.decorator.ClientDecorator",
"datadog.trace.agent.decorator.DatabaseClientDecorator",
@@ -74,7 +74,8 @@ public class QueryInstrumentation extends Instrumenter.Default {
// Note: We don't know what the entity is until the method is returning.
final SessionState state =
- SessionMethodUtils.startScopeFrom(contextStore, query, "hibernate.query." + name, null);
+ SessionMethodUtils.startScopeFrom(
+ contextStore, query, "hibernate.query." + name, null, true);
DECORATOR.onStatement(state.getMethodScope().span(), query.getQueryString());
return state;
}

MODIFY
dd-java-agent/instrumentation/hibernate/src/main/java/datadog/trace/instrumentation/hibernate/SessionFactoryInstrumentation.java
dd-java-agent/instrumentation/hibernate/src/main/java/datadog/trace/instrumentation/hibernate/SessionFactoryInstrumentation.java
@@ -38,7 +38,7 @@ public class SessionFactoryInstrumentation extends Instrumenter.Default {
@Override
public String[] helperClassNames() {
return new String[] {
- "datadog.trace.instrumentation.hibernate.SessionState",
+ packageName + ".SessionState",
"datadog.trace.agent.decorator.BaseDecorator",
"datadog.trace.agent.decorator.ClientDecorator",
"datadog.trace.agent.decorator.DatabaseClientDecorator",

MODIFY
dd-java-agent/instrumentation/hibernate/src/main/java/datadog/trace/instrumentation/hibernate/SessionInstrumentation.java
dd-java-agent/instrumentation/hibernate/src/main/java/datadog/trace/instrumentation/hibernate/SessionInstrumentation.java
@@ -15,9 +15,12 @@ import datadog.trace.agent.tooling.Instrumenter;
import datadog.trace.bootstrap.ContextStore;
import datadog.trace.bootstrap.InstrumentationContext;
import io.opentracing.Span;
+import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
+import java.util.HashSet;
import java.util.Map;
+import java.util.Set;
import net.bytebuddy.asm.Advice;
import net.bytebuddy.description.method.MethodDescription;
import net.bytebuddy.description.type.TypeDescription;
@@ -27,7 +30,6 @@ import org.hibernate.Criteria;
import org.hibernate.Query;
import org.hibernate.SharedSessionContract;
import org.hibernate.Transaction;
-import org.hibernate.engine.HibernateIterator;
import org.hibernate.procedure.ProcedureCall;
@AutoService(Instrumenter.class)
@@ -45,15 +47,14 @@ public class SessionInstrumentation extends Instrumenter.Default {
map.put("org.hibernate.Transaction", SessionState.class.getName());
map.put("org.hibernate.Criteria", SessionState.class.getName());
map.put("org.hibernate.procedure.ProcedureCall", SessionState.class.getName());
- map.put("org.hibernate.engine.HibernateIterator", SessionState.class.getName());
return Collections.unmodifiableMap(map);
}
@Override
public String[] helperClassNames() {
return new String[] {
- "datadog.trace.instrumentation.hibernate.SessionMethodUtils",
- "datadog.trace.instrumentation.hibernate.SessionState",
+ packageName + ".SessionMethodUtils",
+ packageName + ".SessionState",
"datadog.trace.agent.decorator.BaseDecorator",
"datadog.trace.agent.decorator.ClientDecorator",
"datadog.trace.agent.decorator.DatabaseClientDecorator",
@@ -150,15 +151,20 @@ public class SessionInstrumentation extends Instrumenter.Default {
public static class SessionMethodAdvice {
+ public static final Set<String> SCOPE_ONLY_METHODS =
+ new HashSet<>(Arrays.asList("immediateLoad", "internalLoad"));
+
@Advice.OnMethodEnter(suppress = Throwable.class)
public static SessionState startMethod(
@Advice.This final SharedSessionContract session,
@Advice.Origin("#m") final String name,
@Advice.Argument(0) final Object entity) {
+ final boolean startSpan = !SCOPE_ONLY_METHODS.contains(name);
final ContextStore<SharedSessionContract, SessionState> contextStore =
InstrumentationContext.get(SharedSessionContract.class, SessionState.class);
- return SessionMethodUtils.startScopeFrom(contextStore, session, "hibernate." + name, entity);
+ return SessionMethodUtils.startScopeFrom(
+ contextStore, session, "hibernate." + name, entity, startSpan);
}
@Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class)
@@ -169,19 +175,6 @@ public class SessionInstrumentation extends Instrumenter.Default {
@Advice.Return(typing = Assigner.Typing.DYNAMIC) final Object returned) {
SessionMethodUtils.closeScope(sessionState, throwable, returned);
-
- // Attach instrumentation to any returned object (of eligible type).
- if (returned == null) {
- return;
- }
- final ContextStore<SharedSessionContract, SessionState> sessionContextStore =
- InstrumentationContext.get(SharedSessionContract.class, SessionState.class);
- if (returned instanceof HibernateIterator) {
- final ContextStore<HibernateIterator, SessionState> iteratorContextStore =
- InstrumentationContext.get(HibernateIterator.class, SessionState.class);
- SessionMethodUtils.attachSpanFromStore(
- sessionContextStore, session, iteratorContextStore, (HibernateIterator) returned);
- }
}
}

MODIFY
dd-java-agent/instrumentation/hibernate/src/main/java/datadog/trace/instrumentation/hibernate/SessionMethodUtils.java
dd-java-agent/instrumentation/hibernate/src/main/java/datadog/trace/instrumentation/hibernate/SessionMethodUtils.java
@@ -2,6 +2,7 @@ package datadog.trace.instrumentation.hibernate;
import static datadog.trace.instrumentation.hibernate.HibernateDecorator.DECORATOR;
+import datadog.trace.bootstrap.CallDepthThreadLocalMap;
import datadog.trace.bootstrap.ContextStore;
import io.opentracing.Scope;
import io.opentracing.Span;
@@ -15,28 +16,33 @@ public class SessionMethodUtils {
final ContextStore<TARGET, SessionState> contextStore,
final TARGET spanKey,
final String operationName,
- final ENTITY entity) {
+ final ENTITY entity,
+ final boolean createSpan) {
final SessionState sessionState = contextStore.get(spanKey);
if (sessionState == null) {
- // No state found. We aren't in a Session.
- return null;
+ return null; // No state found. We aren't in a Session.
}
- if (sessionState.getMethodScope() != null) {
- // This method call was re-entrant. Do nothing, since it is being traced by the parent/first
- // call.
- return null;
+ final int depth = CallDepthThreadLocalMap.incrementCallDepth(SessionMethodUtils.class);
+ if (depth > 0) {
+ return null; // This method call is being traced already.
}
- final Scope scope =
+ final Scope scope;
+ if (createSpan) {
+ scope =
GlobalTracer.get()
.buildSpan(operationName)
.asChildOf(sessionState.getSessionSpan())
.startActive(true);
DECORATOR.afterStart(scope.span());
DECORATOR.onOperation(scope.span(), entity);
+ } else {
+ scope = GlobalTracer.get().scopeManager().activate(sessionState.getSessionSpan(), false);
+ sessionState.setHasChildSpan(false);
+ }
sessionState.setMethodScope(scope);
return sessionState;
@@ -52,9 +58,10 @@ public class SessionMethodUtils {
return;
}
+ CallDepthThreadLocalMap.reset(SessionMethodUtils.class);
final Scope scope = sessionState.getMethodScope();
final Span span = scope.span();
- if (span != null) {
+ if (span != null && sessionState.hasChildSpan) {
DECORATOR.onError(span, throwable);
if (entity != null) {
DECORATOR.onOperation(span, entity);

MODIFY
dd-java-agent/instrumentation/hibernate/src/main/java/datadog/trace/instrumentation/hibernate/SessionState.java
dd-java-agent/instrumentation/hibernate/src/main/java/datadog/trace/instrumentation/hibernate/SessionState.java
@@ -9,4 +9,5 @@ import lombok.NonNull;
public class SessionState {
@NonNull public Span sessionSpan;
public Scope methodScope;
+ public boolean hasChildSpan = true;
}

MODIFY
dd-java-agent/instrumentation/hibernate/src/main/java/datadog/trace/instrumentation/hibernate/TransactionInstrumentation.java
dd-java-agent/instrumentation/hibernate/src/main/java/datadog/trace/instrumentation/hibernate/TransactionInstrumentation.java
@@ -34,8 +34,8 @@ public class TransactionInstrumentation extends Instrumenter.Default {
@Override
public String[] helperClassNames() {
return new String[] {
- "datadog.trace.instrumentation.hibernate.SessionMethodUtils",
- "datadog.trace.instrumentation.hibernate.SessionState",
+ packageName + ".SessionMethodUtils",
+ packageName + ".SessionState",
"datadog.trace.agent.decorator.BaseDecorator",
"datadog.trace.agent.decorator.ClientDecorator",
"datadog.trace.agent.decorator.DatabaseClientDecorator",
@@ -65,7 +65,7 @@ public class TransactionInstrumentation extends Instrumenter.Default {
InstrumentationContext.get(Transaction.class, SessionState.class);
return SessionMethodUtils.startScopeFrom(
- contextStore, transaction, "hibernate.transaction.commit", null);
+ contextStore, transaction, "hibernate.transaction.commit", null, true);
}
@Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class)

MODIFY
dd-java-agent/instrumentation/hibernate/src/test/groovy/QueryTest.groovy
dd-java-agent/instrumentation/hibernate/src/test/groovy/QueryTest.groovy
@@ -147,7 +147,7 @@ class QueryTest extends AbstractHibernateTest {
expect:
assertTraces(1) {
- trace(0, 6) {
+ trace(0, 4) {
span(0) {
serviceName "hibernate"
resourceName "hibernate.session"
@@ -175,32 +175,6 @@ class QueryTest extends AbstractHibernateTest {
}
}
span(2) {
- serviceName "hibernate"
- resourceName "hibernate.iterator.next"
- operationName "hibernate.iterator.next"
- spanType DDSpanTypes.HIBERNATE
- childOf span(0)
- tags {
- "$Tags.COMPONENT.key" "java-hibernate"
- "$Tags.SPAN_KIND.key" Tags.SPAN_KIND_CLIENT
- "$DDTags.SPAN_TYPE" DDSpanTypes.HIBERNATE
- defaultTags()
- }
- }
- span(3) {
- serviceName "hibernate"
- resourceName "hibernate.iterator.next"
- operationName "hibernate.iterator.next"
- spanType DDSpanTypes.HIBERNATE
- childOf span(0)
- tags {
- "$Tags.COMPONENT.key" "java-hibernate"
- "$Tags.SPAN_KIND.key" Tags.SPAN_KIND_CLIENT
- "$DDTags.SPAN_TYPE" DDSpanTypes.HIBERNATE
- defaultTags()
- }
- }
- span(4) {
serviceName "hibernate"
resourceName "from Value"
operationName "hibernate.iterate"
@@ -213,9 +187,9 @@ class QueryTest extends AbstractHibernateTest {
defaultTags()
}
}
- span(5) {
+ span(3) {
serviceName "h2"
- childOf span(4)
+ childOf span(2)
}
}
}

MODIFY
dd-trace-api/src/main/java/datadog/trace/api/DDSpanTypes.java
dd-trace-api/src/main/java/datadog/trace/api/DDSpanTypes.java
@@ -14,7 +14,7 @@ public class DDSpanTypes {
public static final String REDIS = "redis";
public static final String MEMCACHED = "memcached";
public static final String ELASTICSEARCH = "elasticsearch";
- public static final String HIBERNATE = "hibernate"; // TODO: Could this just be "db", or "orm"?
+ public static final String HIBERNATE = "hibernate";
public static final String MESSAGE_CLIENT = "queue";
public static final String MESSAGE_CONSUMER = "queue";

Review comments: Replace un-needed instrumentation by just propagating scope not spans