Improved classLoaderMatcher comments

Author: trask

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

@@ -82,21 +82,45 @@ class suffices for a simple version boundary — use multiple classes only when:
   present (e.g., Jersey checks both the JAX-RS spec API and the Jersey implementation class;
   AWS SDK Lambda checks both the Lambda module and JSON protocol core).
 
-Place a comment **above each class name string** stating its version role and matcher shape:
+For multi-class checks or negated exclusions, place a comment **above each class name string**
+stating its version role and matcher shape:
 
 - **Positive floor class** in `hasClassesNamed(...)` (proves "at least version X"):
-  use `// added in X.Y`.
+  use `// added in X.Y`, optionally followed by brief parenthetical context only when it adds
+  interesting, non-duplicative signal.
 - **Positive ceiling class** in `hasClassesNamed(...)` (proves "not yet version Y"):
-  use `// removed in Y.Z`.
+  use `// removed in Y.Z`, optionally followed by brief parenthetical context only when it
+  adds interesting, non-duplicative signal.
 - **Negated exclusion class** in `not(hasClassesNamed(...))` or
-  `.and(not(hasClassesNamed(...)))`: use `// added in Y.Z`, because the class's presence
-  starts excluding `Y.Z+`.
+  `.and(not(hasClassesNamed(...)))`: use `// added in Y.Z`, optionally followed by brief
+  parenthetical context only when it adds interesting, non-duplicative signal, because the
+  class's presence starts excluding `Y.Z+`.
+
+Short parenthetical details are allowed only when they add interesting, non-duplicative
+signal beyond the version role itself, for example native instrumentation notes, backport
+context, or namespace rename notes.
 
 A single **positive** landmark class can sometimes provide **both** bounds: its presence
 proves the module is at least version `X.Y`, and the same class disappears in `Y.Z`, so
 its presence also excludes `Y.Z+`. In that case, a combined comment such as
 `// added in X.Y, removed in Y.Z` is appropriate.
 
