opentelemetry-java-instrumentation
opentelemetry-java-instrumentation copied to clipboard
make hibernate-* indy compatible
Part of #11457
- needs to have a common classloader for all
hibernateinstrumentation modules - is using
@Advice.Local
Introduces HibernateOperationScope class to carry the state between enter and exit advice (and also helps to reduce a bit code duplication).
"indy compatible" is defined in https://github.com/open-telemetry/opentelemetry-java-instrumentation/pull/11546/files#diff-c01c02c7d6cde11986ed4c95f749da6318b0deb89c371d866319371cc1757bd0
This PR needs to have the indy test label added for validation in CI.
Local validation can be done with gradle test -PtestIndy=true
@trask can you add test indy label to this PR ?
closing and re-opening to trigger indy tests
I think you should split this PR in 2. The first one should just do the changes that are needed to make the instrumentation work with indy like adding the getModuleGroup (assuming this all that is needed for that). This PR could be merged immediately. In the second PR you could explore the options for rewriting the advice in a way that would work without inlining the advice, like removing @Advice.Local and erasing the type for @Advice.Enter. This PR will probably need more time as there will be cases where the indy version is clearly worse than what we have currently (for example the need to erase the type for @Advice.Enter). This will definitely raise questions like can we do something to get the type back? Can we rewrite the advice in a way where the loss of type doesn't matter. For example perhaps we should write the advice like
@Advice.OnMethodEnter(suppress = Throwable.class)
public static Consumer<Throwable> startMethod(@Advice.This ProcedureCall call, @Advice.Origin("#m") String name) {
return ProcedureCallMethodHelper.start(call, name);
}
@Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class)
public static void endMethod(@Advice.Thrown Throwable throwable, @Advice.Enter Consumer<Throwbale> exitAction) {
if (exitAction != null) {
exitAction.accept(throwable);
}
}
and move all the logic that we have in advice into the helper. The returned Consumer instance can have CallDepth as field so you won't need to add any casts.
I think you should split this PR in 2. The first one should just do the changes that are needed to make the instrumentation work with indy like adding the
getModuleGroup(assuming this all that is needed for that).
Unfortunately, I think we can't do that, as migrating to indy advices means removing the local variables.
I agree with you that there may be more elegant (and maybe generic) ways to do that, here I tried to minimize the changes to the strictly necessary.
I really like your idea to use a Consumer as a way to avoid the type cast, it could even be possible to do some object pooling in the helper to minimize allocation if needed.
On the long term, once using indy advices everywhere, there isn't any separation needed between helper classes and advice classes, so those helper classes could just be inner classes of the advice class if needed for simplicity.
When we did similar migration in the Elastic APM agent we did similar compromises on the code simplicity as we think the benefits (get rid of shading, being able to debug, isolated classloaders, ...) were definitely worth doing it, however here I completely understand that doing so might be harder to accept, especially given this work will take a while and many releases to be completed.
IMO, the Consumer approach for Advices feels a bit more complicated, though it preserves type safety. However this comes at the cost of allocation of which I'm not sure whether the JIT is capable of getting rid of. I'm pretty sure for plain Objects / Object-arrays those are allocation-free after JIT-inlining the Advice code on hot paths.
@laurit if you want to, I can have a try at implementing the approach suggested in this comment to allow references to any types visible to the Advice in it's method signatures if that is important to you:
- When inserting the invokedynamic, replace all unaccessible types with Object
- We store the orginial signature before erasure as a String in the constant pool and pass it to the invokedynamic bootstrap method
- Using this original signature, the invokedynamic bootstrapping method can lookup the MethodHandle for the Advice method and alter it to match the erased signature using the utilities in MethodHandle.asType.
EDIT:
Did a small microbenchmark to verify that MethodHandle.asType does not impact the JIT-ability to inline:
Benchmark code
@OutputTimeUnit(TimeUnit.NANOSECONDS)
@BenchmarkMode(Mode.AverageTime)
public class MethodHandleInlining {
private static final MethodHandle original = createMH();
private static final MethodHandle erased = original.asType(
MethodType.methodType(Object.class, Object.class));
private static MethodHandle createMH() {
try {
return MethodHandles.publicLookup()
.findStatic(MethodHandleInlining.class, "doStuff",
MethodType.methodType(Blackhole.class, Blackhole.class));
} catch (Exception e) {
throw new IllegalStateException(e);
}
}
@Benchmark
public void invokeNormal(Blackhole bh) {
bh.consume(doStuff(bh));
}
@Benchmark
public void invokeOriginalNotExact(Blackhole bh) {
try {
bh.consume(original.invoke((Object) bh));
} catch (Throwable e) {
throw new IllegalStateException(e);
}
bh.consume(doStuff(bh));
}
@Benchmark
public void invokeOriginalExact(Blackhole bh) {
try {
bh.consume((Blackhole) original.invokeExact(bh));
} catch (Throwable e) {
throw new IllegalStateException(e);
}
}
@Benchmark
public void invokeErasedExact(Blackhole bh) {
try {
bh.consume((Object) erased.invokeExact((Object) bh));
} catch (Throwable e) {
throw new IllegalStateException(e);
}
}
public static Blackhole doStuff(Blackhole bh) {
bh.consume(bh);
return bh;
}
}
Results on JDK 17:
Benchmark Mode Cnt Score Error Units
MethodHandleInlining.invokeErasedExact avgt 5 0,568 ± 0,001 ns/op
MethodHandleInlining.invokeNormal avgt 5 0,573 ± 0,027 ns/op
MethodHandleInlining.invokeOriginalExact avgt 5 0,569 ± 0,011 ns/op
MethodHandleInlining.invokeOriginalNotExact avgt 5 2,534 ± 0,019 ns/op
So, it seems like MethodHandle.asType is free after JIT. Therefore I'd propose to go that route.
Unfortunately, I think we can't do that, as migrating to indy advices means removing the local variables.
As far as I remember the advice transformer can rewrite @Advice.Local, so while we need to get rid of these eventually it shouldn't be strictly necessary to get tests running with indy.
IMO, the Consumer approach for Advices feels a bit more complicated, though it preserves type safety. However this comes at the cost of allocation of which I'm not sure whether the JIT is capable of getting rid of. I'm pretty sure for plain Objects / Object-arrays those are allocation-free after JIT-inlining the Advice code on hot paths.
Creating a span and exporting it does a lot of things. Adding a few more allocations there isn't really going to change anything. Also many of the existing advices already allocate some objects so I wouldn't worry about this too much. As for the JIT I'd guess whether it can stack allocate something will really depend on whether it can prove with escape analysis whether the object is used only in this method or not. In that regard it should not matter whether it is an object array or something else. What matters is whether this object gets passed to some other method and whether that method gets inlined.
@laurit if you want to, I can have a try at implementing the approach suggested in https://github.com/open-telemetry/opentelemetry-java-instrumentation/pull/11559#discussion_r1669872466 to allow references to any types visible to the Advice in it's method signatures if that is important to you:
I think this could improve the readability of the advice code. It would be awesome if you could look into this.
Thanks to the work of @JonasKunz in https://github.com/open-telemetry/opentelemetry-java-instrumentation/pull/11873 we can have better advice signatures without Object nor casts, so I have updated this PR accordingly.
As a consequence, this PR is now mostly about removing usages of Advice.Local, @laurit could you take a second look at it ?
@SylvainJuge I wanted to create a PR for your PR that does this a bit differently, but with your last changes it won't apply cleanly and I don't have time to fix it so I'll just paste the diff here for an alternative idea how you could have solved this (I didn't run all tests so could be buggy).
diff --git a/instrumentation/hibernate/hibernate-3.3/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/hibernate/v3_3/CriteriaInstrumentation.java b/instrumentation/hibernate/hibernate-3.3/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/hibernate/v3_3/CriteriaInstrumentation.java
index 80e57dd549..0fd07613db 100644
--- a/instrumentation/hibernate/hibernate-3.3/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/hibernate/v3_3/CriteriaInstrumentation.java
+++ b/instrumentation/hibernate/hibernate-3.3/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/hibernate/v3_3/CriteriaInstrumentation.java
@@ -14,7 +14,6 @@ import static net.bytebuddy.matcher.ElementMatchers.namedOneOf;
import io.opentelemetry.context.Context;
import io.opentelemetry.instrumentation.api.util.VirtualField;
-import io.opentelemetry.javaagent.bootstrap.CallDepth;
import io.opentelemetry.javaagent.bootstrap.Java8BytecodeBridge;
import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer;
@@ -53,9 +52,8 @@ public class CriteriaInstrumentation implements TypeInstrumentation {
public static HibernateOperationScope startMethod(
@Advice.This Criteria criteria, @Advice.Origin("#m") String name) {
- CallDepth callDepth = CallDepth.forClass(HibernateOperation.class);
- if (callDepth.getAndIncrement() > 0) {
- return HibernateOperationScope.wrapCallDepth(callDepth);
+ if (HibernateOperationScope.isStarted()) {
+ return null;
}
String entityName = null;
@@ -70,19 +68,15 @@ public class CriteriaInstrumentation implements TypeInstrumentation {
Context parentContext = Java8BytecodeBridge.currentContext();
HibernateOperation hibernateOperation =
new HibernateOperation("Criteria." + name, entityName, sessionInfo);
- if (!instrumenter().shouldStart(parentContext, hibernateOperation)) {
- return HibernateOperationScope.wrapCallDepth(callDepth);
- }
- return HibernateOperationScope.startNew(
- callDepth, hibernateOperation, parentContext, instrumenter());
+ return HibernateOperationScope.start(hibernateOperation, parentContext, instrumenter());
}
@Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class)
public static void endMethod(
- @Advice.Thrown Throwable throwable, @Advice.Enter HibernateOperationScope enterScope) {
+ @Advice.Thrown Throwable throwable, @Advice.Enter HibernateOperationScope scope) {
- HibernateOperationScope.end(enterScope, instrumenter(), throwable);
+ HibernateOperationScope.end(scope, throwable);
}
}
}
diff --git a/instrumentation/hibernate/hibernate-3.3/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/hibernate/v3_3/QueryInstrumentation.java b/instrumentation/hibernate/hibernate-3.3/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/hibernate/v3_3/QueryInstrumentation.java
index 2a72cd5126..d4c170f191 100644
--- a/instrumentation/hibernate/hibernate-3.3/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/hibernate/v3_3/QueryInstrumentation.java
+++ b/instrumentation/hibernate/hibernate-3.3/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/hibernate/v3_3/QueryInstrumentation.java
@@ -15,7 +15,6 @@ import static net.bytebuddy.matcher.ElementMatchers.namedOneOf;
import io.opentelemetry.context.Context;
import io.opentelemetry.instrumentation.api.util.VirtualField;
-import io.opentelemetry.javaagent.bootstrap.CallDepth;
import io.opentelemetry.javaagent.bootstrap.Java8BytecodeBridge;
import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer;
@@ -52,9 +51,8 @@ public class QueryInstrumentation implements TypeInstrumentation {
@Advice.OnMethodEnter(suppress = Throwable.class)
public static HibernateOperationScope startMethod(@Advice.This Query query) {
- CallDepth callDepth = CallDepth.forClass(HibernateOperation.class);
- if (callDepth.getAndIncrement() > 0) {
- return HibernateOperationScope.wrapCallDepth(callDepth);
+ if (HibernateOperationScope.isStarted()) {
+ return null;
}
VirtualField<Query, SessionInfo> queryVirtualField =
@@ -64,19 +62,15 @@ public class QueryInstrumentation implements TypeInstrumentation {
Context parentContext = Java8BytecodeBridge.currentContext();
HibernateOperation hibernateOperation =
new HibernateOperation(getOperationNameForQuery(query.getQueryString()), sessionInfo);
- if (!instrumenter().shouldStart(parentContext, hibernateOperation)) {
- return HibernateOperationScope.wrapCallDepth(callDepth);
- }
- return HibernateOperationScope.startNew(
- callDepth, hibernateOperation, parentContext, instrumenter());
+ return HibernateOperationScope.start(hibernateOperation, parentContext, instrumenter());
}
@Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class)
public static void endMethod(
- @Advice.Thrown Throwable throwable, @Advice.Enter HibernateOperationScope enterScope) {
+ @Advice.Thrown Throwable throwable, @Advice.Enter HibernateOperationScope scope) {
- HibernateOperationScope.end(enterScope, instrumenter(), throwable);
+ HibernateOperationScope.end(scope, throwable);
}
}
}
diff --git a/instrumentation/hibernate/hibernate-3.3/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/hibernate/v3_3/SessionInstrumentation.java b/instrumentation/hibernate/hibernate-3.3/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/hibernate/v3_3/SessionInstrumentation.java
index b6ce66a58a..b7595ece72 100644
--- a/instrumentation/hibernate/hibernate-3.3/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/hibernate/v3_3/SessionInstrumentation.java
+++ b/instrumentation/hibernate/hibernate-3.3/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/hibernate/v3_3/SessionInstrumentation.java
@@ -19,7 +19,6 @@ import static net.bytebuddy.matcher.ElementMatchers.takesArgument;
import io.opentelemetry.context.Context;
import io.opentelemetry.instrumentation.api.util.VirtualField;
-import io.opentelemetry.javaagent.bootstrap.CallDepth;
import io.opentelemetry.javaagent.bootstrap.Java8BytecodeBridge;
import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer;
@@ -99,9 +98,8 @@ public class SessionInstrumentation implements TypeInstrumentation {
@Advice.Argument(0) Object arg0,
@Advice.Argument(value = 1, optional = true) Object arg1) {
- CallDepth callDepth = CallDepth.forClass(HibernateOperation.class);
- if (callDepth.getAndIncrement() > 0) {
- return HibernateOperationScope.wrapCallDepth(callDepth);
+ if (HibernateOperationScope.isStarted()) {
+ return null;
}
Context parentContext = Java8BytecodeBridge.currentContext();
@@ -110,19 +108,15 @@ public class SessionInstrumentation implements TypeInstrumentation {
getEntityName(descriptor, arg0, arg1, EntityNameUtil.bestGuessEntityName(session));
HibernateOperation hibernateOperation =
new HibernateOperation(getSessionMethodOperationName(name), entityName, sessionInfo);
- if (!instrumenter().shouldStart(parentContext, hibernateOperation)) {
- return HibernateOperationScope.wrapCallDepth(callDepth);
- }
- return HibernateOperationScope.startNew(
- callDepth, hibernateOperation, parentContext, instrumenter());
+ return HibernateOperationScope.start(hibernateOperation, parentContext, instrumenter());
}
@Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class)
public static void endMethod(
- @Advice.Thrown Throwable throwable, @Advice.Enter HibernateOperationScope enterScope) {
+ @Advice.Thrown Throwable throwable, @Advice.Enter HibernateOperationScope scope) {
- HibernateOperationScope.end(enterScope, instrumenter(), throwable);
+ HibernateOperationScope.end(scope, throwable);
}
}
diff --git a/instrumentation/hibernate/hibernate-3.3/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/hibernate/v3_3/TransactionInstrumentation.java b/instrumentation/hibernate/hibernate-3.3/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/hibernate/v3_3/TransactionInstrumentation.java
index 52162e48b4..91e83fae8c 100644
--- a/instrumentation/hibernate/hibernate-3.3/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/hibernate/v3_3/TransactionInstrumentation.java
+++ b/instrumentation/hibernate/hibernate-3.3/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/hibernate/v3_3/TransactionInstrumentation.java
@@ -14,7 +14,6 @@ import static net.bytebuddy.matcher.ElementMatchers.takesArguments;
import io.opentelemetry.context.Context;
import io.opentelemetry.instrumentation.api.util.VirtualField;
-import io.opentelemetry.javaagent.bootstrap.CallDepth;
import io.opentelemetry.javaagent.bootstrap.Java8BytecodeBridge;
import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer;
@@ -51,9 +50,8 @@ public class TransactionInstrumentation implements TypeInstrumentation {
@Advice.OnMethodEnter(suppress = Throwable.class)
public static HibernateOperationScope startCommit(@Advice.This Transaction transaction) {
- CallDepth callDepth = CallDepth.forClass(HibernateOperation.class);
- if (callDepth.getAndIncrement() > 0) {
- return HibernateOperationScope.wrapCallDepth(callDepth);
+ if (HibernateOperationScope.isStarted()) {
+ return null;
}
VirtualField<Transaction, SessionInfo> transactionVirtualField =
@@ -63,19 +61,15 @@ public class TransactionInstrumentation implements TypeInstrumentation {
Context parentContext = Java8BytecodeBridge.currentContext();
HibernateOperation hibernateOperation =
new HibernateOperation("Transaction.commit", sessionInfo);
- if (!instrumenter().shouldStart(parentContext, hibernateOperation)) {
- return HibernateOperationScope.wrapCallDepth(callDepth);
- }
- return HibernateOperationScope.startNew(
- callDepth, hibernateOperation, parentContext, instrumenter());
+ return HibernateOperationScope.start(hibernateOperation, parentContext, instrumenter());
}
@Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class)
public static void endCommit(
- @Advice.Thrown Throwable throwable, @Advice.Enter HibernateOperationScope enterScope) {
+ @Advice.Thrown Throwable throwable, @Advice.Enter HibernateOperationScope scope) {
- HibernateOperationScope.end(enterScope, instrumenter(), throwable);
+ HibernateOperationScope.end(scope, throwable);
}
}
}
diff --git a/instrumentation/hibernate/hibernate-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/hibernate/v4_0/CriteriaInstrumentation.java b/instrumentation/hibernate/hibernate-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/hibernate/v4_0/CriteriaInstrumentation.java
index db8d4d379b..ecfe47211c 100644
--- a/instrumentation/hibernate/hibernate-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/hibernate/v4_0/CriteriaInstrumentation.java
+++ b/instrumentation/hibernate/hibernate-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/hibernate/v4_0/CriteriaInstrumentation.java
@@ -14,7 +14,6 @@ import static net.bytebuddy.matcher.ElementMatchers.namedOneOf;
import io.opentelemetry.context.Context;
import io.opentelemetry.instrumentation.api.util.VirtualField;
-import io.opentelemetry.javaagent.bootstrap.CallDepth;
import io.opentelemetry.javaagent.bootstrap.Java8BytecodeBridge;
import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer;
@@ -53,9 +52,8 @@ public class CriteriaInstrumentation implements TypeInstrumentation {
public static HibernateOperationScope startMethod(
@Advice.This Criteria criteria, @Advice.Origin("#m") String name) {
- CallDepth callDepth = CallDepth.forClass(HibernateOperation.class);
- if (callDepth.getAndIncrement() > 0) {
- return HibernateOperationScope.wrapCallDepth(callDepth);
+ if (HibernateOperationScope.isStarted()) {
+ return null;
}
String entityName = null;
@@ -70,19 +68,15 @@ public class CriteriaInstrumentation implements TypeInstrumentation {
Context parentContext = Java8BytecodeBridge.currentContext();
HibernateOperation hibernateOperation =
new HibernateOperation("Criteria." + name, entityName, sessionInfo);
- if (!instrumenter().shouldStart(parentContext, hibernateOperation)) {
- return HibernateOperationScope.wrapCallDepth(callDepth);
- }
- return HibernateOperationScope.startNew(
- callDepth, hibernateOperation, parentContext, instrumenter());
+ return HibernateOperationScope.start(hibernateOperation, parentContext, instrumenter());
}
@Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class)
public static void endMethod(
- @Advice.Thrown Throwable throwable, @Advice.Enter HibernateOperationScope enterScope) {
+ @Advice.Thrown Throwable throwable, @Advice.Enter HibernateOperationScope scope) {
- HibernateOperationScope.end(enterScope, instrumenter(), throwable);
+ HibernateOperationScope.end(scope, throwable);
}
}
}
diff --git a/instrumentation/hibernate/hibernate-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/hibernate/v4_0/QueryInstrumentation.java b/instrumentation/hibernate/hibernate-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/hibernate/v4_0/QueryInstrumentation.java
index 5a5e86ebfd..f79f021441 100644
--- a/instrumentation/hibernate/hibernate-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/hibernate/v4_0/QueryInstrumentation.java
+++ b/instrumentation/hibernate/hibernate-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/hibernate/v4_0/QueryInstrumentation.java
@@ -15,7 +15,6 @@ import static net.bytebuddy.matcher.ElementMatchers.namedOneOf;
import io.opentelemetry.context.Context;
import io.opentelemetry.instrumentation.api.util.VirtualField;
-import io.opentelemetry.javaagent.bootstrap.CallDepth;
import io.opentelemetry.javaagent.bootstrap.Java8BytecodeBridge;
import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer;
@@ -52,9 +51,8 @@ public class QueryInstrumentation implements TypeInstrumentation {
@Advice.OnMethodEnter(suppress = Throwable.class)
public static HibernateOperationScope startMethod(@Advice.This Query query) {
- CallDepth callDepth = CallDepth.forClass(HibernateOperation.class);
- if (callDepth.getAndIncrement() > 0) {
- return HibernateOperationScope.wrapCallDepth(callDepth);
+ if (HibernateOperationScope.isStarted()) {
+ return null;
}
VirtualField<Query, SessionInfo> queryVirtualField =
@@ -64,19 +62,15 @@ public class QueryInstrumentation implements TypeInstrumentation {
Context parentContext = Java8BytecodeBridge.currentContext();
HibernateOperation hibernateOperation =
new HibernateOperation(getOperationNameForQuery(query.getQueryString()), sessionInfo);
- if (!instrumenter().shouldStart(parentContext, hibernateOperation)) {
- return HibernateOperationScope.wrapCallDepth(callDepth);
- }
- return HibernateOperationScope.startNew(
- callDepth, hibernateOperation, parentContext, instrumenter());
+ return HibernateOperationScope.start(hibernateOperation, parentContext, instrumenter());
}
@Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class)
public static void endMethod(
- @Advice.Thrown Throwable throwable, @Advice.Enter HibernateOperationScope enterScope) {
+ @Advice.Thrown Throwable throwable, @Advice.Enter HibernateOperationScope scope) {
- HibernateOperationScope.end(enterScope, instrumenter(), throwable);
+ HibernateOperationScope.end(scope, throwable);
}
}
}
diff --git a/instrumentation/hibernate/hibernate-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/hibernate/v4_0/SessionInstrumentation.java b/instrumentation/hibernate/hibernate-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/hibernate/v4_0/SessionInstrumentation.java
index 84dca3c724..a4a4417acf 100644
--- a/instrumentation/hibernate/hibernate-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/hibernate/v4_0/SessionInstrumentation.java
+++ b/instrumentation/hibernate/hibernate-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/hibernate/v4_0/SessionInstrumentation.java
@@ -19,7 +19,6 @@ import static net.bytebuddy.matcher.ElementMatchers.takesArgument;
import io.opentelemetry.context.Context;
import io.opentelemetry.instrumentation.api.util.VirtualField;
-import io.opentelemetry.javaagent.bootstrap.CallDepth;
import io.opentelemetry.javaagent.bootstrap.Java8BytecodeBridge;
import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer;
@@ -103,9 +102,8 @@ public class SessionInstrumentation implements TypeInstrumentation {
@Advice.Argument(0) Object arg0,
@Advice.Argument(value = 1, optional = true) Object arg1) {
- CallDepth callDepth = CallDepth.forClass(HibernateOperation.class);
- if (callDepth.getAndIncrement() > 0) {
- return HibernateOperationScope.wrapCallDepth(callDepth);
+ if (HibernateOperationScope.isStarted()) {
+ return null;
}
VirtualField<SharedSessionContract, SessionInfo> virtualField =
@@ -117,19 +115,15 @@ public class SessionInstrumentation implements TypeInstrumentation {
getEntityName(descriptor, arg0, arg1, EntityNameUtil.bestGuessEntityName(session));
HibernateOperation hibernateOperation =
new HibernateOperation(getSessionMethodOperationName(name), entityName, sessionInfo);
- if (!instrumenter().shouldStart(parentContext, hibernateOperation)) {
- return HibernateOperationScope.wrapCallDepth(callDepth);
- }
- return HibernateOperationScope.startNew(
- callDepth, hibernateOperation, parentContext, instrumenter());
+ return HibernateOperationScope.start(hibernateOperation, parentContext, instrumenter());
}
@Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class)
public static void endMethod(
- @Advice.Thrown Throwable throwable, @Advice.Enter HibernateOperationScope enterState) {
+ @Advice.Thrown Throwable throwable, @Advice.Enter HibernateOperationScope scope) {
- HibernateOperationScope.end(enterState, instrumenter(), throwable);
+ HibernateOperationScope.end(scope, throwable);
}
}
diff --git a/instrumentation/hibernate/hibernate-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/hibernate/v4_0/TransactionInstrumentation.java b/instrumentation/hibernate/hibernate-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/hibernate/v4_0/TransactionInstrumentation.java
index dce7dd4a85..a7f1a3b54e 100644
--- a/instrumentation/hibernate/hibernate-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/hibernate/v4_0/TransactionInstrumentation.java
+++ b/instrumentation/hibernate/hibernate-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/hibernate/v4_0/TransactionInstrumentation.java
@@ -14,7 +14,6 @@ import static net.bytebuddy.matcher.ElementMatchers.takesArguments;
import io.opentelemetry.context.Context;
import io.opentelemetry.instrumentation.api.util.VirtualField;
-import io.opentelemetry.javaagent.bootstrap.CallDepth;
import io.opentelemetry.javaagent.bootstrap.Java8BytecodeBridge;
import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer;
@@ -51,9 +50,8 @@ public class TransactionInstrumentation implements TypeInstrumentation {
@Advice.OnMethodEnter(suppress = Throwable.class)
public static HibernateOperationScope startCommit(@Advice.This Transaction transaction) {
- CallDepth callDepth = CallDepth.forClass(HibernateOperation.class);
- if (callDepth.getAndIncrement() > 0) {
- return HibernateOperationScope.wrapCallDepth(callDepth);
+ if (HibernateOperationScope.isStarted()) {
+ return null;
}
VirtualField<Transaction, SessionInfo> transactionVirtualField =
@@ -63,19 +61,15 @@ public class TransactionInstrumentation implements TypeInstrumentation {
Context parentContext = Java8BytecodeBridge.currentContext();
HibernateOperation hibernateOperation =
new HibernateOperation("Transaction.commit", sessionInfo);
- if (!instrumenter().shouldStart(parentContext, hibernateOperation)) {
- return HibernateOperationScope.wrapCallDepth(callDepth);
- }
- return HibernateOperationScope.startNew(
- callDepth, hibernateOperation, parentContext, instrumenter());
+ return HibernateOperationScope.start(hibernateOperation, parentContext, instrumenter());
}
@Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class)
public static void endCommit(
- @Advice.Thrown Throwable throwable, @Advice.Enter HibernateOperationScope enterScope) {
+ @Advice.Thrown Throwable throwable, @Advice.Enter HibernateOperationScope scope) {
- HibernateOperationScope.end(enterScope, instrumenter(), throwable);
+ HibernateOperationScope.end(scope, throwable);
}
}
}
diff --git a/instrumentation/hibernate/hibernate-6.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/hibernate/v6_0/QueryInstrumentation.java b/instrumentation/hibernate/hibernate-6.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/hibernate/v6_0/QueryInstrumentation.java
index bc024f3087..12509e8d3c 100644
--- a/instrumentation/hibernate/hibernate-6.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/hibernate/v6_0/QueryInstrumentation.java
+++ b/instrumentation/hibernate/hibernate-6.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/hibernate/v6_0/QueryInstrumentation.java
@@ -15,7 +15,6 @@ import static net.bytebuddy.matcher.ElementMatchers.namedOneOf;
import io.opentelemetry.context.Context;
import io.opentelemetry.instrumentation.api.util.VirtualField;
-import io.opentelemetry.javaagent.bootstrap.CallDepth;
import io.opentelemetry.javaagent.bootstrap.Java8BytecodeBridge;
import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer;
@@ -66,9 +65,8 @@ public class QueryInstrumentation implements TypeInstrumentation {
@Advice.OnMethodEnter(suppress = Throwable.class)
public static HibernateOperationScope startMethod(@Advice.This CommonQueryContract query) {
- CallDepth callDepth = CallDepth.forClass(HibernateOperation.class);
- if (callDepth.getAndIncrement() > 0) {
- return HibernateOperationScope.wrapCallDepth(callDepth);
+ if (HibernateOperationScope.isStarted()) {
+ return null;
}
String queryString = null;
@@ -90,19 +88,15 @@ public class QueryInstrumentation implements TypeInstrumentation {
Context parentContext = Java8BytecodeBridge.currentContext();
HibernateOperation hibernateOperation =
new HibernateOperation(getOperationNameForQuery(queryString), sessionInfo);
- if (!instrumenter().shouldStart(parentContext, hibernateOperation)) {
- return HibernateOperationScope.wrapCallDepth(callDepth);
- }
- return HibernateOperationScope.startNew(
- callDepth, hibernateOperation, parentContext, instrumenter());
+ return HibernateOperationScope.start(hibernateOperation, parentContext, instrumenter());
}
@Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class)
public static void endMethod(
- @Advice.Thrown Throwable throwable, @Advice.Enter HibernateOperationScope enterScope) {
+ @Advice.Thrown Throwable throwable, @Advice.Enter HibernateOperationScope scope) {
- HibernateOperationScope.end(enterScope, instrumenter(), throwable);
+ HibernateOperationScope.end(scope, throwable);
}
}
}
diff --git a/instrumentation/hibernate/hibernate-6.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/hibernate/v6_0/SessionInstrumentation.java b/instrumentation/hibernate/hibernate-6.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/hibernate/v6_0/SessionInstrumentation.java
index f0f810c5b9..b5eee5cf2a 100644
--- a/instrumentation/hibernate/hibernate-6.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/hibernate/v6_0/SessionInstrumentation.java
+++ b/instrumentation/hibernate/hibernate-6.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/hibernate/v6_0/SessionInstrumentation.java
@@ -19,7 +19,6 @@ import static net.bytebuddy.matcher.ElementMatchers.takesArgument;
import io.opentelemetry.context.Context;
import io.opentelemetry.instrumentation.api.util.VirtualField;
-import io.opentelemetry.javaagent.bootstrap.CallDepth;
import io.opentelemetry.javaagent.bootstrap.Java8BytecodeBridge;
import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer;
@@ -102,9 +101,8 @@ public class SessionInstrumentation implements TypeInstrumentation {
@Advice.Argument(0) Object arg0,
@Advice.Argument(value = 1, optional = true) Object arg1) {
- CallDepth callDepth = CallDepth.forClass(HibernateOperation.class);
- if (callDepth.getAndIncrement() > 0) {
- return HibernateOperationScope.wrapCallDepth(callDepth);
+ if (HibernateOperationScope.isStarted()) {
+ return null;
}
VirtualField<SharedSessionContract, SessionInfo> virtualField =
@@ -116,19 +114,15 @@ public class SessionInstrumentation implements TypeInstrumentation {
getEntityName(descriptor, arg0, arg1, EntityNameUtil.bestGuessEntityName(session));
HibernateOperation hibernateOperation =
new HibernateOperation(getSessionMethodOperationName(name), entityName, sessionInfo);
- if (!instrumenter().shouldStart(parentContext, hibernateOperation)) {
- return HibernateOperationScope.wrapCallDepth(callDepth);
- }
- return HibernateOperationScope.startNew(
- callDepth, hibernateOperation, parentContext, instrumenter());
+ return HibernateOperationScope.start(hibernateOperation, parentContext, instrumenter());
}
@Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class)
public static void endMethod(
- @Advice.Thrown Throwable throwable, @Advice.Enter HibernateOperationScope enterScope) {
+ @Advice.Thrown Throwable throwable, @Advice.Enter HibernateOperationScope scope) {
- HibernateOperationScope.end(enterScope, instrumenter(), throwable);
+ HibernateOperationScope.end(scope, throwable);
}
}
diff --git a/instrumentation/hibernate/hibernate-6.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/hibernate/v6_0/TransactionInstrumentation.java b/instrumentation/hibernate/hibernate-6.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/hibernate/v6_0/TransactionInstrumentation.java
index f82a6882c4..a304331825 100644
--- a/instrumentation/hibernate/hibernate-6.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/hibernate/v6_0/TransactionInstrumentation.java
+++ b/instrumentation/hibernate/hibernate-6.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/hibernate/v6_0/TransactionInstrumentation.java
@@ -14,7 +14,6 @@ import static net.bytebuddy.matcher.ElementMatchers.takesArguments;
import io.opentelemetry.context.Context;
import io.opentelemetry.instrumentation.api.util.VirtualField;
-import io.opentelemetry.javaagent.bootstrap.CallDepth;
import io.opentelemetry.javaagent.bootstrap.Java8BytecodeBridge;
import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer;
@@ -51,9 +50,8 @@ public class TransactionInstrumentation implements TypeInstrumentation {
@Advice.OnMethodEnter(suppress = Throwable.class)
public static HibernateOperationScope startCommit(@Advice.This Transaction transaction) {
- CallDepth callDepth = CallDepth.forClass(HibernateOperation.class);
- if (callDepth.getAndIncrement() > 0) {
- return HibernateOperationScope.wrapCallDepth(callDepth);
+ if (HibernateOperationScope.isStarted()) {
+ return null;
}
VirtualField<Transaction, SessionInfo> transactionVirtualField =
@@ -64,18 +62,17 @@ public class TransactionInstrumentation implements TypeInstrumentation {
HibernateOperation hibernateOperation =
new HibernateOperation("Transaction.commit", sessionInfo);
if (!instrumenter().shouldStart(parentContext, hibernateOperation)) {
- return HibernateOperationScope.wrapCallDepth(callDepth);
+ return null;
}
- return HibernateOperationScope.startNew(
- callDepth, hibernateOperation, parentContext, instrumenter());
+ return HibernateOperationScope.start(hibernateOperation, parentContext, instrumenter());
}
@Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class)
public static void endCommit(
- @Advice.Thrown Throwable throwable, @Advice.Enter HibernateOperationScope enterScope) {
+ @Advice.Thrown Throwable throwable, @Advice.Enter HibernateOperationScope scope) {
- HibernateOperationScope.end(enterScope, instrumenter(), throwable);
+ HibernateOperationScope.end(scope, throwable);
}
}
}
diff --git a/instrumentation/hibernate/hibernate-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/hibernate/HibernateOperationScope.java b/instrumentation/hibernate/hibernate-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/hibernate/HibernateOperationScope.java
index 96e9dba438..53a8d131c8 100644
--- a/instrumentation/hibernate/hibernate-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/hibernate/HibernateOperationScope.java
+++ b/instrumentation/hibernate/hibernate-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/hibernate/HibernateOperationScope.java
@@ -10,80 +10,76 @@ import io.opentelemetry.context.Scope;
import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter;
import io.opentelemetry.javaagent.bootstrap.CallDepth;
-public class HibernateOperationScope {
+public final class HibernateOperationScope {
- private final CallDepth callDepth;
+ private final Instrumenter<HibernateOperation, Void> instrumenter;
private final HibernateOperation hibernateOperation;
private final Context context;
private final Scope scope;
private HibernateOperationScope(
- CallDepth callDepth, HibernateOperation hibernateOperation, Context context, Scope scope) {
- this.callDepth = callDepth;
+ Instrumenter<HibernateOperation, Void> instrumenter,
+ HibernateOperation hibernateOperation,
+ Context context,
+ Scope scope) {
+ this.instrumenter = instrumenter;
this.hibernateOperation = hibernateOperation;
this.context = context;
this.scope = scope;
}
+ /**
+ * Checks whether there already is an ongoing operation
+ *
+ * @return true if the operation has already been started
+ */
+ public static boolean isStarted() {
+ CallDepth callDepth = CallDepth.forClass(HibernateOperation.class);
+ return callDepth.getAndIncrement() > 0;
+ }
+
/**
* Starts operation scope
*
- * @param callDepth call depth
* @param hibernateOperation hibernate operation
* @param parentContext parent context
* @param instrumenter instrumenter
- * @return operation scope, to be ended with {@link #end(HibernateOperationScope, Instrumenter,
- * Throwable)} on exit advice
+ * @return operation scope, to be ended with {@link #end(HibernateOperationScope, Throwable)} on
+ * exit advice, or null
*/
- public static HibernateOperationScope startNew(
- CallDepth callDepth,
+ public static HibernateOperationScope start(
HibernateOperation hibernateOperation,
Context parentContext,
Instrumenter<HibernateOperation, Void> instrumenter) {
+ if (!instrumenter.shouldStart(parentContext, hibernateOperation)) {
+ return null;
+ }
+
Context context = instrumenter.start(parentContext, hibernateOperation);
return new HibernateOperationScope(
- callDepth, hibernateOperation, context, context.makeCurrent());
- }
-
- /**
- * Builds hibernate operation scope from an existing call depth for cases where only the call
- * depth is needed
- *
- * @param callDepth call depth
- * @return hibernate operation scope wrapping the provided call depth.
- */
- public static HibernateOperationScope wrapCallDepth(CallDepth callDepth) {
- return new HibernateOperationScope(callDepth, null, null, null);
+ instrumenter, hibernateOperation, context, context.makeCurrent());
}
/**
* Ends operation scope
*
- * @param hibernateOperationScope hibernate operation scope
- * @param instrumenter instrumenter
+ * @param scope operation scope
* @param throwable thrown exception
*/
- public static void end(
- HibernateOperationScope hibernateOperationScope,
- Instrumenter<HibernateOperation, Void> instrumenter,
- Throwable throwable) {
-
- if (hibernateOperationScope.context == null) {
- // call depth only
- hibernateOperationScope.callDepth.decrementAndGet();
+ public static void end(HibernateOperationScope scope, Throwable throwable) {
+ CallDepth callDepth = CallDepth.forClass(HibernateOperation.class);
+ if (callDepth.decrementAndGet() > 0) {
return;
}
- int depth = hibernateOperationScope.callDepth.decrementAndGet();
- if (depth != 0) {
- throw new IllegalStateException("unexpected call depth " + depth);
+ if (scope != null) {
+ scope.end(throwable);
}
- hibernateOperationScope.scope.close();
- instrumenter.end(
- hibernateOperationScope.context,
- hibernateOperationScope.hibernateOperation,
- null,
- throwable);
+ }
+
+ private void end(Throwable throwable) {
+ scope.close();
+ instrumenter.end(context, hibernateOperation, null, throwable);
}
}
diff --git a/instrumentation/hibernate/hibernate-procedure-call-4.3/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/hibernate/v4_3/ProcedureCallInstrumentation.java b/instrumentation/hibernate/hibernate-procedure-call-4.3/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/hibernate/v4_3/ProcedureCallInstrumentation.java
index 6c8a51dbdc..4574536735 100644
--- a/instrumentation/hibernate/hibernate-procedure-call-4.3/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/hibernate/v4_3/ProcedureCallInstrumentation.java
+++ b/instrumentation/hibernate/hibernate-procedure-call-4.3/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/hibernate/v4_3/ProcedureCallInstrumentation.java
@@ -13,7 +13,6 @@ import static net.bytebuddy.matcher.ElementMatchers.named;
import io.opentelemetry.context.Context;
import io.opentelemetry.instrumentation.api.util.VirtualField;
-import io.opentelemetry.javaagent.bootstrap.CallDepth;
import io.opentelemetry.javaagent.bootstrap.Java8BytecodeBridge;
import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer;
@@ -51,9 +50,8 @@ public class ProcedureCallInstrumentation implements TypeInstrumentation {
public static HibernateOperationScope startMethod(
@Advice.This ProcedureCall call, @Advice.Origin("#m") String name) {
- CallDepth callDepth = CallDepth.forClass(HibernateOperation.class);
- if (callDepth.getAndIncrement() > 0) {
- return HibernateOperationScope.wrapCallDepth(callDepth);
+ if (HibernateOperationScope.isStarted()) {
+ return null;
}
VirtualField<ProcedureCall, SessionInfo> criteriaVirtualField =
@@ -64,18 +62,17 @@ public class ProcedureCallInstrumentation implements TypeInstrumentation {
HibernateOperation hibernateOperation =
new HibernateOperation("ProcedureCall." + name, call.getProcedureName(), sessionInfo);
if (!instrumenter().shouldStart(parentContext, hibernateOperation)) {
- return HibernateOperationScope.wrapCallDepth(callDepth);
+ return null;
}
- return HibernateOperationScope.startNew(
- callDepth, hibernateOperation, parentContext, instrumenter());
+ return HibernateOperationScope.start(hibernateOperation, parentContext, instrumenter());
}
@Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class)
public static void endMethod(
- @Advice.Thrown Throwable throwable, @Advice.Enter HibernateOperationScope enterScope) {
+ @Advice.Thrown Throwable throwable, @Advice.Enter HibernateOperationScope scope) {
- HibernateOperationScope.end(enterScope, instrumenter(), throwable);
+ HibernateOperationScope.end(scope, throwable);
}
}
}
I think this is a good idea @laurit, I initially wanted to minimize the changes to "strictly necessary" for the migration, but it's even better if we can further simplify the enter advice by not directly using the call depth and passing it to the end advice. The only minor downside I see with this approach is that the state between enter and exit advices is not obvious (but we can make it clear in comments).
Given that CallDepth.forClass(HibernateOperation.class) does a lookup in a thread local storage there is little/no harm to do the lookup twice.
I will try to include your changes in this PR and update it.
@laurit this works perfectly, I've only slightly renamed the nested call check method.