Code review agent: Establish AdviceScope pattern (open-telemetry#17419)

Author: trask

.github/agents/knowledge/javaagent-advice-patterns.md

@@ -172,6 +172,95 @@ Because `none()` matches no methods, ByteBuddy never inlines this advice into an
 `suppress = Throwable.class` on such a method is entirely meaningless — **do not add it**,
 and **do not flag its absence** during review.
 
+## AdviceScope Patterns
+
+`AdviceScope` usage in this repository falls into **two justified state patterns**.
+Review new code against these patterns instead of treating every existing variation as equally
+canonical.
+
+### Pattern 1 — Nullable `AdviceScope` for ordinary advice
+
+Use this by default when enter advice may decide not to start instrumentation.
+
+- `@Advice.OnMethodEnter` returns `@Nullable AdviceScope`
+- The factory method returns `null` when preconditions fail or instrumentation should not start
+- If an `AdviceScope` is created, its `Context` and `Scope` fields should be non-null
+- The `end()` method should close `scope` unconditionally; the null check belongs in exit advice,
+  not inside `AdviceScope.end()`
+
+In this document, use `start()` / `end()` as the canonical `AdviceScope` naming.
+For new code, prefer `start()` for the factory method and `end()` for the completion method.
+Do not introduce one-off names such as `create()` for ordinary `AdviceScope` factories.
+
+Preferred shape:
+
+```java
+public static class AdviceScope {
+  private final Context context;
+  private final Scope scope;
+
+  private AdviceScope(Context context) {
+    this.context = context;
+    this.scope = context.makeCurrent();
+  }
+
+  @Nullable
+  public static AdviceScope start(Request request) {
+    Context parentContext = Context.current();
+    if (!instrumenter().shouldStart(parentContext, request)) {
+      return null;
+    }
+    Context context = instrumenter().start(parentContext, request);
+    return new AdviceScope(context);
+  }
+
+  public void end(@Nullable Throwable throwable) {
+    scope.close();
+    instrumenter().end(context, request, null, throwable);
+  }
+}
+
+@Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class)
+public static void onExit(
+    @Advice.Thrown @Nullable Throwable throwable,
+    @Advice.Enter @Nullable AdviceScope adviceScope) {
+  if (adviceScope != null) {
+    adviceScope.end(throwable);
+  }
+}
+```
+
+### Pattern 2 — Non-null placeholder `AdviceScope` for bookkeeping
+
+Use a non-null `AdviceScope` with nullable internals only when exit advice still needs state even
+if no tracing scope was started, for example:
+
+- `CallDepth` tracking
+- wrapped arguments or listeners that must be carried from enter to exit
+- other bookkeeping that must survive even on the "no span started" path
+
+In this pattern, `AdviceScope` is the carrier object for control-flow state, not just tracing
+state. Nullable inner fields and exit guards are justified here.
+
+### Do not use the defensive hybrid as a standard pattern
+
+Avoid the hybrid shape where:
+
+- `@Advice.Enter` is `@Nullable AdviceScope`
+- `AdviceScope` still stores a nullable `Scope`
+- `AdviceScope.end()` has an extra `if (scope == null)` guard
+
+That double-guards the same condition and makes simple advice harder to reason about.
+If the helper that creates the context can return `null`, prefer returning `null` from
+`AdviceScope.start()` and only creating `AdviceScope` when the context is present.
+
+### Async completion is not a separate `AdviceScope` state pattern
+
+Async instrumentations may end spans from listeners, callbacks, or wrapped handlers instead of
+directly in method exit. That affects **where** the scope/span is completed, but it does not
+justify a third core `AdviceScope` state model. Async advice should still use either Pattern 1 or
+Pattern 2 as appropriate.
+
 ## Never Throw Exceptions in Javaagent Code
 
 Javaagent instrumentations must never throw exceptions. The goal is to be invisible to the
@@ -193,3 +282,5 @@ the instrumentation automatically rather than letting it fail at runtime.
 - **`@Advice.OnMethodExit` method named `onEnter`** (or vice versa) — the method name should match the annotation. A mismatch is a copy-paste bug that compiles but confuses readers and may mask intent errors.
 - **Advice referenced in `transform()` using anything other than `getClass().getName() + "$InnerAdvice"`** — see `javaagent-module-patterns.md` for the canonical pattern. Flag `this.getClass().getName() + "$InnerAdvice"` as a redundant qualifier, and flag both `InnerAdvice.class.getName()` and `OuterInstrumentation.class.getName() + "$InnerAdvice"` because any `.class` literal in a `transform()` method triggers unwanted class loading.
 - **`onThrowable = Throwable.class` on return-only exit advice** — if the exit method only processes `@Advice.Return` and has no `@Advice.Enter` state to clean up, `onThrowable` should be omitted. The return value is `null`/zero on the exceptional path, and dereferencing it causes a suppressed exception for no benefit. Keep `suppress = Throwable.class` but remove `onThrowable`.
+- **One-off `AdviceScope` method naming** — for new ordinary advice, prefer `start()` / `end()` for `AdviceScope` methods, and avoid introducing unique names such as `create()`.
+- **Defensive hybrid `AdviceScope` in simple advice** — if `@Advice.Enter` is already nullable, flag simple advice that also stores a nullable inner `Scope`/`Context` and re-checks it inside `AdviceScope.end()`. Prefer returning `null` from the factory and keeping the created `AdviceScope` fully initialized.

instrumentation/influxdb-2.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/influxdb/v2_4/InfluxDbScope.java

@@ -28,10 +28,6 @@ public static InfluxDbScope start(Context parentContext, InfluxDbRequest influxD
   }
 
   public void end(Throwable throwable) {
-    if (scope == null) {
-      return;
-    }
-
     scope.close();
 
     instrumenter().end(context, influxDbRequest, null, throwable);

instrumentation/jaxrs/jaxrs-2.0/jaxrs-2.0-annotations/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jaxrs/v2_0/DefaultRequestContextInstrumentation.java

@@ -48,7 +48,7 @@ private AdviceScope(Context context, Scope scope, Jaxrs2HandlerData handlerData)
       }
 
       @Nullable
-      public static AdviceScope enter(Class<?> filterClass, Method method) {
+      public static AdviceScope start(Class<?> filterClass, Method method) {
 
         Context parentContext = Context.current();
         Jaxrs2HandlerData handlerData = new Jaxrs2HandlerData(filterClass, method);
@@ -66,10 +66,7 @@ public static AdviceScope enter(Class<?> filterClass, Method method) {
         return new AdviceScope(context, context.makeCurrent(), handlerData);
       }
 
-      public void exit(Throwable throwable) {
-        if (scope == null) {
-          return;
-        }
+      public void end(@Nullable Throwable throwable) {
         scope.close();
         instrumenter().end(context, handlerData, null, throwable);
       }
@@ -101,7 +98,7 @@ public static AdviceScope createGenericSpan(
       if (method == null) {
         return null;
       }
-      return AdviceScope.enter(filterClass, method);
+      return AdviceScope.start(filterClass, method);
     }
 
     @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class)
@@ -110,7 +107,7 @@ public static void stopSpan(
         @Advice.Enter @Nullable AdviceScope adviceScope) {
 
       if (adviceScope != null) {
-        adviceScope.exit(throwable);
+        adviceScope.end(throwable);
       }
     }
   }

instrumentation/jaxrs/jaxrs-2.0/jaxrs-2.0-cxf-3.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jaxrs/v2_0/CxfRequestContextInstrumentation.java

@@ -60,19 +60,26 @@ public static class AdviceScope {
       private final Context context;
       private final Scope scope;
 
-      public AdviceScope(
+      private AdviceScope(Jaxrs2HandlerData handlerData, Context context) {
+        this.handlerData = handlerData;
+        this.context = context;
+        scope = context.makeCurrent();
+      }
+
+      @Nullable
+      public static AdviceScope start(
           Class<?> resourceClass, Method method, AbstractRequestContextImpl requestContext) {
-        handlerData = new Jaxrs2HandlerData(resourceClass, method);
-        context =
+        Jaxrs2HandlerData handlerData = new Jaxrs2HandlerData(resourceClass, method);
+        Context context =
             Jaxrs2RequestContextHelper.createOrUpdateAbortSpan(
                 instrumenter(), (ContainerRequestContext) requestContext, handlerData);
-        scope = context != null ? context.makeCurrent() : null;
+        if (context == null) {
+          return null;
+        }
+        return new AdviceScope(handlerData, context);
       }
 
-      public void exit(@Nullable Throwable throwable) {
-        if (scope == null) {
-          return;
-        }
+      public void end(@Nullable Throwable throwable) {
         scope.close();
         instrumenter().end(context, handlerData, null, throwable);
       }
@@ -99,14 +106,15 @@ public static AdviceScope decorateAbortSpan(
       MethodInvocationInfo invocationInfo = resourceInfoStack.peek();
       Method method = invocationInfo.getMethodInfo().getMethodToInvoke();
       Class<?> resourceClass = invocationInfo.getRealClass();
-      return new AdviceScope(resourceClass, method, requestContext);
+      return AdviceScope.start(resourceClass, method, requestContext);
     }
 
     @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class)
     public static void stopSpan(
-        @Advice.Thrown Throwable throwable, @Advice.Enter @Nullable AdviceScope adviceScope) {
+        @Advice.Thrown @Nullable Throwable throwable,
+        @Advice.Enter @Nullable AdviceScope adviceScope) {
       if (adviceScope != null) {
-        adviceScope.exit(throwable);
+        adviceScope.end(throwable);
       }
     }
   }

instrumentation/jaxrs/jaxrs-2.0/jaxrs-2.0-resteasy-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jaxrs/v2_0/Resteasy30RequestContextInstrumentation.java

@@ -41,19 +41,26 @@ public static class AdviceScope {
       private final Context context;
       private final Scope scope;
 
-      public AdviceScope(
+      private AdviceScope(Jaxrs2HandlerData handlerData, Context context) {
+        this.handlerData = handlerData;
+        this.context = context;
+        scope = context.makeCurrent();
+      }
+
+      @Nullable
+      public static AdviceScope start(
           Class<?> resourceClass, Method method, ContainerRequestContext requestContext) {
-        handlerData = new Jaxrs2HandlerData(resourceClass, method);
-        context =
+        Jaxrs2HandlerData handlerData = new Jaxrs2HandlerData(resourceClass, method);
+        Context context =
             Jaxrs2RequestContextHelper.createOrUpdateAbortSpan(
                 instrumenter(), requestContext, handlerData);
-        scope = context != null ? context.makeCurrent() : null;
+        if (context == null) {
+          return null;
+        }
+        return new AdviceScope(handlerData, context);
       }
 
-      public void exit(Throwable throwable) {
-        if (scope == null) {
-          return;
-        }
+      public void end(@Nullable Throwable throwable) {
         scope.close();
         instrumenter().end(context, handlerData, null, throwable);
       }
@@ -74,15 +81,15 @@ public static AdviceScope decorateAbortSpan(
       Method method = resourceMethodInvoker.getMethod();
       Class<?> resourceClass = resourceMethodInvoker.getResourceClass();
 
-      return new AdviceScope(resourceClass, method, requestContext);
+      return AdviceScope.start(resourceClass, method, requestContext);
     }
 
     @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class)
     public static void stopSpan(
         @Advice.Thrown @Nullable Throwable throwable,
         @Advice.Enter @Nullable AdviceScope adviceScope) {
       if (adviceScope != null) {
-        adviceScope.exit(throwable);
+        adviceScope.end(throwable);
       }
     }
   }

instrumentation/jaxrs/jaxrs-2.0/jaxrs-2.0-resteasy-3.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jaxrs/v2_0/Resteasy31RequestContextInstrumentation.java

@@ -41,20 +41,26 @@ public static class AdviceScope {
       private final Context context;
       private final Scope scope;
 
-      public AdviceScope(
+      private AdviceScope(Jaxrs2HandlerData handlerData, Context context) {
+        this.handlerData = handlerData;
+        this.context = context;
+        scope = context.makeCurrent();
+      }
+
+      @Nullable
+      public static AdviceScope start(
           Class<?> resourceClass, Method method, ContainerRequestContext requestContext) {
-        handlerData = new Jaxrs2HandlerData(resourceClass, method);
-        context =
+        Jaxrs2HandlerData handlerData = new Jaxrs2HandlerData(resourceClass, method);
+        Context context =
             Jaxrs2RequestContextHelper.createOrUpdateAbortSpan(
                 instrumenter(), requestContext, handlerData);
-
-        scope = context != null ? context.makeCurrent() : null;
+        if (context == null) {
+          return null;
+        }
+        return new AdviceScope(handlerData, context);
       }
 
-      public void exit(@Nullable Throwable throwable) {
-        if (scope == null) {
-          return;
-        }
+      public void end(@Nullable Throwable throwable) {
         scope.close();
         instrumenter().end(context, handlerData, null, throwable);
       }
@@ -75,15 +81,15 @@ public static AdviceScope decorateAbortSpan(
       Method method = resourceMethodInvoker.getMethod();
       Class<?> resourceClass = resourceMethodInvoker.getResourceClass();
 
-      return new AdviceScope(resourceClass, method, requestContext);
+      return AdviceScope.start(resourceClass, method, requestContext);
     }
 
     @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class)
     public static void stopSpan(
         @Advice.Thrown @Nullable Throwable throwable,
         @Advice.Enter @Nullable AdviceScope adviceScope) {
       if (adviceScope != null) {
-        adviceScope.exit(throwable);
+        adviceScope.end(throwable);
       }
     }
   }

instrumentation/jaxrs/jaxrs-3.0/jaxrs-3.0-annotations/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jaxrs/v3_0/DefaultRequestContextInstrumentation.java

@@ -48,8 +48,7 @@ private AdviceScope(Context context, Scope scope, Jaxrs3HandlerData handlerData)
       }
 
       @Nullable
-      public static AdviceScope enter(
-          Class<?> filterClass, Method method, ContainerRequestContext requestContext) {
+      public static AdviceScope start(Class<?> filterClass, Method method) {
         Context parentContext = Context.current();
         Jaxrs3HandlerData handlerData = new Jaxrs3HandlerData(filterClass, method);
 
@@ -67,7 +66,7 @@ public static AdviceScope enter(
         return new AdviceScope(context, context.makeCurrent(), handlerData);
       }
 
-      public void exit(@Nullable Throwable throwable) {
+      public void end(@Nullable Throwable throwable) {
         scope.close();
         instrumenter().end(context, handlerData, null, throwable);
       }
@@ -97,15 +96,16 @@ public static AdviceScope createGenericSpan(
         return null;
       }
 
-      return AdviceScope.enter(filterClass, method, requestContext);
+      return AdviceScope.start(filterClass, method);
     }
 
     @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class)
     public static void stopSpan(
-        @Advice.Thrown Throwable throwable, @Advice.Enter @Nullable AdviceScope adviceScope) {
+        @Advice.Thrown @Nullable Throwable throwable,
+        @Advice.Enter @Nullable AdviceScope adviceScope) {
 
       if (adviceScope != null) {
-        adviceScope.exit(throwable);
+        adviceScope.end(throwable);
       }
     }
   }