From 093387bb01084d813e4e4f11d12bc4a97f9ffcad Mon Sep 17 00:00:00 2001 From: Will Gittoes Date: Thu, 28 Feb 2019 11:04:31 +1100 Subject: [PATCH] Review comments and additional testing --- .../hibernate/CriteriaInstrumentation.java | 16 +- .../hibernate/HibernateDecorator.java | 4 +- .../hibernate/HibernateInstrumentation.java | 5 + .../hibernate/IteratorInstrumentation.java | 15 +- .../ProcedureCallInstrumentation.java | 15 +- .../hibernate/QueryInstrumentation.java | 15 +- .../SessionFactoryInstrumentation.java | 17 +- .../hibernate/SessionInstrumentation.java | 8 +- .../hibernate/TransactionInstrumentation.java | 20 +- .../src/test/groovy/QueryTest.groovy | 58 ++++- .../src/test/groovy/SessionTest.groovy | 215 ++++++++++++++---- .../src/test/resources/hibernate.cfg.xml | 2 +- 12 files changed, 272 insertions(+), 118 deletions(-) create mode 100644 dd-java-agent/instrumentation/hibernate/src/main/java/datadog/trace/instrumentation/hibernate/HibernateInstrumentation.java diff --git a/dd-java-agent/instrumentation/hibernate/src/main/java/datadog/trace/instrumentation/hibernate/CriteriaInstrumentation.java b/dd-java-agent/instrumentation/hibernate/src/main/java/datadog/trace/instrumentation/hibernate/CriteriaInstrumentation.java index 3e305ff457..cf6230d8cf 100644 --- a/dd-java-agent/instrumentation/hibernate/src/main/java/datadog/trace/instrumentation/hibernate/CriteriaInstrumentation.java +++ b/dd-java-agent/instrumentation/hibernate/src/main/java/datadog/trace/instrumentation/hibernate/CriteriaInstrumentation.java @@ -1,6 +1,8 @@ package datadog.trace.instrumentation.hibernate; import static datadog.trace.agent.tooling.ByteBuddyElementMatchers.safeHasSuperType; +import static datadog.trace.instrumentation.hibernate.HibernateInstrumentation.INSTRUMENTATION_NAME; +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; @@ -10,8 +12,6 @@ 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.Collections; -import java.util.HashMap; import java.util.Map; import net.bytebuddy.asm.Advice; import net.bytebuddy.description.method.MethodDescription; @@ -24,14 +24,12 @@ import org.hibernate.Criteria; public class CriteriaInstrumentation extends Instrumenter.Default { public CriteriaInstrumentation() { - super("hibernate"); + super(INSTRUMENTATION_NAME); } @Override public Map contextStore() { - final Map map = new HashMap<>(); - map.put("org.hibernate.Criteria", SessionState.class.getName()); - return Collections.unmodifiableMap(map); + return singletonMap("org.hibernate.Criteria", SessionState.class.getName()); } @Override @@ -54,12 +52,9 @@ public class CriteriaInstrumentation extends Instrumenter.Default { @Override public Map, String> transformers() { - final Map, String> transformers = new HashMap<>(); - transformers.put( + return singletonMap( isMethod().and(named("list").or(named("uniqueResult")).or(named("scroll"))), CriteriaMethodAdvice.class.getName()); - - return transformers; } public static class CriteriaMethodAdvice { @@ -77,7 +72,6 @@ public class CriteriaInstrumentation extends Instrumenter.Default { @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) public static void endMethod( - @Advice.This final Criteria criteria, @Advice.Enter final SessionState state, @Advice.Thrown final Throwable throwable, @Advice.Return(typing = Assigner.Typing.DYNAMIC) final Object entity) { diff --git a/dd-java-agent/instrumentation/hibernate/src/main/java/datadog/trace/instrumentation/hibernate/HibernateDecorator.java b/dd-java-agent/instrumentation/hibernate/src/main/java/datadog/trace/instrumentation/hibernate/HibernateDecorator.java index 05aefe4c38..55c7510ab6 100644 --- a/dd-java-agent/instrumentation/hibernate/src/main/java/datadog/trace/instrumentation/hibernate/HibernateDecorator.java +++ b/dd-java-agent/instrumentation/hibernate/src/main/java/datadog/trace/instrumentation/hibernate/HibernateDecorator.java @@ -1,5 +1,7 @@ package datadog.trace.instrumentation.hibernate; +import static datadog.trace.instrumentation.hibernate.HibernateInstrumentation.INSTRUMENTATION_NAME; + import datadog.trace.agent.decorator.OrmClientDecorator; import datadog.trace.api.DDSpanTypes; import java.util.List; @@ -15,7 +17,7 @@ public class HibernateDecorator extends OrmClientDecorator { @Override protected String[] instrumentationNames() { - return new String[] {"hibernate"}; + return new String[] {INSTRUMENTATION_NAME}; } @Override diff --git a/dd-java-agent/instrumentation/hibernate/src/main/java/datadog/trace/instrumentation/hibernate/HibernateInstrumentation.java b/dd-java-agent/instrumentation/hibernate/src/main/java/datadog/trace/instrumentation/hibernate/HibernateInstrumentation.java new file mode 100644 index 0000000000..1fdfff41ed --- /dev/null +++ b/dd-java-agent/instrumentation/hibernate/src/main/java/datadog/trace/instrumentation/hibernate/HibernateInstrumentation.java @@ -0,0 +1,5 @@ +package datadog.trace.instrumentation.hibernate; + +public class HibernateInstrumentation { + public static final String INSTRUMENTATION_NAME = "hibernate-core"; +} diff --git a/dd-java-agent/instrumentation/hibernate/src/main/java/datadog/trace/instrumentation/hibernate/IteratorInstrumentation.java b/dd-java-agent/instrumentation/hibernate/src/main/java/datadog/trace/instrumentation/hibernate/IteratorInstrumentation.java index 7bf10d8912..a34f520b22 100644 --- a/dd-java-agent/instrumentation/hibernate/src/main/java/datadog/trace/instrumentation/hibernate/IteratorInstrumentation.java +++ b/dd-java-agent/instrumentation/hibernate/src/main/java/datadog/trace/instrumentation/hibernate/IteratorInstrumentation.java @@ -1,6 +1,8 @@ package datadog.trace.instrumentation.hibernate; import static datadog.trace.agent.tooling.ByteBuddyElementMatchers.safeHasSuperType; +import static datadog.trace.instrumentation.hibernate.HibernateInstrumentation.INSTRUMENTATION_NAME; +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; @@ -10,8 +12,6 @@ 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.Collections; -import java.util.HashMap; import java.util.Map; import net.bytebuddy.asm.Advice; import net.bytebuddy.description.method.MethodDescription; @@ -24,14 +24,12 @@ import org.hibernate.engine.HibernateIterator; public class IteratorInstrumentation extends Instrumenter.Default { public IteratorInstrumentation() { - super("hibernate"); + super(INSTRUMENTATION_NAME); } @Override public Map contextStore() { - final Map map = new HashMap<>(); - map.put("org.hibernate.engine.HibernateIterator", SessionState.class.getName()); - return Collections.unmodifiableMap(map); + return singletonMap("org.hibernate.engine.HibernateIterator", SessionState.class.getName()); } @Override @@ -55,10 +53,8 @@ public class IteratorInstrumentation extends Instrumenter.Default { @Override public Map, String> transformers() { - final Map, String> transformers = new HashMap<>(); - transformers.put( + return singletonMap( isMethod().and(named("next").or(named("remove"))), IteratorAdvice.class.getName()); - return transformers; } public static class IteratorAdvice { @@ -76,7 +72,6 @@ public class IteratorInstrumentation extends Instrumenter.Default { @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) public static void endMethod( - @Advice.This final HibernateIterator iterator, @Advice.Enter final SessionState state, @Advice.Thrown final Throwable throwable, @Advice.Return(typing = Assigner.Typing.DYNAMIC) final Object entity) { diff --git a/dd-java-agent/instrumentation/hibernate/src/main/java/datadog/trace/instrumentation/hibernate/ProcedureCallInstrumentation.java b/dd-java-agent/instrumentation/hibernate/src/main/java/datadog/trace/instrumentation/hibernate/ProcedureCallInstrumentation.java index b28934d8a7..48fbfe0f2e 100644 --- a/dd-java-agent/instrumentation/hibernate/src/main/java/datadog/trace/instrumentation/hibernate/ProcedureCallInstrumentation.java +++ b/dd-java-agent/instrumentation/hibernate/src/main/java/datadog/trace/instrumentation/hibernate/ProcedureCallInstrumentation.java @@ -1,6 +1,8 @@ package datadog.trace.instrumentation.hibernate; import static datadog.trace.agent.tooling.ByteBuddyElementMatchers.safeHasSuperType; +import static datadog.trace.instrumentation.hibernate.HibernateInstrumentation.INSTRUMENTATION_NAME; +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; @@ -10,8 +12,6 @@ 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.Collections; -import java.util.HashMap; import java.util.Map; import net.bytebuddy.asm.Advice; import net.bytebuddy.description.method.MethodDescription; @@ -23,14 +23,12 @@ import org.hibernate.procedure.ProcedureCall; public class ProcedureCallInstrumentation extends Instrumenter.Default { public ProcedureCallInstrumentation() { - super("hibernate"); + super(INSTRUMENTATION_NAME); } @Override public Map contextStore() { - final Map map = new HashMap<>(); - map.put("org.hibernate.procedure.ProcedureCall", SessionState.class.getName()); - return Collections.unmodifiableMap(map); + return singletonMap("org.hibernate.procedure.ProcedureCall", SessionState.class.getName()); } @Override @@ -53,11 +51,8 @@ public class ProcedureCallInstrumentation extends Instrumenter.Default { @Override public Map, String> transformers() { - final Map, String> transformers = new HashMap<>(); - transformers.put( + return singletonMap( isMethod().and(named("getOutputs")), ProcedureCallMethodAdvice.class.getName()); - - return transformers; } public static class ProcedureCallMethodAdvice { diff --git a/dd-java-agent/instrumentation/hibernate/src/main/java/datadog/trace/instrumentation/hibernate/QueryInstrumentation.java b/dd-java-agent/instrumentation/hibernate/src/main/java/datadog/trace/instrumentation/hibernate/QueryInstrumentation.java index b0be87d1e9..196ada0459 100644 --- a/dd-java-agent/instrumentation/hibernate/src/main/java/datadog/trace/instrumentation/hibernate/QueryInstrumentation.java +++ b/dd-java-agent/instrumentation/hibernate/src/main/java/datadog/trace/instrumentation/hibernate/QueryInstrumentation.java @@ -2,6 +2,8 @@ package datadog.trace.instrumentation.hibernate; import static datadog.trace.agent.tooling.ByteBuddyElementMatchers.safeHasSuperType; import static datadog.trace.instrumentation.hibernate.HibernateDecorator.DECORATOR; +import static datadog.trace.instrumentation.hibernate.HibernateInstrumentation.INSTRUMENTATION_NAME; +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; @@ -11,8 +13,6 @@ 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.Collections; -import java.util.HashMap; import java.util.Map; import net.bytebuddy.asm.Advice; import net.bytebuddy.description.method.MethodDescription; @@ -26,14 +26,12 @@ import org.hibernate.SQLQuery; public class QueryInstrumentation extends Instrumenter.Default { public QueryInstrumentation() { - super("hibernate"); + super(INSTRUMENTATION_NAME); } @Override public Map contextStore() { - final Map map = new HashMap<>(); - map.put("org.hibernate.Query", SessionState.class.getName()); - return Collections.unmodifiableMap(map); + return singletonMap("org.hibernate.Query", SessionState.class.getName()); } @Override @@ -56,8 +54,7 @@ public class QueryInstrumentation extends Instrumenter.Default { @Override public Map, String> transformers() { - final Map, String> transformers = new HashMap<>(); - transformers.put( + return singletonMap( isMethod() .and( named("list") @@ -65,8 +62,6 @@ public class QueryInstrumentation extends Instrumenter.Default { .or(named("uniqueResult")) .or(named("scroll"))), QueryMethodAdvice.class.getName()); - - return transformers; } public static class QueryMethodAdvice { diff --git a/dd-java-agent/instrumentation/hibernate/src/main/java/datadog/trace/instrumentation/hibernate/SessionFactoryInstrumentation.java b/dd-java-agent/instrumentation/hibernate/src/main/java/datadog/trace/instrumentation/hibernate/SessionFactoryInstrumentation.java index 7db8c145d1..a32af064b3 100644 --- a/dd-java-agent/instrumentation/hibernate/src/main/java/datadog/trace/instrumentation/hibernate/SessionFactoryInstrumentation.java +++ b/dd-java-agent/instrumentation/hibernate/src/main/java/datadog/trace/instrumentation/hibernate/SessionFactoryInstrumentation.java @@ -2,6 +2,8 @@ package datadog.trace.instrumentation.hibernate; import static datadog.trace.agent.tooling.ByteBuddyElementMatchers.safeHasSuperType; import static datadog.trace.instrumentation.hibernate.HibernateDecorator.DECORATOR; +import static datadog.trace.instrumentation.hibernate.HibernateInstrumentation.INSTRUMENTATION_NAME; +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; @@ -15,8 +17,6 @@ import datadog.trace.bootstrap.ContextStore; import datadog.trace.bootstrap.InstrumentationContext; import io.opentracing.Span; import io.opentracing.util.GlobalTracer; -import java.util.Collections; -import java.util.HashMap; import java.util.Map; import net.bytebuddy.asm.Advice; import net.bytebuddy.description.method.MethodDescription; @@ -28,14 +28,12 @@ import org.hibernate.SharedSessionContract; public class SessionFactoryInstrumentation extends Instrumenter.Default { public SessionFactoryInstrumentation() { - super("hibernate"); + super(INSTRUMENTATION_NAME); } @Override public Map contextStore() { - final Map map = new HashMap<>(); - map.put("org.hibernate.SharedSessionContract", SessionState.class.getName()); - return Collections.unmodifiableMap(map); + return singletonMap("org.hibernate.SharedSessionContract", SessionState.class.getName()); } @Override @@ -57,10 +55,7 @@ public class SessionFactoryInstrumentation extends Instrumenter.Default { @Override public Map, String> transformers() { - final Map, String> transformers = new HashMap<>(); - // Session lifecycle. A span will cover openSession->Session.close, but no scope will be - // generated. - transformers.put( + return singletonMap( isMethod() .and(named("openSession").or(named("openStatelessSession"))) .and(takesArguments(0)) @@ -68,8 +63,6 @@ public class SessionFactoryInstrumentation extends Instrumenter.Default { returns( named("org.hibernate.Session").or(named("org.hibernate.StatelessSession")))), SessionFactoryAdvice.class.getName()); - - return transformers; } public static class SessionFactoryAdvice { diff --git a/dd-java-agent/instrumentation/hibernate/src/main/java/datadog/trace/instrumentation/hibernate/SessionInstrumentation.java b/dd-java-agent/instrumentation/hibernate/src/main/java/datadog/trace/instrumentation/hibernate/SessionInstrumentation.java index 577ddc28e4..2886c45b88 100644 --- a/dd-java-agent/instrumentation/hibernate/src/main/java/datadog/trace/instrumentation/hibernate/SessionInstrumentation.java +++ b/dd-java-agent/instrumentation/hibernate/src/main/java/datadog/trace/instrumentation/hibernate/SessionInstrumentation.java @@ -2,6 +2,7 @@ package datadog.trace.instrumentation.hibernate; import static datadog.trace.agent.tooling.ByteBuddyElementMatchers.safeHasSuperType; import static datadog.trace.instrumentation.hibernate.HibernateDecorator.DECORATOR; +import static datadog.trace.instrumentation.hibernate.HibernateInstrumentation.INSTRUMENTATION_NAME; import static net.bytebuddy.matcher.ElementMatchers.isInterface; import static net.bytebuddy.matcher.ElementMatchers.isMethod; import static net.bytebuddy.matcher.ElementMatchers.named; @@ -34,7 +35,7 @@ import org.hibernate.procedure.ProcedureCall; public class SessionInstrumentation extends Instrumenter.Default { public SessionInstrumentation() { - super("hibernate"); + super(INSTRUMENTATION_NAME); } @Override @@ -93,7 +94,7 @@ public class SessionInstrumentation extends Instrumenter.Default { .or(named("immediateLoad")) .or(named("internalLoad"))), SessionMethodAdvice.class.getName()); - // Handle the generic and non-generic 'get' separately. + // Handle the non-generic 'get' separately. transformers.put( isMethod() .and(named("get")) @@ -106,7 +107,6 @@ public class SessionInstrumentation extends Instrumenter.Default { transformers.put( isMethod() .and(named("beginTransaction").or(named("getTransaction"))) - .and(takesArguments(0)) .and(returns(named("org.hibernate.Transaction"))), GetTransactionAdvice.class.getName()); @@ -139,7 +139,7 @@ public class SessionInstrumentation extends Instrumenter.Default { return; } if (state.getMethodScope() != null) { - System.err.println("THIS IS WRONG"); // TODO: proper warning/logging. + state.getMethodScope().close(); } final Span span = state.getSessionSpan(); diff --git a/dd-java-agent/instrumentation/hibernate/src/main/java/datadog/trace/instrumentation/hibernate/TransactionInstrumentation.java b/dd-java-agent/instrumentation/hibernate/src/main/java/datadog/trace/instrumentation/hibernate/TransactionInstrumentation.java index 30be85d733..b458c8c0ca 100644 --- a/dd-java-agent/instrumentation/hibernate/src/main/java/datadog/trace/instrumentation/hibernate/TransactionInstrumentation.java +++ b/dd-java-agent/instrumentation/hibernate/src/main/java/datadog/trace/instrumentation/hibernate/TransactionInstrumentation.java @@ -1,7 +1,8 @@ package datadog.trace.instrumentation.hibernate; import static datadog.trace.agent.tooling.ByteBuddyElementMatchers.safeHasSuperType; -import static net.bytebuddy.matcher.ElementMatchers.isDeclaredBy; +import static datadog.trace.instrumentation.hibernate.HibernateInstrumentation.INSTRUMENTATION_NAME; +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; @@ -12,8 +13,6 @@ 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.Collections; -import java.util.HashMap; import java.util.Map; import net.bytebuddy.asm.Advice; import net.bytebuddy.description.method.MethodDescription; @@ -25,14 +24,12 @@ import org.hibernate.Transaction; public class TransactionInstrumentation extends Instrumenter.Default { public TransactionInstrumentation() { - super("hibernate"); + super(INSTRUMENTATION_NAME); } @Override public Map contextStore() { - final Map map = new HashMap<>(); - map.put("org.hibernate.Transaction", SessionState.class.getName()); - return Collections.unmodifiableMap(map); + return singletonMap("org.hibernate.Transaction", SessionState.class.getName()); } @Override @@ -55,14 +52,9 @@ public class TransactionInstrumentation extends Instrumenter.Default { @Override public Map, String> transformers() { - final Map, String> transformers = new HashMap<>(); - transformers.put( - isMethod() - .and(named("commit")) - .and(isDeclaredBy(safeHasSuperType(named("org.hibernate.Transaction")))) - .and(takesArguments(0)), + return singletonMap( + isMethod().and(named("commit")).and(takesArguments(0)), TransactionCommitAdvice.class.getName()); - return transformers; } public static class TransactionCommitAdvice { diff --git a/dd-java-agent/instrumentation/hibernate/src/test/groovy/QueryTest.groovy b/dd-java-agent/instrumentation/hibernate/src/test/groovy/QueryTest.groovy index 806923b46d..4cbac8ee80 100644 --- a/dd-java-agent/instrumentation/hibernate/src/test/groovy/QueryTest.groovy +++ b/dd-java-agent/instrumentation/hibernate/src/test/groovy/QueryTest.groovy @@ -9,14 +9,23 @@ class QueryTest extends AbstractHibernateTest { def "test hibernate query.#queryMethodName single call"() { setup: + // With Transaction Session session = sessionFactory.openSession() session.beginTransaction() queryInteraction(session) session.getTransaction().commit() session.close() + // Without Transaction + if (!requiresTransaction) { + session = sessionFactory.openSession() + queryInteraction(session) + session.close() + } + expect: - assertTraces(1) { + assertTraces(requiresTransaction ? 1 : 2) { + // With Transaction trace(0, 4) { span(0) { serviceName "hibernate" @@ -62,27 +71,62 @@ class QueryTest extends AbstractHibernateTest { childOf span(2) } } + if (!requiresTransaction) { + // Without Transaction + trace(1, 3) { + span(0) { + serviceName "hibernate" + resourceName "hibernate.session" + operationName "hibernate.session" + spanType DDSpanTypes.HIBERNATE + parent() + tags { + "$Tags.COMPONENT.key" "java-hibernate" + "$Tags.SPAN_KIND.key" Tags.SPAN_KIND_CLIENT + "$DDTags.SPAN_TYPE" DDSpanTypes.HIBERNATE + defaultTags() + } + } + span(1) { + serviceName "hibernate" + resourceName "$resource" + operationName "hibernate.$queryMethodName" + 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(2) { + serviceName "h2" + childOf span(1) + } + } + } } where: - queryMethodName | isError | resource | queryInteraction - "query.list" | false | "Value" | { sess -> + queryMethodName | resource | requiresTransaction | queryInteraction + "query.list" | "Value" | false | { sess -> Query q = sess.createQuery("from Value") q.list() } - "query.executeUpdate" | false | "update Value set name = 'alyx'" | { sess -> + "query.executeUpdate" | "update Value set name = 'alyx'" | true | { sess -> Query q = sess.createQuery("update Value set name = 'alyx'") q.executeUpdate() } - "query.uniqueResult" | false | "Value" | { sess -> + "query.uniqueResult" | "Value" | false | { sess -> Query q = sess.createQuery("from Value where id = 1") q.uniqueResult() } - "iterate" | false | "from Value" | { sess -> + "iterate" | "from Value" | false | { sess -> Query q = sess.createQuery("from Value") q.iterate() } - "query.scroll" | false | "from Value" | { sess -> + "query.scroll" | "from Value" | false | { sess -> Query q = sess.createQuery("from Value") q.scroll() } diff --git a/dd-java-agent/instrumentation/hibernate/src/test/groovy/SessionTest.groovy b/dd-java-agent/instrumentation/hibernate/src/test/groovy/SessionTest.groovy index 5746a00513..c7febcb552 100644 --- a/dd-java-agent/instrumentation/hibernate/src/test/groovy/SessionTest.groovy +++ b/dd-java-agent/instrumentation/hibernate/src/test/groovy/SessionTest.groovy @@ -1,6 +1,9 @@ import datadog.trace.api.DDSpanTypes import datadog.trace.api.DDTags +import io.opentracing.Scope +import io.opentracing.Tracer import io.opentracing.tag.Tags +import io.opentracing.util.GlobalTracer import org.hibernate.* import spock.lang.Shared @@ -72,13 +75,7 @@ class SessionTest extends AbstractHibernateTest { operationName "hibernate.$methodName" spanType DDSpanTypes.HIBERNATE childOf span(0) - if (isError) { - errored true - } tags { - if (isError) { - errorTags(MappingException, "Unknown entity: java.lang.Long") - } "$Tags.COMPONENT.key" "java-hibernate" "$Tags.SPAN_KIND.key" Tags.SPAN_KIND_CLIENT "$DDTags.SPAN_TYPE" DDSpanTypes.HIBERNATE @@ -94,28 +91,28 @@ class SessionTest extends AbstractHibernateTest { } where: - testName | methodName | resource | isError | sessionImplementations | sessionMethodTest - "lock" | "lock" | "Value" | false | ["Session"] | { sesh, val -> + testName | methodName | resource | sessionImplementations | sessionMethodTest + "lock" | "lock" | "Value" | ["Session"] | { sesh, val -> sesh.lock(val, LockMode.READ) } - "refresh" | "refresh" | "Value" | false | ["Session", "StatelessSession"] | { sesh, val -> + "refresh" | "refresh" | "Value" | ["Session", "StatelessSession"] | { sesh, val -> sesh.refresh(val) } - "get" | "get" | "Value" | false | ["Session", "StatelessSession"] | { sesh, val -> + "get" | "get" | "Value" | ["Session", "StatelessSession"] | { sesh, val -> sesh.get("Value", val.getId()) } - "insert" | "insert" | "Value" | false | ["StatelessSession"] | { sesh, val -> + "insert" | "insert" | "Value" | ["StatelessSession"] | { sesh, val -> sesh.insert("Value", new Value("insert me")) } - "update (StatelessSession)" | "update" | "Value" | false | ["StatelessSession"] | { sesh, val -> + "update (StatelessSession)" | "update" | "Value" | ["StatelessSession"] | { sesh, val -> val.setName("New name") sesh.update(val) } - "update by entityName (StatelessSession)" | "update" | "Value" | false | ["StatelessSession"] | { sesh, val -> + "update by entityName (StatelessSession)" | "update" | "Value" | ["StatelessSession"] | { sesh, val -> val.setName("New name") sesh.update("Value", val) } - "delete (Session)" | "delete" | "Value" | false | ["StatelessSession"] | { sesh, val -> + "delete (Session)" | "delete" | "Value" | ["StatelessSession"] | { sesh, val -> sesh.delete(val) } } @@ -175,13 +172,7 @@ class SessionTest extends AbstractHibernateTest { operationName "hibernate.$methodName" spanType DDSpanTypes.HIBERNATE childOf span(0) - if (isError) { - errored true - } tags { - if (isError) { - errorTags(MappingException, "Unknown entity: java.lang.Long") - } "$Tags.COMPONENT.key" "java-hibernate" "$Tags.SPAN_KIND.key" Tags.SPAN_KIND_CLIENT "$DDTags.SPAN_TYPE" DDSpanTypes.HIBERNATE @@ -197,13 +188,13 @@ class SessionTest extends AbstractHibernateTest { } where: - testName | methodName | resource | isError | sessionMethodTest - "replicate" | "replicate" | "Value" | false | { sesh, val -> + testName | methodName | resource | sessionMethodTest + "replicate" | "replicate" | "Value" | { sesh, val -> Value replicated = new Value(val.getName() + " replicated") replicated.setId(val.getId()) sesh.replicate(replicated, ReplicationMode.OVERWRITE) } - "replicate by entityName" | "replicate" | "Value" | false | { sesh, val -> + "replicate by entityName" | "replicate" | "Value" | { sesh, val -> Value replicated = new Value(val.getName() + " replicated") replicated.setId(val.getId()) sesh.replicate("Value", replicated, ReplicationMode.OVERWRITE) @@ -334,13 +325,7 @@ class SessionTest extends AbstractHibernateTest { operationName "hibernate.$methodName" spanType DDSpanTypes.HIBERNATE childOf span(0) - if (isError) { - errored true - } tags { - if (isError) { - errorTags(MappingException, "Unknown entity: java.lang.Long") - } "$Tags.COMPONENT.key" "java-hibernate" "$Tags.SPAN_KIND.key" Tags.SPAN_KIND_CLIENT "$DDTags.SPAN_TYPE" DDSpanTypes.HIBERNATE @@ -352,32 +337,32 @@ class SessionTest extends AbstractHibernateTest { } where: - testName | methodName | resource | isError | sessionImplementations | sessionMethodTest - "save" | "save" | "Value" | false | ["Session"] | { sesh, val -> + testName | methodName | resource | sessionImplementations | sessionMethodTest + "save" | "save" | "Value" | ["Session"] | { sesh, val -> sesh.save(new Value("Another value")) } - "saveOrUpdate save" | "saveOrUpdate" | "Value" | false | ["Session"] | { sesh, val -> + "saveOrUpdate save" | "saveOrUpdate" | "Value" | ["Session"] | { sesh, val -> sesh.saveOrUpdate(new Value("Value")) } - "saveOrUpdate update" | "saveOrUpdate" | "Value" | false | ["Session"] | { sesh, val -> + "saveOrUpdate update" | "saveOrUpdate" | "Value" | ["Session"] | { sesh, val -> val.setName("New name") sesh.saveOrUpdate(val) } - "merge" | "merge" | "Value" | false | ["Session"] | { sesh, val -> + "merge" | "merge" | "Value" | ["Session"] | { sesh, val -> sesh.merge(new Value("merge me in")) } - "persist" | "persist" | "Value" | false | ["Session"] | { sesh, val -> + "persist" | "persist" | "Value" | ["Session"] | { sesh, val -> sesh.persist(new Value("merge me in")) } - "update (Session)" | "update" | "Value" | false | ["Session"] | { sesh, val -> + "update (Session)" | "update" | "Value" | ["Session"] | { sesh, val -> val.setName("New name") sesh.update(val) } - "update by entityName (Session)" | "update" | "Value" | false | ["Session"] | { sesh, val -> + "update by entityName (Session)" | "update" | "Value" | ["Session"] | { sesh, val -> val.setName("New name") sesh.update("Value", val) } - "delete (Session)" | "delete" | "Value" | false | ["Session"] | { sesh, val -> + "delete (Session)" | "delete" | "Value" | ["Session"] | { sesh, val -> sesh.delete(val) } } @@ -447,5 +432,159 @@ class SessionTest extends AbstractHibernateTest { "getNamedQuery" | "Value" | { sess -> sess.getNamedQuery("TestNamedQuery") } "createSQLQuery" | "SELECT * FROM Value" | { sess -> sess.createSQLQuery("SELECT * FROM Value") } } + + + def "test hibernate overlapping Sessions"() { + setup: + + Tracer tracer = GlobalTracer.get() + + Scope scope = tracer.buildSpan("overlapping Sessions").startActive(true) + + def session1 = sessionFactory.openSession() + session1.beginTransaction() + def session2 = sessionFactory.openStatelessSession() + def session3 = sessionFactory.openSession() + + def value1 = new Value("Value 1") + session1.save(value1) + session2.insert(new Value("Value 2")) + session3.save(new Value("Value 3")) + session1.delete(value1) + + session2.close() + session1.getTransaction().commit() + session1.close() + session3.close() + + scope.close(); + + + expect: + assertTraces(1) { + trace(0, 12) { + span(0) { + serviceName "unnamed-java-app" + operationName "overlapping Sessions" + } + span(1) { + serviceName "hibernate" + resourceName "hibernate.session" + operationName "hibernate.session" + 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(2) { + serviceName "hibernate" + resourceName "hibernate.session" + operationName "hibernate.session" + 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.transaction.commit" + operationName "hibernate.transaction.commit" + spanType DDSpanTypes.HIBERNATE + childOf span(2) + tags { + "$Tags.COMPONENT.key" "java-hibernate" + "$Tags.SPAN_KIND.key" Tags.SPAN_KIND_CLIENT + "$DDTags.SPAN_TYPE" DDSpanTypes.HIBERNATE + defaultTags() + } + } + span(4) { + serviceName "h2" + childOf span(3) + } + span(5) { + serviceName "h2" + childOf span(3) + } + span(6) { + serviceName "hibernate" + resourceName "hibernate.session" + operationName "hibernate.session" + 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(7) { + serviceName "hibernate" + resourceName "Value" + operationName "hibernate.delete" + spanType DDSpanTypes.HIBERNATE + childOf span(2) + tags { + "$Tags.COMPONENT.key" "java-hibernate" + "$Tags.SPAN_KIND.key" Tags.SPAN_KIND_CLIENT + "$DDTags.SPAN_TYPE" DDSpanTypes.HIBERNATE + defaultTags() + } + } + span(8) { + serviceName "hibernate" + resourceName "Value" + operationName "hibernate.save" + spanType DDSpanTypes.HIBERNATE + childOf span(1) + tags { + "$Tags.COMPONENT.key" "java-hibernate" + "$Tags.SPAN_KIND.key" Tags.SPAN_KIND_CLIENT + "$DDTags.SPAN_TYPE" DDSpanTypes.HIBERNATE + defaultTags() + } + } + span(9) { + serviceName "hibernate" + resourceName "Value" + operationName "hibernate.insert" + spanType DDSpanTypes.HIBERNATE + childOf span(6) + tags { + "$Tags.COMPONENT.key" "java-hibernate" + "$Tags.SPAN_KIND.key" Tags.SPAN_KIND_CLIENT + "$DDTags.SPAN_TYPE" DDSpanTypes.HIBERNATE + defaultTags() + } + } + span(10) { + serviceName "h2" + childOf span(9) + } + span(11) { + serviceName "hibernate" + resourceName "Value" + operationName "hibernate.save" + spanType DDSpanTypes.HIBERNATE + childOf span(2) + tags { + "$Tags.COMPONENT.key" "java-hibernate" + "$Tags.SPAN_KIND.key" Tags.SPAN_KIND_CLIENT + "$DDTags.SPAN_TYPE" DDSpanTypes.HIBERNATE + defaultTags() + } + } + } + } + } } diff --git a/dd-java-agent/instrumentation/hibernate/src/test/resources/hibernate.cfg.xml b/dd-java-agent/instrumentation/hibernate/src/test/resources/hibernate.cfg.xml index 5b20390ba9..fe5e5de6b1 100644 --- a/dd-java-agent/instrumentation/hibernate/src/test/resources/hibernate.cfg.xml +++ b/dd-java-agent/instrumentation/hibernate/src/test/resources/hibernate.cfg.xml @@ -14,7 +14,7 @@ org.hibernate.dialect.H2Dialect - 1 + 3 org.hibernate.cache.internal.NoCacheProvider true