+For a single-item `hasClassesNamed(...)` check, prefer the compact form with the version
+comment above the statement or expression that uses it, instead of splitting the sole string
+onto its own line. For example:
+
+```java
+// added in 3.0
+return hasClassesNamed("jakarta.faces.context.FacesContext");
+```
+
+And for a single negated exclusion:
+
+```java
+// added in 8.10 (native OTel support)
+.and(not(hasClassesNamed("co.elastic.clients.transport.instrumentation.Instrumentation")));
+```
+
 **How to identify ceiling classes**: check whether a **newer sibling module** exists for
 the same library (e.g., `mongo-4.0` next to `mongo-3.7`). If the newer module's
 `classLoaderMatcher()` checks a different variant of the same class (e.g.,
@@ -140,7 +164,7 @@ public ElementMatcher.Junction<ClassLoader> classLoaderMatcher() {
   return hasClassesNamed(
       // added in 3.7
       "com.mongodb.MongoClientSettings$Builder",
-      // removed in 4.0 — excludes driver 4.0+ where the 4.0 module takes over
+      // removed in 4.0 — replaced by com.mongodb.internal.async.SingleResultCallback
       "com.mongodb.async.SingleResultCallback");
 }
 ```
@@ -187,14 +211,19 @@ public ElementMatcher.Junction<ClassLoader> classLoaderMatcher() {
   is correct when muzzle can detect version boundaries on its own (the common case). Only
   flag a *missing* override when the module targets a specific version range and the
   versioning signal (added/removed landmark class) is not part of the classes muzzle inspects.
-- **Version comments on landmark classes.** For multi-class checks, `.and(not(...))`
-  chains, or cases where the landmark version differs from the module's base version,
-  each `hasClassesNamed()` call must have a `//` comment stating the class's **role**:
-  - Positive floor class → `// added in X.Y` (class introduced in version X.Y).
-  - Positive ceiling class → `// removed in Y.Z` (class removed in version Y.Z, ensuring
-    the module does not activate on Y.Z+).
-  - Negated exclusion class in `not(hasClassesNamed(...))` → `// added in Y.Z` (class
-    introduced in version Y.Z, so its presence excludes Y.Z+).
+- **Version comments on landmark classes.** For multi-class checks or cases where the
+  landmark version differs from the module's base version, each `hasClassesNamed()` call
+  must have a `//` comment stating the class's **role**. For a single-item
+  `hasClassesNamed(...)` check, including a negated exclusion in `.and(not(...))`, prefer
+  the compact form with the comment above the statement or expression instead of splitting
+  the sole string onto its own line:
+  - Positive floor class → `// added in X.Y` (optionally with brief, non-duplicative
+    parenthetical context such as `renamed from javax` or `backported to 2.12.3`).
+  - Positive ceiling class → `// removed in Y.Z` (optionally with brief, non-duplicative
+    parenthetical context such as `replaced by jakarta variant` or `moved to internal.async`).
+  - Negated exclusion class in `not(hasClassesNamed(...))` → `// added in Y.Z`
+    (optionally with brief, non-duplicative parenthetical context such as `native OTel
+    support` or `shaded into core module`).
   - Single positive class that serves as both floor and ceiling → include both boundaries,
     e.g. `// added in X.Y, removed in Y.Z`.
   Do not use `// added in` for a **positive** ceiling class — that states when the class
@@ -204,13 +233,15 @@ public ElementMatcher.Junction<ClassLoader> classLoaderMatcher() {
   To identify ceiling classes, check if a newer sibling module's `classLoaderMatcher()`
   checks a different variant or replacement class.
 - **Single-class landmark comments are required.** When a single-class
-  `hasClassesNamed(...)` check establishes the module's lower bound, add an inline
-  `// added in X.Y` comment. If that same class also establishes the upper bound because
-  it is removed or replaced in a newer sibling version, include that fact too, e.g.
-  `// added in X.Y, removed in Y.Z`. First validate each stated boundary from repository
-  or upstream evidence; do not infer versions from the module name alone. Flag a missing
-  boundary comment in this case, and do not flag an existing dual-role comment for removal
-  unless it is demonstrably wrong.
+  `hasClassesNamed(...)` check establishes the module's lower bound, or when a single-class
+  negated check establishes an excluded upper range, prefer the compact form with the
+  version comment immediately above the statement or expression that uses the matcher.
+  Parenthetical context is fine only when it adds interesting, non-duplicative signal. If
+  the same positive class also establishes the upper bound because it is removed or replaced
+  in a newer sibling version, include that fact too, e.g. `// added in X.Y, removed in Y.Z`.
+  First validate each stated boundary from repository or upstream evidence; do not infer
+  versions from the module name alone. Flag a missing boundary comment in this case, and do
+  not flag an existing dual-role comment for removal unless it is demonstrably wrong.
 - Prefer **one landmark class** per version boundary — choose the most stable/specific class.
 - Pair with **muzzle config** (`assertInverse.set(true)`) for full coverage.
 - Use `hasClassesNamed(...)` (from `AgentElementMatchers`) — not raw ByteBuddy matchers.

instrumentation/azure-core/azure-core-1.14/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/azurecore/v1_14/AzureSdkInstrumentationModule.java

@@ -53,20 +53,12 @@ public void injectClasses(ClassInjector injector) {
 
   @Override
   public ElementMatcher.Junction<ClassLoader> classLoaderMatcher() {
-    return hasClassesNamed(
-            // added in 1.14.0
-            "com.azure.core.util.tracing.Tracer")
-        // this is needed to prevent this instrumentation from being applied to azure-core 1.19+
-        .and(
-            not(
-                hasClassesNamed(
-                    // added in 1.19.0
-                    "com.azure.core.util.tracing.StartSpanOptions")))
-        .and(
-            not(
-                hasClassesNamed(
-                    // added in 1.19.0
-                    "com.azure.core.tracing.opentelemetry.OpenTelemetryTracer")));
+    // added in azure-core 1.14.0
+    return hasClassesNamed("com.azure.core.util.tracing.Tracer")
+        // added in azure-core 1.19.0
+        .and(not(hasClassesNamed("com.azure.core.util.tracing.StartSpanOptions")))
+        // added in azure-core-tracing-opentelemetry 1.0.0-beta.47 (native OTel support)
+        .and(not(hasClassesNamed("com.azure.core.tracing.opentelemetry.OpenTelemetryTracer")));
   }
 
   @Override

instrumentation/azure-core/azure-core-1.19/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/azurecore/v1_19/AzureSdkInstrumentationModule.java

@@ -53,11 +53,11 @@ public void injectClasses(ClassInjector injector) {
 
   @Override
   public ElementMatcher.Junction<ClassLoader> classLoaderMatcher() {
-    // this class was introduced in azure-core 1.19
+    // added in azure-core 1.19
     return hasClassesNamed("com.azure.core.util.tracing.StartSpanOptions")
-        // OpenTelemetryTracer was introduced in azure-core-tracing-opentelemetry 1.0.0-beta.47
+        // added in azure-core-tracing-opentelemetry 1.0.0-beta.47 (native OTel support)
         .and(not(hasClassesNamed("com.azure.core.tracing.opentelemetry.OpenTelemetryTracer")))
-        // TracerProvider was introduced in azure-core 1.36
+        // added in azure-core 1.36
         .and(not(hasClassesNamed("com.azure.core.util.tracing.TracerProvider")));
   }
 

instrumentation/azure-core/azure-core-1.36/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/azurecore/v1_36/AzureSdkInstrumentationModule.java

@@ -55,11 +55,11 @@ public void injectClasses(ClassInjector injector) {
 
   @Override
   public ElementMatcher.Junction<ClassLoader> classLoaderMatcher() {
-    // TracerProvider was introduced in azure-core 1.36
+    // added in azure-core 1.36
     return hasClassesNamed("com.azure.core.util.tracing.TracerProvider")
-        // LibraryTelemetryOptions was introduced in azure-core 1.53
+        // added in azure-core 1.53
         .and(not(hasClassesNamed("com.azure.core.util.LibraryTelemetryOptions")))
-        // OpenTelemetryTracer was introduced in azure-core-tracing-opentelemetry 1.0.0-beta.47
+        // added in azure-core-tracing-opentelemetry 1.0.0-beta.47 (native OTel support)
         .and(not(hasClassesNamed("com.azure.core.tracing.opentelemetry.OpenTelemetryTracer")));
   }
 

instrumentation/azure-core/azure-core-1.53/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/azurecore/v1_53/AzureSdkInstrumentationModule.java

@@ -55,9 +55,9 @@ public void injectClasses(ClassInjector injector) {
 
   @Override
   public ElementMatcher.Junction<ClassLoader> classLoaderMatcher() {
-    // LibraryTelemetryOptions was introduced in azure-core 1.53
+    // added in azure-core 1.53
     return hasClassesNamed("com.azure.core.util.LibraryTelemetryOptions")
-        // OpenTelemetryTracer was introduced in azure-core-tracing-opentelemetry 1.0.0-beta.47
+        // added in azure-core-tracing-opentelemetry 1.0.0-beta.47 (native OTel support)
         .and(not(hasClassesNamed("com.azure.core.tracing.opentelemetry.OpenTelemetryTracer")));
   }
 

instrumentation/elasticsearch/elasticsearch-api-client-7.16/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/elasticsearch/apiclient/ElasticsearchApiClientInstrumentationModule.java

@@ -25,12 +25,8 @@ public ElasticsearchApiClientInstrumentationModule() {
 
   @Override
   public ElementMatcher.Junction<ClassLoader> classLoaderMatcher() {
-    // Since Elasticsearch client version 8.10, the ES client comes with a native OTel
-    // instrumentation
-    // that introduced the class `co.elastic.clients.transport.instrumentation.Instrumentation`.
-    // Disabling agent instrumentation for those cases.
     return hasClassesNamed(
-            // present since 7.16 (base version of this instrumentation)
+            // added in 7.16
             "co.elastic.clients.elasticsearch.ElasticsearchClient")
         // added in 8.10 (native OTel instrumentation)
         .and(not(hasClassesNamed("co.elastic.clients.transport.instrumentation.Instrumentation")));

instrumentation/elasticsearch/elasticsearch-rest-7.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/elasticsearch/rest/v7_0/ElasticsearchRest7InstrumentationModule.java

@@ -25,12 +25,9 @@ public ElasticsearchRest7InstrumentationModule() {
 
   @Override
   public ElementMatcher.Junction<ClassLoader> classLoaderMatcher() {
-    // Class `org.elasticsearch.client.RestClient$InternalRequest` introduced in 7.0.0.
-    // Since Elasticsearch client version 8.10, the ES client comes with a native OTel
-    // instrumentation that introduced the class
-    // `co.elastic.clients.transport.instrumentation.Instrumentation`.
-    // Disabling agent instrumentation for those cases.
+    // added in 7.0.0
     return hasClassesNamed("org.elasticsearch.client.RestClient$InternalRequest")
+        // added in 8.10 (native OTel instrumentation)
         .and(not(hasClassesNamed("co.elastic.clients.transport.instrumentation.Instrumentation")));
   }
 

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

@@ -26,9 +26,8 @@ public JaxrsAnnotationsInstrumentationModule() {
   // require jax-rs 2
   @Override
   public ElementMatcher.Junction<ClassLoader> classLoaderMatcher() {
-    return hasClassesNamed(
-        // added in 2.0
-        "javax.ws.rs.core.Configurable");
+    // added in 2.0
+    return hasClassesNamed("javax.ws.rs.core.Configurable");
   }
 
   @Override

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

@@ -25,7 +25,8 @@ public ElementMatcher.Junction<ClassLoader> classLoaderMatcher() {
     return hasClassesNamed(
         // added in JAX-RS 2.0
         "javax.ws.rs.Path",
-        // moved to jaxrs subpackage in 3.1.0, moved back in 3.5.0, moved again in 4.0.0
+        // removed in 3.1.0.Final (moved to core.interception.jaxrs; back in 3.5.0; moved again in
+        // 4.0.0)
         "org.jboss.resteasy.core.interception.PostMatchContainerRequestContext");
   }
 

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

@@ -23,9 +23,9 @@ public Resteasy31InstrumentationModule() {
   @Override
   public ElementMatcher.Junction<ClassLoader> classLoaderMatcher() {
     return hasClassesNamed(
-        // present in JAX-RS 2.0+
+        // added in JAX-RS 2.0
         "javax.ws.rs.Path",
-        // added in RESTEasy 3.1.0.Final
+        // added in RESTEasy 3.1.0.Final (moved from core.interception)
         "org.jboss.resteasy.core.interception.jaxrs.PostMatchContainerRequestContext");
   }