From 5aa65ea5eb5d2d02923bdc6875a7a83a4dd5e89d Mon Sep 17 00:00:00 2001 From: Cesar Munoz <56847527+LikeTheSalad@users.noreply.github.com> Date: Fri, 28 Jun 2024 14:11:02 +0200 Subject: [PATCH 01/20] Making influxdb advices indy friendly --- .../v2_4/InfluxDbImplInstrumentation.java | 106 +++++++++--------- .../influxdb/v2_4/InfluxDbScope.java | 20 ++++ 2 files changed, 73 insertions(+), 53 deletions(-) create mode 100644 instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbScope.java diff --git a/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbImplInstrumentation.java b/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbImplInstrumentation.java index 92456cc181da..087f179632ba 100644 --- a/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbImplInstrumentation.java +++ b/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbImplInstrumentation.java @@ -70,67 +70,69 @@ public void transform(TypeTransformer transformer) { public static class InfluxDbQueryAdvice { @Advice.OnMethodEnter(suppress = Throwable.class) - public static void onEnter( - @Advice.Argument(0) Query query, - @Advice.AllArguments(readOnly = false, typing = Assigner.Typing.DYNAMIC) Object[] arguments, - @Advice.FieldValue(value = "retrofit") Retrofit retrofit, - @Advice.Local("otelCallDepth") CallDepth callDepth, - @Advice.Local("otelRequest") InfluxDbRequest influxDbRequest, - @Advice.Local("otelContext") Context context, - @Advice.Local("otelScope") Scope scope) { - callDepth = CallDepth.forClass(InfluxDBImpl.class); + @Advice.AssignReturned.ToAllArguments(index = 0, typing = Assigner.Typing.DYNAMIC) + public static Object[] onEnter( + @Advice.AllArguments(typing = Assigner.Typing.DYNAMIC) Object[] arguments, + @Advice.FieldValue(value = "retrofit") Retrofit retrofit) { + CallDepth callDepth = CallDepth.forClass(InfluxDBImpl.class); if (callDepth.getAndIncrement() > 0) { - return; + return null; } + Query query = arguments[0] instanceof Query ? (Query) arguments[0] : null; if (query == null) { - return; + return null; } Context parentContext = currentContext(); HttpUrl httpUrl = retrofit.baseUrl(); - influxDbRequest = + InfluxDbRequest influxDbRequest = InfluxDbRequest.create( httpUrl.host(), httpUrl.port(), query.getDatabase(), null, query.getCommand()); if (!instrumenter().shouldStart(parentContext, influxDbRequest)) { - return; + return null; } // wrap callbacks so they'd run in the context of the parent span - Object[] newArguments = new Object[arguments.length]; - boolean hasChangedArgument = false; + Object[] newArguments = new Object[arguments.length + 1]; for (int i = 0; i < arguments.length; i++) { newArguments[i] = InfluxDbObjetWrapper.wrap(arguments[i], parentContext); - hasChangedArgument |= newArguments[i] != arguments[i]; - } - if (hasChangedArgument) { - arguments = newArguments; } - context = instrumenter().start(parentContext, influxDbRequest); - scope = context.makeCurrent(); + Context context = instrumenter().start(parentContext, influxDbRequest); + Scope scope = context.makeCurrent(); + + newArguments[arguments.length] = + new InfluxDbScope(callDepth, influxDbRequest, context, scope); + + return newArguments; } @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) public static void onExit( - @Advice.Thrown Throwable throwable, - @Advice.Local("otelCallDepth") CallDepth callDepth, - @Advice.Local("otelRequest") InfluxDbRequest influxDbRequest, - @Advice.Local("otelContext") Context context, - @Advice.Local("otelScope") Scope scope) { + @Advice.Thrown Throwable throwable, @Advice.Enter Object[] enterArgs) { + if (enterArgs == null) { + return; + } - if (callDepth.decrementAndGet() > 0) { + Object extraObject = enterArgs[enterArgs.length - 1]; + if (!(extraObject instanceof InfluxDbScope)) { return; } + InfluxDbScope influxDbScope = (InfluxDbScope) extraObject; - if (scope == null) { + if (influxDbScope.callDepth.decrementAndGet() > 0) { return; } - scope.close(); + if (influxDbScope.scope == null) { + return; + } - instrumenter().end(context, influxDbRequest, null, throwable); + influxDbScope.scope.close(); + + instrumenter().end(influxDbScope.context, influxDbScope.influxDbRequest, null, throwable); } } @@ -138,21 +140,17 @@ public static void onExit( public static class InfluxDbModifyAdvice { @Advice.OnMethodEnter(suppress = Throwable.class) - public static void onEnter( + public static Object onEnter( @Advice.Origin("#m") String methodName, @Advice.Argument(0) Object arg0, - @Advice.FieldValue(value = "retrofit") Retrofit retrofit, - @Advice.Local("otelCallDepth") CallDepth callDepth, - @Advice.Local("otelRequest") InfluxDbRequest influxDbRequest, - @Advice.Local("otelContext") Context context, - @Advice.Local("otelScope") Scope scope) { - callDepth = CallDepth.forClass(InfluxDBImpl.class); + @Advice.FieldValue(value = "retrofit") Retrofit retrofit) { + CallDepth callDepth = CallDepth.forClass(InfluxDBImpl.class); if (callDepth.getAndIncrement() > 0) { - return; + return null; } if (arg0 == null) { - return; + return null; } Context parentContext = currentContext(); @@ -173,35 +171,37 @@ public static void onEnter( operation = "WRITE"; } - influxDbRequest = + InfluxDbRequest influxDbRequest = InfluxDbRequest.create(httpUrl.host(), httpUrl.port(), database, operation, null); if (!instrumenter().shouldStart(parentContext, influxDbRequest)) { - return; + return null; } - context = instrumenter().start(parentContext, influxDbRequest); - scope = context.makeCurrent(); + Context context = instrumenter().start(parentContext, influxDbRequest); + Scope scope = context.makeCurrent(); + + return new InfluxDbScope(callDepth, influxDbRequest, context, scope); } @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) - public static void onExit( - @Advice.Thrown Throwable throwable, - @Advice.Local("otelCallDepth") CallDepth callDepth, - @Advice.Local("otelRequest") InfluxDbRequest influxDbRequest, - @Advice.Local("otelContext") Context context, - @Advice.Local("otelScope") Scope scope) { + public static void onExit(@Advice.Thrown Throwable throwable, @Advice.Enter Object scope) { + if (!(scope instanceof InfluxDbScope)) { + return; + } + + InfluxDbScope influxDbScope = (InfluxDbScope) scope; - if (callDepth.decrementAndGet() > 0) { + if (influxDbScope.callDepth.decrementAndGet() > 0) { return; } - if (scope == null) { + if (influxDbScope.scope == null) { return; } - scope.close(); + influxDbScope.scope.close(); - instrumenter().end(context, influxDbRequest, null, throwable); + instrumenter().end(influxDbScope.context, influxDbScope.influxDbRequest, null, throwable); } } } diff --git a/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbScope.java b/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbScope.java new file mode 100644 index 000000000000..e1181f7b5a44 --- /dev/null +++ b/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbScope.java @@ -0,0 +1,20 @@ +package io.opentelemetry.javaagent.instrumentation.influxdb.v2_4; + +import io.opentelemetry.context.Context; +import io.opentelemetry.context.Scope; +import io.opentelemetry.javaagent.bootstrap.CallDepth; + +public class InfluxDbScope { + public final CallDepth callDepth; + public final InfluxDbRequest influxDbRequest; + public final Context context; + public final Scope scope; + + public InfluxDbScope( + CallDepth callDepth, InfluxDbRequest influxDbRequest, Context context, Scope scope) { + this.callDepth = callDepth; + this.influxDbRequest = influxDbRequest; + this.context = context; + this.scope = scope; + } +} From 736f58f7338234a679546276b4171b86f242bbd6 Mon Sep 17 00:00:00 2001 From: Cesar Munoz <56847527+LikeTheSalad@users.noreply.github.com> Date: Fri, 28 Jun 2024 14:23:50 +0200 Subject: [PATCH 02/20] Returning original args when skipping the advice --- .../influxdb/v2_4/InfluxDbImplInstrumentation.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbImplInstrumentation.java b/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbImplInstrumentation.java index 087f179632ba..ad0be30aea17 100644 --- a/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbImplInstrumentation.java +++ b/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbImplInstrumentation.java @@ -69,19 +69,19 @@ public void transform(TypeTransformer transformer) { @SuppressWarnings("unused") public static class InfluxDbQueryAdvice { - @Advice.OnMethodEnter(suppress = Throwable.class) + @Advice.OnMethodEnter @Advice.AssignReturned.ToAllArguments(index = 0, typing = Assigner.Typing.DYNAMIC) public static Object[] onEnter( @Advice.AllArguments(typing = Assigner.Typing.DYNAMIC) Object[] arguments, @Advice.FieldValue(value = "retrofit") Retrofit retrofit) { CallDepth callDepth = CallDepth.forClass(InfluxDBImpl.class); if (callDepth.getAndIncrement() > 0) { - return null; + return arguments; } Query query = arguments[0] instanceof Query ? (Query) arguments[0] : null; if (query == null) { - return null; + return arguments; } Context parentContext = currentContext(); @@ -91,7 +91,7 @@ public static Object[] onEnter( httpUrl.host(), httpUrl.port(), query.getDatabase(), null, query.getCommand()); if (!instrumenter().shouldStart(parentContext, influxDbRequest)) { - return null; + return arguments; } // wrap callbacks so they'd run in the context of the parent span From 47e3749d81cec4641a3f3059bdb63770845073ad Mon Sep 17 00:00:00 2001 From: Cesar Munoz <56847527+LikeTheSalad@users.noreply.github.com> Date: Fri, 28 Jun 2024 14:41:54 +0200 Subject: [PATCH 03/20] Re-adding enter advice suppress throwable --- .../influxdb/v2_4/InfluxDbImplInstrumentation.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbImplInstrumentation.java b/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbImplInstrumentation.java index ad0be30aea17..70a6f8b9856c 100644 --- a/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbImplInstrumentation.java +++ b/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbImplInstrumentation.java @@ -69,7 +69,7 @@ public void transform(TypeTransformer transformer) { @SuppressWarnings("unused") public static class InfluxDbQueryAdvice { - @Advice.OnMethodEnter + @Advice.OnMethodEnter(suppress = Throwable.class) @Advice.AssignReturned.ToAllArguments(index = 0, typing = Assigner.Typing.DYNAMIC) public static Object[] onEnter( @Advice.AllArguments(typing = Assigner.Typing.DYNAMIC) Object[] arguments, From 1307c57a19aad7f33a42df21b34afef8fca2ca03 Mon Sep 17 00:00:00 2001 From: Cesar Munoz <56847527+LikeTheSalad@users.noreply.github.com> Date: Fri, 28 Jun 2024 15:07:06 +0200 Subject: [PATCH 04/20] Moving influxdb start/end logic to InfluxDbScope --- .../v2_4/InfluxDbImplInstrumentation.java | 55 +++---------------- .../influxdb/v2_4/InfluxDbScope.java | 40 ++++++++++++-- 2 files changed, 44 insertions(+), 51 deletions(-) diff --git a/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbImplInstrumentation.java b/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbImplInstrumentation.java index 70a6f8b9856c..2d86c6221df9 100644 --- a/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbImplInstrumentation.java +++ b/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbImplInstrumentation.java @@ -15,7 +15,7 @@ import static net.bytebuddy.matcher.ElementMatchers.takesArguments; import io.opentelemetry.context.Context; -import io.opentelemetry.context.Scope; +import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter; import io.opentelemetry.javaagent.bootstrap.CallDepth; import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation; import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer; @@ -90,7 +90,8 @@ public static Object[] onEnter( InfluxDbRequest.create( httpUrl.host(), httpUrl.port(), query.getDatabase(), null, query.getCommand()); - if (!instrumenter().shouldStart(parentContext, influxDbRequest)) { + Instrumenter instrumenter = instrumenter(); + if (!instrumenter.shouldStart(parentContext, influxDbRequest)) { return arguments; } @@ -100,11 +101,8 @@ public static Object[] onEnter( newArguments[i] = InfluxDbObjetWrapper.wrap(arguments[i], parentContext); } - Context context = instrumenter().start(parentContext, influxDbRequest); - Scope scope = context.makeCurrent(); - newArguments[arguments.length] = - new InfluxDbScope(callDepth, influxDbRequest, context, scope); + InfluxDbScope.start(instrumenter, callDepth, parentContext, influxDbRequest); return newArguments; } @@ -112,27 +110,9 @@ public static Object[] onEnter( @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) public static void onExit( @Advice.Thrown Throwable throwable, @Advice.Enter Object[] enterArgs) { - if (enterArgs == null) { - return; - } - Object extraObject = enterArgs[enterArgs.length - 1]; - if (!(extraObject instanceof InfluxDbScope)) { - return; - } - InfluxDbScope influxDbScope = (InfluxDbScope) extraObject; - - if (influxDbScope.callDepth.decrementAndGet() > 0) { - return; - } - - if (influxDbScope.scope == null) { - return; - } - - influxDbScope.scope.close(); - instrumenter().end(influxDbScope.context, influxDbScope.influxDbRequest, null, throwable); + InfluxDbScope.end(extraObject, instrumenter(), throwable); } } @@ -174,34 +154,17 @@ public static Object onEnter( InfluxDbRequest influxDbRequest = InfluxDbRequest.create(httpUrl.host(), httpUrl.port(), database, operation, null); - if (!instrumenter().shouldStart(parentContext, influxDbRequest)) { + Instrumenter instrumenter = instrumenter(); + if (!instrumenter.shouldStart(parentContext, influxDbRequest)) { return null; } - Context context = instrumenter().start(parentContext, influxDbRequest); - Scope scope = context.makeCurrent(); - - return new InfluxDbScope(callDepth, influxDbRequest, context, scope); + return InfluxDbScope.start(instrumenter, callDepth, parentContext, influxDbRequest); } @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) public static void onExit(@Advice.Thrown Throwable throwable, @Advice.Enter Object scope) { - if (!(scope instanceof InfluxDbScope)) { - return; - } - - InfluxDbScope influxDbScope = (InfluxDbScope) scope; - - if (influxDbScope.callDepth.decrementAndGet() > 0) { - return; - } - - if (influxDbScope.scope == null) { - return; - } - influxDbScope.scope.close(); - - instrumenter().end(influxDbScope.context, influxDbScope.influxDbRequest, null, throwable); + InfluxDbScope.end(scope, instrumenter(), throwable); } } } diff --git a/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbScope.java b/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbScope.java index e1181f7b5a44..a99ae5d59f8a 100644 --- a/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbScope.java +++ b/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbScope.java @@ -2,19 +2,49 @@ import io.opentelemetry.context.Context; import io.opentelemetry.context.Scope; +import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter; import io.opentelemetry.javaagent.bootstrap.CallDepth; public class InfluxDbScope { - public final CallDepth callDepth; - public final InfluxDbRequest influxDbRequest; - public final Context context; - public final Scope scope; + private final CallDepth callDepth; + private final InfluxDbRequest influxDbRequest; + private final Context context; + private final Scope scope; - public InfluxDbScope( + private InfluxDbScope( CallDepth callDepth, InfluxDbRequest influxDbRequest, Context context, Scope scope) { this.callDepth = callDepth; this.influxDbRequest = influxDbRequest; this.context = context; this.scope = scope; } + + public static InfluxDbScope start( + Instrumenter instrumenter, + CallDepth callDepth, + Context parentContext, + InfluxDbRequest influxDbRequest) { + Context context = instrumenter.start(parentContext, influxDbRequest); + return new InfluxDbScope(callDepth, influxDbRequest, context, context.makeCurrent()); + } + + public static void end( + Object scope, Instrumenter instrumenter, Throwable throwable) { + if (!(scope instanceof InfluxDbScope)) { + return; + } + InfluxDbScope influxDbScope = (InfluxDbScope) scope; + + if (influxDbScope.callDepth.decrementAndGet() > 0) { + return; + } + + if (influxDbScope.scope == null) { + return; + } + + influxDbScope.scope.close(); + + instrumenter.end(influxDbScope.context, influxDbScope.influxDbRequest, null, throwable); + } } From e6b7869a8a1f45ff2419e2086d33d5b8ef96ca63 Mon Sep 17 00:00:00 2001 From: Cesar Munoz <56847527+LikeTheSalad@users.noreply.github.com> Date: Fri, 28 Jun 2024 15:29:22 +0200 Subject: [PATCH 05/20] Always updating calldepth --- .../influxdb/v2_4/InfluxDbImplInstrumentation.java | 8 ++++++-- .../influxdb/v2_4/InfluxDbScope.java | 13 ++----------- 2 files changed, 8 insertions(+), 13 deletions(-) diff --git a/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbImplInstrumentation.java b/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbImplInstrumentation.java index 2d86c6221df9..3335bdea0f6d 100644 --- a/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbImplInstrumentation.java +++ b/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbImplInstrumentation.java @@ -102,7 +102,7 @@ public static Object[] onEnter( } newArguments[arguments.length] = - InfluxDbScope.start(instrumenter, callDepth, parentContext, influxDbRequest); + InfluxDbScope.start(instrumenter, parentContext, influxDbRequest); return newArguments; } @@ -159,11 +159,15 @@ public static Object onEnter( return null; } - return InfluxDbScope.start(instrumenter, callDepth, parentContext, influxDbRequest); + return InfluxDbScope.start(instrumenter, parentContext, influxDbRequest); } @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) public static void onExit(@Advice.Thrown Throwable throwable, @Advice.Enter Object scope) { + CallDepth callDepth = CallDepth.forClass(InfluxDBImpl.class); + if (callDepth.decrementAndGet() > 0) { + return; + } InfluxDbScope.end(scope, instrumenter(), throwable); } } diff --git a/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbScope.java b/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbScope.java index a99ae5d59f8a..dfb745b33c64 100644 --- a/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbScope.java +++ b/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbScope.java @@ -3,17 +3,13 @@ import io.opentelemetry.context.Context; import io.opentelemetry.context.Scope; import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter; -import io.opentelemetry.javaagent.bootstrap.CallDepth; public class InfluxDbScope { - private final CallDepth callDepth; private final InfluxDbRequest influxDbRequest; private final Context context; private final Scope scope; - private InfluxDbScope( - CallDepth callDepth, InfluxDbRequest influxDbRequest, Context context, Scope scope) { - this.callDepth = callDepth; + private InfluxDbScope(InfluxDbRequest influxDbRequest, Context context, Scope scope) { this.influxDbRequest = influxDbRequest; this.context = context; this.scope = scope; @@ -21,11 +17,10 @@ private InfluxDbScope( public static InfluxDbScope start( Instrumenter instrumenter, - CallDepth callDepth, Context parentContext, InfluxDbRequest influxDbRequest) { Context context = instrumenter.start(parentContext, influxDbRequest); - return new InfluxDbScope(callDepth, influxDbRequest, context, context.makeCurrent()); + return new InfluxDbScope(influxDbRequest, context, context.makeCurrent()); } public static void end( @@ -35,10 +30,6 @@ public static void end( } InfluxDbScope influxDbScope = (InfluxDbScope) scope; - if (influxDbScope.callDepth.decrementAndGet() > 0) { - return; - } - if (influxDbScope.scope == null) { return; } From ba1e1878d5dfe80e2f33952b83ff3e1848b7f765 Mon Sep 17 00:00:00 2001 From: Cesar Munoz <56847527+LikeTheSalad@users.noreply.github.com> Date: Fri, 28 Jun 2024 16:17:56 +0200 Subject: [PATCH 06/20] Decrementing calldepth for all advices --- .../influxdb/v2_4/InfluxDbImplInstrumentation.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbImplInstrumentation.java b/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbImplInstrumentation.java index 3335bdea0f6d..6004b9544064 100644 --- a/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbImplInstrumentation.java +++ b/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbImplInstrumentation.java @@ -110,6 +110,11 @@ public static Object[] onEnter( @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) public static void onExit( @Advice.Thrown Throwable throwable, @Advice.Enter Object[] enterArgs) { + CallDepth callDepth = CallDepth.forClass(InfluxDBImpl.class); + if (callDepth.decrementAndGet() > 0) { + return; + } + Object extraObject = enterArgs[enterArgs.length - 1]; InfluxDbScope.end(extraObject, instrumenter(), throwable); From 51aee44c684d06602d78bbe1fb80f59c1c5521f2 Mon Sep 17 00:00:00 2001 From: Cesar Munoz <56847527+LikeTheSalad@users.noreply.github.com> Date: Fri, 28 Jun 2024 17:13:15 +0200 Subject: [PATCH 07/20] Making InfluxDbInstrumentationModule indy --- .../influxdb/v2_4/InfluxDbInstrumentationModule.java | 6 ------ 1 file changed, 6 deletions(-) diff --git a/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbInstrumentationModule.java b/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbInstrumentationModule.java index 3b95cc8b1c29..3aba153fc408 100644 --- a/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbInstrumentationModule.java +++ b/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbInstrumentationModule.java @@ -23,10 +23,4 @@ public InfluxDbInstrumentationModule() { public List typeInstrumentations() { return singletonList(new InfluxDbImplInstrumentation()); } - - @Override - public boolean isIndyModule() { - // Uses multiple Advice.Locals and argument assignment - return false; - } } From c6bbf1b8a37e76f5bd6bab2a78e64d5c9ee2033f Mon Sep 17 00:00:00 2001 From: Cesar Munoz <56847527+LikeTheSalad@users.noreply.github.com> Date: Mon, 1 Jul 2024 11:34:27 +0200 Subject: [PATCH 08/20] Adding javadoc to InfluxDbScope --- .../javaagent/instrumentation/influxdb/v2_4/InfluxDbScope.java | 1 + 1 file changed, 1 insertion(+) diff --git a/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbScope.java b/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbScope.java index dfb745b33c64..662a7c7a227f 100644 --- a/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbScope.java +++ b/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbScope.java @@ -4,6 +4,7 @@ import io.opentelemetry.context.Scope; import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter; +/** Container used to carry state between enter and exit advices */ public class InfluxDbScope { private final InfluxDbRequest influxDbRequest; private final Context context; From 0fd93461612c2e6aea2835b288afe7077133c7dd Mon Sep 17 00:00:00 2001 From: Cesar Munoz <56847527+LikeTheSalad@users.noreply.github.com> Date: Mon, 1 Jul 2024 11:41:23 +0200 Subject: [PATCH 09/20] Spotless --- .../instrumentation/influxdb/v2_4/InfluxDbScope.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbScope.java b/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbScope.java index 662a7c7a227f..0109c7ea2d94 100644 --- a/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbScope.java +++ b/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbScope.java @@ -1,3 +1,8 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + package io.opentelemetry.javaagent.instrumentation.influxdb.v2_4; import io.opentelemetry.context.Context; From 0661d67793f668470618938dfaf52db1cad229bb Mon Sep 17 00:00:00 2001 From: Jonas Kunz Date: Thu, 13 Jun 2024 10:42:14 +0200 Subject: [PATCH 10/20] Enable @Advice.AssignReturned for non-indy Advices --- .../tooling/instrumentation/TypeTransformerImpl.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/TypeTransformerImpl.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/TypeTransformerImpl.java index 1f1059f7e492..20c7a9f4f1c7 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/TypeTransformerImpl.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/TypeTransformerImpl.java @@ -9,14 +9,18 @@ import io.opentelemetry.javaagent.tooling.Utils; import io.opentelemetry.javaagent.tooling.bytebuddy.ExceptionHandlers; import net.bytebuddy.agent.builder.AgentBuilder; +import net.bytebuddy.asm.Advice; import net.bytebuddy.description.method.MethodDescription; import net.bytebuddy.matcher.ElementMatcher; final class TypeTransformerImpl implements TypeTransformer { private AgentBuilder.Identified.Extendable agentBuilder; + private final Advice.WithCustomMapping adviceMapping; TypeTransformerImpl(AgentBuilder.Identified.Extendable agentBuilder) { this.agentBuilder = agentBuilder; + adviceMapping = Advice.withCustomMapping() + .with(new Advice.AssignReturned.Factory().withSuppressed(Throwable.class)); } @Override @@ -24,7 +28,7 @@ public void applyAdviceToMethod( ElementMatcher methodMatcher, String adviceClassName) { agentBuilder = agentBuilder.transform( - new AgentBuilder.Transformer.ForAdvice() + new AgentBuilder.Transformer.ForAdvice(adviceMapping) .include( Utils.getBootstrapProxy(), Utils.getAgentClassLoader(), From 73e1c139292add9455fd01959a31dbeaafb4adb3 Mon Sep 17 00:00:00 2001 From: Jonas Kunz Date: Mon, 24 Jun 2024 16:31:59 +0200 Subject: [PATCH 11/20] Prevent advice transformations on advices already using @Advice.AssignReturned --- .../indy/AdviceTransformer.java | 211 ++++++++++-------- 1 file changed, 119 insertions(+), 92 deletions(-) diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/AdviceTransformer.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/AdviceTransformer.java index e71ba5b7874d..43a53ca450ac 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/AdviceTransformer.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/AdviceTransformer.java @@ -50,6 +50,10 @@ static byte[] transform(byte[] bytes) { return cw.toByteArray(); } + //Advices already using Advice.AssignReturned are assumed to be already compatible + //Those won't be transformed except for setting inline to false + boolean justDelegateAdvice = usesAssignReturned(classNode); + // sort enter advice method before exit advice classNode.methods.sort( Comparator.comparingInt( @@ -75,7 +79,7 @@ public MethodVisitor visitMethod( public void visitEnd() { super.visitEnd(); - instrument(context, this, classVisitor); + instrument(context, this, classVisitor, justDelegateAdvice); } }; } @@ -225,6 +229,25 @@ private static List getLocals(MethodNode source) { return result; } + private static final Type ADVICE_ASSIGN_RETURNED_TO_RETURNED = Type.getType(Advice.AssignReturned.ToReturned.class); + private static final Type ADVICE_ASSIGN_RETURNED_TO_ARGUMENTS = Type.getType(Advice.AssignReturned.ToArguments.class); + private static final Type ADVICE_ASSIGN_RETURNED_TO_FIELDS = Type.getType(Advice.AssignReturned.ToFields.class); + + private static boolean usesAssignReturned(MethodNode source) { + return hasAnnotation(source, ADVICE_ASSIGN_RETURNED_TO_RETURNED) + || hasAnnotation(source, ADVICE_ASSIGN_RETURNED_TO_ARGUMENTS) + || hasAnnotation(source, ADVICE_ASSIGN_RETURNED_TO_FIELDS); + } + + private static boolean usesAssignReturned(ClassNode classNode) { + for (MethodNode mn : classNode.methods) { + if (usesAssignReturned(mn)) { + return true; + } + } + return false; + } + private static final Type ADVICE_ON_METHOD_ENTER = Type.getType(Advice.OnMethodEnter.class); private static boolean isEnterAdvice(MethodNode source) { @@ -554,105 +577,109 @@ public void visitCode() { } private static void instrument( - TransformationContext context, MethodNode methodNode, ClassVisitor classVisitor) { + TransformationContext context, MethodNode methodNode, ClassVisitor classVisitor, boolean justDelegateAdvice) { + String originalDescriptor = methodNode.desc; String[] exceptionsArray = methodNode.exceptions.toArray(new String[0]); - List writableArguments = getWritableArguments(methodNode); - OutputArgument writableReturn = getWritableReturnValue(methodNode); - OutputArgument enterArgument = getEnterArgument(methodNode); - List adviceLocals = getLocals(methodNode); - boolean isEnterAdvice = isEnterAdvice(methodNode); - boolean isExitAdvice = isExitAdvice(methodNode); - Type returnType = Type.getReturnType(methodNode.desc); - - // currently we don't support rewriting enter advice returning a primitive type - if (isEnterAdvice - && !(returnType.getSort() == Type.VOID - || returnType.getSort() == Type.OBJECT - || returnType.getSort() == Type.ARRAY)) { - context.disableReturnTypeChange(); - } - // context is shared by enter and exit advice, if entry advice was rejected don't attempt to - // rewrite usages of @Advice.Enter in the exit advice - if (!context.canChangeReturnType()) { - enterArgument = null; - } + if (!justDelegateAdvice) { + + List writableArguments = getWritableArguments(methodNode); + OutputArgument writableReturn = getWritableReturnValue(methodNode); + OutputArgument enterArgument = getEnterArgument(methodNode); + List adviceLocals = getLocals(methodNode); + boolean isEnterAdvice = isEnterAdvice(methodNode); + boolean isExitAdvice = isExitAdvice(methodNode); + Type returnType = Type.getReturnType(methodNode.desc); + + // currently we don't support rewriting enter advice returning a primitive type + if (isEnterAdvice + && !(returnType.getSort() == Type.VOID + || returnType.getSort() == Type.OBJECT + || returnType.getSort() == Type.ARRAY)) { + context.disableReturnTypeChange(); + } + // context is shared by enter and exit advice, if entry advice was rejected don't attempt to + // rewrite usages of @Advice.Enter in the exit advice + if (!context.canChangeReturnType()) { + enterArgument = null; + } - if (context.canChangeReturnType() || (isExitAdvice && Type.VOID_TYPE.equals(returnType))) { - if (!writableArguments.isEmpty() - || writableReturn != null - || !Type.VOID_TYPE.equals(returnType) - || (!adviceLocals.isEmpty() && isEnterAdvice)) { - Type[] argumentTypes = Type.getArgumentTypes(methodNode.desc); - if (!adviceLocals.isEmpty() && isEnterAdvice) { - // Set type of arguments annotated with @Advice.Local to Object. These arguments are - // likely to be helper classes which currently breaks because the invokedynamic call in - // advised class needs access to the parameter types of the advice method. - for (AdviceLocal adviceLocal : adviceLocals) { - argumentTypes[adviceLocal.adviceIndex] = OBJECT_TYPE; + if (context.canChangeReturnType() || (isExitAdvice && Type.VOID_TYPE.equals(returnType))) { + if (!writableArguments.isEmpty() + || writableReturn != null + || !Type.VOID_TYPE.equals(returnType) + || (!adviceLocals.isEmpty() && isEnterAdvice)) { + Type[] argumentTypes = Type.getArgumentTypes(methodNode.desc); + if (!adviceLocals.isEmpty() && isEnterAdvice) { + // Set type of arguments annotated with @Advice.Local to Object. These arguments are + // likely to be helper classes which currently breaks because the invokedynamic call in + // advised class needs access to the parameter types of the advice method. + for (AdviceLocal adviceLocal : adviceLocals) { + argumentTypes[adviceLocal.adviceIndex] = OBJECT_TYPE; + } } - } - methodNode.desc = Type.getMethodDescriptor(OBJECT_ARRAY_TYPE, argumentTypes); - - MethodNode tmp = - new MethodNode( - methodNode.access, - methodNode.name, - methodNode.desc, - methodNode.signature, - exceptionsArray); - MethodVisitor mv = - instrumentOurParameters( - context, - tmp, - methodNode, - originalDescriptor, - writableArguments, - writableReturn, - adviceLocals); - methodNode.accept(mv); - - methodNode = tmp; - adviceLocals = getLocals(methodNode); - } - - // this is the only transformation that does not change the return type of the advice method, - // thus it is also the only transformation that can be applied on top of the other transforms - if ((!adviceLocals.isEmpty() || enterArgument != null) && isExitAdvice) { - // Set type of arguments annotated with @Advice.Local to Object. These arguments are likely - // to be helper classes which currently breaks because the invokedynamic call in advised - // class needs access to the parameter types of the advice method. - Type[] newArgumentTypes = Type.getArgumentTypes(methodNode.desc); - for (AdviceLocal adviceLocal : adviceLocals) { - newArgumentTypes[adviceLocal.adviceIndex] = OBJECT_TYPE; + methodNode.desc = Type.getMethodDescriptor(OBJECT_ARRAY_TYPE, argumentTypes); + + MethodNode tmp = + new MethodNode( + methodNode.access, + methodNode.name, + methodNode.desc, + methodNode.signature, + exceptionsArray); + MethodVisitor mv = + instrumentOurParameters( + context, + tmp, + methodNode, + originalDescriptor, + writableArguments, + writableReturn, + adviceLocals); + methodNode.accept(mv); + + methodNode = tmp; + adviceLocals = getLocals(methodNode); } - if (enterArgument != null) { - newArgumentTypes[enterArgument.adviceIndex] = OBJECT_TYPE; + + // this is the only transformation that does not change the return type of the advice method, + // thus it is also the only transformation that can be applied on top of the other transforms + if ((!adviceLocals.isEmpty() || enterArgument != null) && isExitAdvice) { + // Set type of arguments annotated with @Advice.Local to Object. These arguments are likely + // to be helper classes which currently breaks because the invokedynamic call in advised + // class needs access to the parameter types of the advice method. + Type[] newArgumentTypes = Type.getArgumentTypes(methodNode.desc); + for (AdviceLocal adviceLocal : adviceLocals) { + newArgumentTypes[adviceLocal.adviceIndex] = OBJECT_TYPE; + } + if (enterArgument != null) { + newArgumentTypes[enterArgument.adviceIndex] = OBJECT_TYPE; + } + List typeList = new ArrayList<>(Arrays.asList(newArgumentTypes)); + // add Object array as the last argument, this array is used to pass info from the enter + // advice + typeList.add(OBJECT_ARRAY_TYPE); + + methodNode.desc = + Type.getMethodDescriptor( + Type.getReturnType(methodNode.desc), typeList.toArray(new Type[0])); + + MethodNode tmp = + new MethodNode( + methodNode.access, + methodNode.name, + methodNode.desc, + methodNode.signature, + exceptionsArray); + MethodVisitor mv = + instrumentAdviceLocals( + false, tmp, methodNode, originalDescriptor, adviceLocals, enterArgument, -1); + methodNode.accept(mv); + + methodNode = tmp; } - List typeList = new ArrayList<>(Arrays.asList(newArgumentTypes)); - // add Object array as the last argument, this array is used to pass info from the enter - // advice - typeList.add(OBJECT_ARRAY_TYPE); - - methodNode.desc = - Type.getMethodDescriptor( - Type.getReturnType(methodNode.desc), typeList.toArray(new Type[0])); - - MethodNode tmp = - new MethodNode( - methodNode.access, - methodNode.name, - methodNode.desc, - methodNode.signature, - exceptionsArray); - MethodVisitor mv = - instrumentAdviceLocals( - false, tmp, methodNode, originalDescriptor, adviceLocals, enterArgument, -1); - methodNode.accept(mv); - - methodNode = tmp; } } From 3ffda00f34517eb7903bfc247d6dc6d18f299790 Mon Sep 17 00:00:00 2001 From: Cesar Munoz <56847527+LikeTheSalad@users.noreply.github.com> Date: Tue, 2 Jul 2024 17:00:39 +0200 Subject: [PATCH 12/20] Adjusting influxdb instrumentation to work as indy and not indy --- .../v2_4/InfluxDbImplInstrumentation.java | 15 +++++++-------- .../instrumentation/indy/AdviceTransformer.java | 8 +++++--- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbImplInstrumentation.java b/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbImplInstrumentation.java index 6004b9544064..eaeb598e14e3 100644 --- a/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbImplInstrumentation.java +++ b/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbImplInstrumentation.java @@ -76,12 +76,12 @@ public static Object[] onEnter( @Advice.FieldValue(value = "retrofit") Retrofit retrofit) { CallDepth callDepth = CallDepth.forClass(InfluxDBImpl.class); if (callDepth.getAndIncrement() > 0) { - return arguments; + return new Object[] {arguments}; } Query query = arguments[0] instanceof Query ? (Query) arguments[0] : null; if (query == null) { - return arguments; + return new Object[] {arguments}; } Context parentContext = currentContext(); @@ -92,19 +92,18 @@ public static Object[] onEnter( Instrumenter instrumenter = instrumenter(); if (!instrumenter.shouldStart(parentContext, influxDbRequest)) { - return arguments; + return new Object[] {arguments}; } // wrap callbacks so they'd run in the context of the parent span - Object[] newArguments = new Object[arguments.length + 1]; + Object[] newArguments = new Object[arguments.length]; for (int i = 0; i < arguments.length; i++) { newArguments[i] = InfluxDbObjetWrapper.wrap(arguments[i], parentContext); } - newArguments[arguments.length] = - InfluxDbScope.start(instrumenter, parentContext, influxDbRequest); - - return newArguments; + return new Object[] { + newArguments, InfluxDbScope.start(instrumenter, parentContext, influxDbRequest) + }; } @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/AdviceTransformer.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/AdviceTransformer.java index 43a53ca450ac..2c4fe02280e7 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/AdviceTransformer.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/AdviceTransformer.java @@ -50,8 +50,8 @@ static byte[] transform(byte[] bytes) { return cw.toByteArray(); } - //Advices already using Advice.AssignReturned are assumed to be already compatible - //Those won't be transformed except for setting inline to false + // Advices already using Advice.AssignReturned are assumed to be already compatible + // Those won't be transformed except for setting inline to false boolean justDelegateAdvice = usesAssignReturned(classNode); // sort enter advice method before exit advice @@ -232,11 +232,13 @@ private static List getLocals(MethodNode source) { private static final Type ADVICE_ASSIGN_RETURNED_TO_RETURNED = Type.getType(Advice.AssignReturned.ToReturned.class); private static final Type ADVICE_ASSIGN_RETURNED_TO_ARGUMENTS = Type.getType(Advice.AssignReturned.ToArguments.class); private static final Type ADVICE_ASSIGN_RETURNED_TO_FIELDS = Type.getType(Advice.AssignReturned.ToFields.class); + private static final Type ADVICE_ASSIGN_RETURNED_TO_ALL_ARGUMENTS = Type.getType(Advice.AssignReturned.ToAllArguments.class); private static boolean usesAssignReturned(MethodNode source) { return hasAnnotation(source, ADVICE_ASSIGN_RETURNED_TO_RETURNED) || hasAnnotation(source, ADVICE_ASSIGN_RETURNED_TO_ARGUMENTS) - || hasAnnotation(source, ADVICE_ASSIGN_RETURNED_TO_FIELDS); + || hasAnnotation(source, ADVICE_ASSIGN_RETURNED_TO_FIELDS) + || hasAnnotation(source, ADVICE_ASSIGN_RETURNED_TO_ALL_ARGUMENTS); } private static boolean usesAssignReturned(ClassNode classNode) { From 30dbf5cb169955a43b5265c0d7d334e4a2529b7a Mon Sep 17 00:00:00 2001 From: Cesar Munoz <56847527+LikeTheSalad@users.noreply.github.com> Date: Tue, 2 Jul 2024 17:24:13 +0200 Subject: [PATCH 13/20] Spotless --- .../instrumentation/TypeTransformerImpl.java | 5 +-- .../indy/AdviceTransformer.java | 36 ++++++++++++------- 2 files changed, 26 insertions(+), 15 deletions(-) diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/TypeTransformerImpl.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/TypeTransformerImpl.java index 20c7a9f4f1c7..1382acc66e2d 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/TypeTransformerImpl.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/TypeTransformerImpl.java @@ -19,8 +19,9 @@ final class TypeTransformerImpl implements TypeTransformer { TypeTransformerImpl(AgentBuilder.Identified.Extendable agentBuilder) { this.agentBuilder = agentBuilder; - adviceMapping = Advice.withCustomMapping() - .with(new Advice.AssignReturned.Factory().withSuppressed(Throwable.class)); + adviceMapping = + Advice.withCustomMapping() + .with(new Advice.AssignReturned.Factory().withSuppressed(Throwable.class)); } @Override diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/AdviceTransformer.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/AdviceTransformer.java index 2c4fe02280e7..c8bea8b42ade 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/AdviceTransformer.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/AdviceTransformer.java @@ -229,16 +229,20 @@ private static List getLocals(MethodNode source) { return result; } - private static final Type ADVICE_ASSIGN_RETURNED_TO_RETURNED = Type.getType(Advice.AssignReturned.ToReturned.class); - private static final Type ADVICE_ASSIGN_RETURNED_TO_ARGUMENTS = Type.getType(Advice.AssignReturned.ToArguments.class); - private static final Type ADVICE_ASSIGN_RETURNED_TO_FIELDS = Type.getType(Advice.AssignReturned.ToFields.class); - private static final Type ADVICE_ASSIGN_RETURNED_TO_ALL_ARGUMENTS = Type.getType(Advice.AssignReturned.ToAllArguments.class); + private static final Type ADVICE_ASSIGN_RETURNED_TO_RETURNED = + Type.getType(Advice.AssignReturned.ToReturned.class); + private static final Type ADVICE_ASSIGN_RETURNED_TO_ARGUMENTS = + Type.getType(Advice.AssignReturned.ToArguments.class); + private static final Type ADVICE_ASSIGN_RETURNED_TO_FIELDS = + Type.getType(Advice.AssignReturned.ToFields.class); + private static final Type ADVICE_ASSIGN_RETURNED_TO_ALL_ARGUMENTS = + Type.getType(Advice.AssignReturned.ToAllArguments.class); private static boolean usesAssignReturned(MethodNode source) { return hasAnnotation(source, ADVICE_ASSIGN_RETURNED_TO_RETURNED) - || hasAnnotation(source, ADVICE_ASSIGN_RETURNED_TO_ARGUMENTS) - || hasAnnotation(source, ADVICE_ASSIGN_RETURNED_TO_FIELDS) - || hasAnnotation(source, ADVICE_ASSIGN_RETURNED_TO_ALL_ARGUMENTS); + || hasAnnotation(source, ADVICE_ASSIGN_RETURNED_TO_ARGUMENTS) + || hasAnnotation(source, ADVICE_ASSIGN_RETURNED_TO_FIELDS) + || hasAnnotation(source, ADVICE_ASSIGN_RETURNED_TO_ALL_ARGUMENTS); } private static boolean usesAssignReturned(ClassNode classNode) { @@ -579,7 +583,10 @@ public void visitCode() { } private static void instrument( - TransformationContext context, MethodNode methodNode, ClassVisitor classVisitor, boolean justDelegateAdvice) { + TransformationContext context, + MethodNode methodNode, + ClassVisitor classVisitor, + boolean justDelegateAdvice) { String originalDescriptor = methodNode.desc; String[] exceptionsArray = methodNode.exceptions.toArray(new String[0]); @@ -597,8 +604,8 @@ private static void instrument( // currently we don't support rewriting enter advice returning a primitive type if (isEnterAdvice && !(returnType.getSort() == Type.VOID - || returnType.getSort() == Type.OBJECT - || returnType.getSort() == Type.ARRAY)) { + || returnType.getSort() == Type.OBJECT + || returnType.getSort() == Type.ARRAY)) { context.disableReturnTypeChange(); } // context is shared by enter and exit advice, if entry advice was rejected don't attempt to @@ -646,10 +653,13 @@ private static void instrument( adviceLocals = getLocals(methodNode); } - // this is the only transformation that does not change the return type of the advice method, - // thus it is also the only transformation that can be applied on top of the other transforms + // this is the only transformation that does not change the return type of the advice + // method, + // thus it is also the only transformation that can be applied on top of the other + // transforms if ((!adviceLocals.isEmpty() || enterArgument != null) && isExitAdvice) { - // Set type of arguments annotated with @Advice.Local to Object. These arguments are likely + // Set type of arguments annotated with @Advice.Local to Object. These arguments are + // likely // to be helper classes which currently breaks because the invokedynamic call in advised // class needs access to the parameter types of the advice method. Type[] newArgumentTypes = Type.getArgumentTypes(methodNode.desc); From bfbb55d9ca03ef976baf5e5e0e95ac329a66ccde Mon Sep 17 00:00:00 2001 From: Cesar Munoz <56847527+LikeTheSalad@users.noreply.github.com> Date: Wed, 7 Aug 2024 14:38:48 +0200 Subject: [PATCH 14/20] Clean up after conflicts --- .../tooling/instrumentation/indy/AdviceTransformer.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/AdviceTransformer.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/AdviceTransformer.java index 49992e34a159..2f0c83726003 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/AdviceTransformer.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/AdviceTransformer.java @@ -255,8 +255,6 @@ private static boolean usesAssignReturned(ClassNode classNode) { return false; } - private static final Type ADVICE_ON_METHOD_ENTER = Type.getType(Advice.OnMethodEnter.class); - private static boolean isEnterAdvice(MethodNode source) { return hasAnnotation(source, ADVICE_ON_METHOD_ENTER); } From f1121ec26fa8471c20836db5ed2a90e3629e993b Mon Sep 17 00:00:00 2001 From: Cesar Munoz <56847527+LikeTheSalad@users.noreply.github.com> Date: Wed, 7 Aug 2024 14:46:37 +0200 Subject: [PATCH 15/20] Returning null when span isn't started --- .../influxdb/v2_4/InfluxDbImplInstrumentation.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbImplInstrumentation.java b/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbImplInstrumentation.java index eaeb598e14e3..6e7d728a9574 100644 --- a/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbImplInstrumentation.java +++ b/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbImplInstrumentation.java @@ -76,12 +76,12 @@ public static Object[] onEnter( @Advice.FieldValue(value = "retrofit") Retrofit retrofit) { CallDepth callDepth = CallDepth.forClass(InfluxDBImpl.class); if (callDepth.getAndIncrement() > 0) { - return new Object[] {arguments}; + return null; } Query query = arguments[0] instanceof Query ? (Query) arguments[0] : null; if (query == null) { - return new Object[] {arguments}; + return null; } Context parentContext = currentContext(); @@ -92,7 +92,7 @@ public static Object[] onEnter( Instrumenter instrumenter = instrumenter(); if (!instrumenter.shouldStart(parentContext, influxDbRequest)) { - return new Object[] {arguments}; + return null; } // wrap callbacks so they'd run in the context of the parent span From 842749f7e3031a5de08c473fc73b085c259f90b0 Mon Sep 17 00:00:00 2001 From: Cesar Munoz <56847527+LikeTheSalad@users.noreply.github.com> Date: Wed, 7 Aug 2024 14:51:11 +0200 Subject: [PATCH 16/20] Returning InfluxDbScope instead of Object --- .../influxdb/v2_4/InfluxDbImplInstrumentation.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbImplInstrumentation.java b/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbImplInstrumentation.java index 6e7d728a9574..f93e7f971bfd 100644 --- a/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbImplInstrumentation.java +++ b/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbImplInstrumentation.java @@ -124,7 +124,7 @@ public static void onExit( public static class InfluxDbModifyAdvice { @Advice.OnMethodEnter(suppress = Throwable.class) - public static Object onEnter( + public static InfluxDbScope onEnter( @Advice.Origin("#m") String methodName, @Advice.Argument(0) Object arg0, @Advice.FieldValue(value = "retrofit") Retrofit retrofit) { From 71d6922c69bee4bfad69e0bd975e95d358017ef6 Mon Sep 17 00:00:00 2001 From: Cesar Munoz <56847527+LikeTheSalad@users.noreply.github.com> Date: Wed, 7 Aug 2024 15:00:10 +0200 Subject: [PATCH 17/20] Applying PR suggestions on InfluxDbScope.end method and its usage --- .../v2_4/InfluxDbImplInstrumentation.java | 13 ++++++++----- .../influxdb/v2_4/InfluxDbScope.java | 16 ++++++---------- 2 files changed, 14 insertions(+), 15 deletions(-) diff --git a/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbImplInstrumentation.java b/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbImplInstrumentation.java index f93e7f971bfd..ff3943c737c5 100644 --- a/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbImplInstrumentation.java +++ b/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbImplInstrumentation.java @@ -114,9 +114,9 @@ public static void onExit( return; } - Object extraObject = enterArgs[enterArgs.length - 1]; - - InfluxDbScope.end(extraObject, instrumenter(), throwable); + if (enterArgs != null && enterArgs.length >= 2 && enterArgs[1] instanceof InfluxDbScope) { + ((InfluxDbScope) enterArgs[1]).end(throwable); + } } } @@ -167,12 +167,15 @@ public static InfluxDbScope onEnter( } @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) - public static void onExit(@Advice.Thrown Throwable throwable, @Advice.Enter Object scope) { + public static void onExit( + @Advice.Thrown Throwable throwable, @Advice.Enter InfluxDbScope scope) { CallDepth callDepth = CallDepth.forClass(InfluxDBImpl.class); if (callDepth.decrementAndGet() > 0) { return; } - InfluxDbScope.end(scope, instrumenter(), throwable); + if (scope != null) { + scope.end(throwable); + } } } } diff --git a/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbScope.java b/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbScope.java index 0109c7ea2d94..c84c549d56bc 100644 --- a/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbScope.java +++ b/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbScope.java @@ -9,6 +9,8 @@ import io.opentelemetry.context.Scope; import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter; +import static io.opentelemetry.javaagent.instrumentation.influxdb.v2_4.InfluxDbSingletons.instrumenter; + /** Container used to carry state between enter and exit advices */ public class InfluxDbScope { private final InfluxDbRequest influxDbRequest; @@ -29,19 +31,13 @@ public static InfluxDbScope start( return new InfluxDbScope(influxDbRequest, context, context.makeCurrent()); } - public static void end( - Object scope, Instrumenter instrumenter, Throwable throwable) { - if (!(scope instanceof InfluxDbScope)) { - return; - } - InfluxDbScope influxDbScope = (InfluxDbScope) scope; - - if (influxDbScope.scope == null) { + public void end(Throwable throwable) { + if (scope == null) { return; } - influxDbScope.scope.close(); + scope.close(); - instrumenter.end(influxDbScope.context, influxDbScope.influxDbRequest, null, throwable); + instrumenter().end(context, influxDbRequest, null, throwable); } } From 6e1dba7e5b61c4d81aa25915ce24cfa07e08e292 Mon Sep 17 00:00:00 2001 From: Cesar Munoz <56847527+LikeTheSalad@users.noreply.github.com> Date: Wed, 7 Aug 2024 15:19:08 +0200 Subject: [PATCH 18/20] Creating and calling helper method when instrumentation is only for delegating advice --- .../indy/AdviceTransformer.java | 212 +++++++++--------- 1 file changed, 109 insertions(+), 103 deletions(-) diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/AdviceTransformer.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/AdviceTransformer.java index 2f0c83726003..6c8aa5a75013 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/AdviceTransformer.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/AdviceTransformer.java @@ -78,8 +78,12 @@ public MethodVisitor visitMethod( @Override public void visitEnd() { super.visitEnd(); - - instrument(context, this, classVisitor, justDelegateAdvice); + if (justDelegateAdvice) { + applyAdviceDelegation( + context, this, classVisitor, exceptions.toArray(new String[0])); + } else { + fullyInstrument(context, this, classVisitor); + } } }; } @@ -581,119 +585,121 @@ public void visitCode() { return ga; } - private static void instrument( - TransformationContext context, - MethodNode methodNode, - ClassVisitor classVisitor, - boolean justDelegateAdvice) { + private static void fullyInstrument( + TransformationContext context, MethodNode methodNode, ClassVisitor classVisitor) { String originalDescriptor = methodNode.desc; String[] exceptionsArray = methodNode.exceptions.toArray(new String[0]); - if (!justDelegateAdvice) { - - List writableArguments = getWritableArguments(methodNode); - OutputArgument writableReturn = getWritableReturnValue(methodNode); - OutputArgument enterArgument = getEnterArgument(methodNode); - List adviceLocals = getLocals(methodNode); - boolean isEnterAdvice = isEnterAdvice(methodNode); - boolean isExitAdvice = isExitAdvice(methodNode); - Type returnType = Type.getReturnType(methodNode.desc); - - // currently we don't support rewriting enter advice returning a primitive type - if (isEnterAdvice - && !(returnType.getSort() == Type.VOID - || returnType.getSort() == Type.OBJECT - || returnType.getSort() == Type.ARRAY)) { - context.disableReturnTypeChange(); - } - // context is shared by enter and exit advice, if entry advice was rejected don't attempt to - // rewrite usages of @Advice.Enter in the exit advice - if (!context.canChangeReturnType()) { - enterArgument = null; - } - - if (context.canChangeReturnType() || (isExitAdvice && Type.VOID_TYPE.equals(returnType))) { - if (!writableArguments.isEmpty() - || writableReturn != null - || !Type.VOID_TYPE.equals(returnType) - || (!adviceLocals.isEmpty() && isEnterAdvice)) { - Type[] argumentTypes = Type.getArgumentTypes(methodNode.desc); - if (!adviceLocals.isEmpty() && isEnterAdvice) { - // Set type of arguments annotated with @Advice.Local to Object. These arguments are - // likely to be helper classes which currently breaks because the invokedynamic call in - // advised class needs access to the parameter types of the advice method. - for (AdviceLocal adviceLocal : adviceLocals) { - argumentTypes[adviceLocal.adviceIndex] = OBJECT_TYPE; - } - } - - methodNode.desc = Type.getMethodDescriptor(OBJECT_ARRAY_TYPE, argumentTypes); - - MethodNode tmp = - new MethodNode( - methodNode.access, - methodNode.name, - methodNode.desc, - methodNode.signature, - exceptionsArray); - MethodVisitor mv = - instrumentOurParameters( - context, - tmp, - methodNode, - originalDescriptor, - writableArguments, - writableReturn, - adviceLocals); - methodNode.accept(mv); - - methodNode = tmp; - adviceLocals = getLocals(methodNode); - } + List writableArguments = getWritableArguments(methodNode); + OutputArgument writableReturn = getWritableReturnValue(methodNode); + OutputArgument enterArgument = getEnterArgument(methodNode); + List adviceLocals = getLocals(methodNode); + boolean isEnterAdvice = isEnterAdvice(methodNode); + boolean isExitAdvice = isExitAdvice(methodNode); + Type returnType = Type.getReturnType(methodNode.desc); + + // currently we don't support rewriting enter advice returning a primitive type + if (isEnterAdvice + && !(returnType.getSort() == Type.VOID + || returnType.getSort() == Type.OBJECT + || returnType.getSort() == Type.ARRAY)) { + context.disableReturnTypeChange(); + } + // context is shared by enter and exit advice, if entry advice was rejected don't attempt to + // rewrite usages of @Advice.Enter in the exit advice + if (!context.canChangeReturnType()) { + enterArgument = null; + } - // this is the only transformation that does not change the return type of the advice - // method, - // thus it is also the only transformation that can be applied on top of the other - // transforms - if ((!adviceLocals.isEmpty() || enterArgument != null) && isExitAdvice) { + if (context.canChangeReturnType() || (isExitAdvice && Type.VOID_TYPE.equals(returnType))) { + if (!writableArguments.isEmpty() + || writableReturn != null + || !Type.VOID_TYPE.equals(returnType) + || (!adviceLocals.isEmpty() && isEnterAdvice)) { + Type[] argumentTypes = Type.getArgumentTypes(methodNode.desc); + if (!adviceLocals.isEmpty() && isEnterAdvice) { // Set type of arguments annotated with @Advice.Local to Object. These arguments are - // likely - // to be helper classes which currently breaks because the invokedynamic call in advised - // class needs access to the parameter types of the advice method. - Type[] newArgumentTypes = Type.getArgumentTypes(methodNode.desc); + // likely to be helper classes which currently breaks because the invokedynamic call in + // advised class needs access to the parameter types of the advice method. for (AdviceLocal adviceLocal : adviceLocals) { - newArgumentTypes[adviceLocal.adviceIndex] = OBJECT_TYPE; - } - if (enterArgument != null) { - newArgumentTypes[enterArgument.adviceIndex] = OBJECT_TYPE; + argumentTypes[adviceLocal.adviceIndex] = OBJECT_TYPE; } - List typeList = new ArrayList<>(Arrays.asList(newArgumentTypes)); - // add Object array as the last argument, this array is used to pass info from the enter - // advice - typeList.add(OBJECT_ARRAY_TYPE); - - methodNode.desc = - Type.getMethodDescriptor( - Type.getReturnType(methodNode.desc), typeList.toArray(new Type[0])); - - MethodNode tmp = - new MethodNode( - methodNode.access, - methodNode.name, - methodNode.desc, - methodNode.signature, - exceptionsArray); - MethodVisitor mv = - instrumentAdviceLocals( - false, tmp, methodNode, originalDescriptor, adviceLocals, enterArgument, -1); - methodNode.accept(mv); - - methodNode = tmp; } + + methodNode.desc = Type.getMethodDescriptor(OBJECT_ARRAY_TYPE, argumentTypes); + + MethodNode tmp = + new MethodNode( + methodNode.access, + methodNode.name, + methodNode.desc, + methodNode.signature, + exceptionsArray); + MethodVisitor mv = + instrumentOurParameters( + context, + tmp, + methodNode, + originalDescriptor, + writableArguments, + writableReturn, + adviceLocals); + methodNode.accept(mv); + + methodNode = tmp; + adviceLocals = getLocals(methodNode); + } + + // this is the only transformation that does not change the return type of the advice + // method, + // thus it is also the only transformation that can be applied on top of the other + // transforms + if ((!adviceLocals.isEmpty() || enterArgument != null) && isExitAdvice) { + // Set type of arguments annotated with @Advice.Local to Object. These arguments are + // likely + // to be helper classes which currently breaks because the invokedynamic call in advised + // class needs access to the parameter types of the advice method. + Type[] newArgumentTypes = Type.getArgumentTypes(methodNode.desc); + for (AdviceLocal adviceLocal : adviceLocals) { + newArgumentTypes[adviceLocal.adviceIndex] = OBJECT_TYPE; + } + if (enterArgument != null) { + newArgumentTypes[enterArgument.adviceIndex] = OBJECT_TYPE; + } + List typeList = new ArrayList<>(Arrays.asList(newArgumentTypes)); + // add Object array as the last argument, this array is used to pass info from the enter + // advice + typeList.add(OBJECT_ARRAY_TYPE); + + methodNode.desc = + Type.getMethodDescriptor( + Type.getReturnType(methodNode.desc), typeList.toArray(new Type[0])); + + MethodNode tmp = + new MethodNode( + methodNode.access, + methodNode.name, + methodNode.desc, + methodNode.signature, + exceptionsArray); + MethodVisitor mv = + instrumentAdviceLocals( + false, tmp, methodNode, originalDescriptor, adviceLocals, enterArgument, -1); + methodNode.accept(mv); + + methodNode = tmp; } } + applyAdviceDelegation(context, methodNode, classVisitor, exceptionsArray); + } + + private static void applyAdviceDelegation( + TransformationContext context, + MethodNode methodNode, + ClassVisitor classVisitor, + String[] exceptionsArray) { MethodVisitor mv = classVisitor.visitMethod( methodNode.access, From e45413361eb377fe5994096fc65ac3f524974d72 Mon Sep 17 00:00:00 2001 From: Cesar Munoz <56847527+LikeTheSalad@users.noreply.github.com> Date: Thu, 8 Aug 2024 09:54:56 +0200 Subject: [PATCH 19/20] Spotless --- .../instrumentation/influxdb/v2_4/InfluxDbScope.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbScope.java b/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbScope.java index c84c549d56bc..5b9304251971 100644 --- a/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbScope.java +++ b/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbScope.java @@ -5,12 +5,12 @@ package io.opentelemetry.javaagent.instrumentation.influxdb.v2_4; +import static io.opentelemetry.javaagent.instrumentation.influxdb.v2_4.InfluxDbSingletons.instrumenter; + import io.opentelemetry.context.Context; import io.opentelemetry.context.Scope; import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter; -import static io.opentelemetry.javaagent.instrumentation.influxdb.v2_4.InfluxDbSingletons.instrumenter; - /** Container used to carry state between enter and exit advices */ public class InfluxDbScope { private final InfluxDbRequest influxDbRequest; From 97f01103cc964e27088a01a5aa492867a5724914 Mon Sep 17 00:00:00 2001 From: Lauri Tulmin Date: Fri, 9 Aug 2024 15:28:34 +0300 Subject: [PATCH 20/20] simplify --- .../v2_4/InfluxDbImplInstrumentation.java | 26 +++++++------------ .../influxdb/v2_4/InfluxDbScope.java | 10 +++---- .../indy/AdviceTransformer.java | 14 ++++------ 3 files changed, 17 insertions(+), 33 deletions(-) diff --git a/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbImplInstrumentation.java b/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbImplInstrumentation.java index ff3943c737c5..72e5ee14bb5f 100644 --- a/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbImplInstrumentation.java +++ b/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbImplInstrumentation.java @@ -15,7 +15,6 @@ import static net.bytebuddy.matcher.ElementMatchers.takesArguments; import io.opentelemetry.context.Context; -import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter; import io.opentelemetry.javaagent.bootstrap.CallDepth; import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation; import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer; @@ -90,8 +89,7 @@ public static Object[] onEnter( InfluxDbRequest.create( httpUrl.host(), httpUrl.port(), query.getDatabase(), null, query.getCommand()); - Instrumenter instrumenter = instrumenter(); - if (!instrumenter.shouldStart(parentContext, influxDbRequest)) { + if (!instrumenter().shouldStart(parentContext, influxDbRequest)) { return null; } @@ -101,22 +99,18 @@ public static Object[] onEnter( newArguments[i] = InfluxDbObjetWrapper.wrap(arguments[i], parentContext); } - return new Object[] { - newArguments, InfluxDbScope.start(instrumenter, parentContext, influxDbRequest) - }; + return new Object[] {newArguments, InfluxDbScope.start(parentContext, influxDbRequest)}; } @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) public static void onExit( @Advice.Thrown Throwable throwable, @Advice.Enter Object[] enterArgs) { CallDepth callDepth = CallDepth.forClass(InfluxDBImpl.class); - if (callDepth.decrementAndGet() > 0) { + if (callDepth.decrementAndGet() > 0 || enterArgs == null) { return; } - if (enterArgs != null && enterArgs.length >= 2 && enterArgs[1] instanceof InfluxDbScope) { - ((InfluxDbScope) enterArgs[1]).end(throwable); - } + ((InfluxDbScope) enterArgs[1]).end(throwable); } } @@ -158,24 +152,22 @@ public static InfluxDbScope onEnter( InfluxDbRequest influxDbRequest = InfluxDbRequest.create(httpUrl.host(), httpUrl.port(), database, operation, null); - Instrumenter instrumenter = instrumenter(); - if (!instrumenter.shouldStart(parentContext, influxDbRequest)) { + if (!instrumenter().shouldStart(parentContext, influxDbRequest)) { return null; } - return InfluxDbScope.start(instrumenter, parentContext, influxDbRequest); + return InfluxDbScope.start(parentContext, influxDbRequest); } @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) public static void onExit( @Advice.Thrown Throwable throwable, @Advice.Enter InfluxDbScope scope) { CallDepth callDepth = CallDepth.forClass(InfluxDBImpl.class); - if (callDepth.decrementAndGet() > 0) { + if (callDepth.decrementAndGet() > 0 || scope == null) { return; } - if (scope != null) { - scope.end(throwable); - } + + scope.end(throwable); } } } diff --git a/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbScope.java b/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbScope.java index 5b9304251971..6daaea80f727 100644 --- a/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbScope.java +++ b/instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbScope.java @@ -9,10 +9,9 @@ import io.opentelemetry.context.Context; import io.opentelemetry.context.Scope; -import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter; /** Container used to carry state between enter and exit advices */ -public class InfluxDbScope { +public final class InfluxDbScope { private final InfluxDbRequest influxDbRequest; private final Context context; private final Scope scope; @@ -23,11 +22,8 @@ private InfluxDbScope(InfluxDbRequest influxDbRequest, Context context, Scope sc this.scope = scope; } - public static InfluxDbScope start( - Instrumenter instrumenter, - Context parentContext, - InfluxDbRequest influxDbRequest) { - Context context = instrumenter.start(parentContext, influxDbRequest); + public static InfluxDbScope start(Context parentContext, InfluxDbRequest influxDbRequest) { + Context context = instrumenter().start(parentContext, influxDbRequest); return new InfluxDbScope(influxDbRequest, context, context.makeCurrent()); } diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/AdviceTransformer.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/AdviceTransformer.java index 6c8aa5a75013..ac1f46a99e3a 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/AdviceTransformer.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/AdviceTransformer.java @@ -82,7 +82,7 @@ public void visitEnd() { applyAdviceDelegation( context, this, classVisitor, exceptions.toArray(new String[0])); } else { - fullyInstrument(context, this, classVisitor); + instrument(context, this, classVisitor); } } }; @@ -585,9 +585,8 @@ public void visitCode() { return ga; } - private static void fullyInstrument( + private static void instrument( TransformationContext context, MethodNode methodNode, ClassVisitor classVisitor) { - String originalDescriptor = methodNode.desc; String[] exceptionsArray = methodNode.exceptions.toArray(new String[0]); @@ -651,13 +650,10 @@ private static void fullyInstrument( adviceLocals = getLocals(methodNode); } - // this is the only transformation that does not change the return type of the advice - // method, - // thus it is also the only transformation that can be applied on top of the other - // transforms + // this is the only transformation that does not change the return type of the advice method, + // thus it is also the only transformation that can be applied on top of the other transforms if ((!adviceLocals.isEmpty() || enterArgument != null) && isExitAdvice) { - // Set type of arguments annotated with @Advice.Local to Object. These arguments are - // likely + // Set type of arguments annotated with @Advice.Local to Object. These arguments are likely // to be helper classes which currently breaks because the invokedynamic call in advised // class needs access to the parameter types of the advice method. Type[] newArgumentTypes = Type.getArgumentTypes(methodNode.desc);