copyedit

Author: trask

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

@@ -41,24 +41,31 @@ public class MyLibrary10InstrumentationModule extends InstrumentationModule {
 
 ### `classLoaderMatcher()` — Version-Boundary Detection
 
-Override `classLoaderMatcher()` when the instrumentation targets a specific library version
-range and muzzle alone cannot distinguish it (e.g., a class was added or removed between
-versions but isn't referenced by helper classes).
+Override `classLoaderMatcher()` only when the instrumentation targets a specific library
+version range and muzzle cannot distinguish that range on its own. Typical cases are classes
+that were added, removed, renamed, or introduced by a library's own native OpenTelemetry
+instrumentation, but are not referenced by the helper classes muzzle inspects.
 
-**Execution order**: `classLoaderMatcher()` runs **first** (fast), then `typeMatcher()`, then
-muzzle (expensive, cached). Use `classLoaderMatcher()` to pre-filter before muzzle runs.
+**Execution order**: `classLoaderMatcher()` runs **first** and should be very cheap. It is
+followed by `typeMatcher()`, then muzzle. Use `classLoaderMatcher()` to reject obvious
+non-matches before the more expensive checks run.
 
-**When to override** (otherwise let muzzle handle it):
+**Override it when**:
 
-- A landmark class was **added** in the target version (exclude older versions).
-- A landmark class was **removed** in a newer version (exclude newer versions).
-- A newer version has **native OpenTelemetry instrumentation** — avoid double-instrumentation.
+- A landmark class was **added** in the target version and excludes older versions.
+- A landmark class was **removed** in a newer version and excludes newer versions.
+- A newer version ships **native OpenTelemetry instrumentation** and the agent should back off.
 
-**When NOT to override** (muzzle catches these automatically):
+**Do not override it for**:
 
-- Method signature changes, parameter type changes, field removals, package relocations.
+- Method signature changes.
+- Parameter type changes.
+- Field removals.
+- Package relocations that muzzle already sees.
 
-#### Pattern A: Check for a class added in the target version (most common)
+#### Pattern A: Single Landmark Class
+
+This is the most common case: one class cleanly identifies the lower bound.
 
 ```java
 @Override
@@ -68,66 +75,58 @@ public ElementMatcher.Junction<ClassLoader> classLoaderMatcher() {
 }
 ```
 
-#### Pattern B: Multiple landmark classes (AND logic)
-
-`hasClassesNamed()` with multiple arguments requires **ALL** classes to be present. A single
-class suffices for a simple version boundary — use multiple classes only when:
-
-- **One class is ambiguous** — e.g., it was backported to an older branch, so it doesn't
-  uniquely identify the target version. Adding a second class narrows the match.
-- **Floor + ceiling pinning** — one class proves "at least version X", another proves
-  "not yet version Y" (e.g., Hibernate 3.3 checks a class absent before 3.3 AND a class
-  absent in 4.0+, pinning to exactly 3.3.x–3.x).
-- **Cross-JAR dependency** — the classes come from different JARs that may be independently
-  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).
-
-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`, 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`, optionally followed by brief parenthetical context only when it
-  adds interesting, non-duplicative signal.
+#### Pattern B: Multiple Landmark Classes
+
+`hasClassesNamed()` with multiple arguments requires **all** listed classes to be present.
+Use multiple classes only when one class is not enough.
+
+- **Ambiguous floor**: one class was backported and no longer uniquely identifies the target
+  version.
+- **Floor + ceiling pinning**: one class proves "at least X", another proves "not yet Y".
+- **Cross-JAR dependency**: the version boundary depends on classes from separate artifacts,
+  such as a spec API plus a library implementation.
+
+For multi-class checks, and for negated exclusions, add a version comment **above each class
+name string**.
+
+- **Positive floor class** in `hasClassesNamed(...)`: use `// added in X.Y`.
+- **Positive ceiling class** in `hasClassesNamed(...)`: use `// removed in Y.Z`.
 - **Negated exclusion class** in `not(hasClassesNamed(...))` or
-  `.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+`.
+  `.and(not(hasClassesNamed(...)))`: use `// added in Y.Z`, because that class's first
+  appearance is what starts excluding `Y.Z+`.
 
-Short parenthetical details are allowed only when they add interesting, non-duplicative
+You may add brief parenthetical context, but only when it adds 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.
+One 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 it also excludes
+`Y.Z+`. In that case, use a combined comment such as `// added in X.Y, removed in Y.Z`.
 
 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:
+comment above the statement or expression that uses the matcher, instead of splitting the
+single string onto its own line:
 
 ```java
 // added in 3.0
 return hasClassesNamed("jakarta.faces.context.FacesContext");
 ```
 
-And for a single negated exclusion:
+The same compact style applies to single negated exclusions:
 
 ```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.,
-`com.mongodb.internal.async.SingleResultCallback` vs `com.mongodb.async.SingleResultCallback`),
-the old variant was likely removed in the newer version and serves as a ceiling. A ceiling
-class may have been *introduced* much earlier than the module's target version — the relevant
-fact is when it was **removed**, not when it was added.
+**How to identify ceiling classes**: look for a **newer sibling module** for the same
+library, such as `mongo-4.0` next to `mongo-3.7`. If the newer module's
+`classLoaderMatcher()` checks a different variant of the same class, such as
+`com.mongodb.internal.async.SingleResultCallback` instead of
+`com.mongodb.async.SingleResultCallback`, the older variant was likely removed in the newer
+version and can serve as the ceiling. A ceiling class may have been introduced much earlier
+than the module's target version; the important fact is when it was **removed**, not when it
+first appeared.
 
 ```java
 @Override
@@ -140,10 +139,10 @@ public ElementMatcher.Junction<ClassLoader> classLoaderMatcher() {
 }
 ```
 
-Here `ConfigurationStrSubstitutor` alone would also match 2.12.3 (due to back-port),
-so `DefaultArbiter` (added in 2.15) is required to tighten the match to 2.17+.
+Here `ConfigurationStrSubstitutor` alone would also match 2.12.3 because of the backport, so
+`DefaultArbiter` is needed to tighten the match to 2.17+.
 
-Another common shape — floor + ceiling:
+Another common shape is floor + ceiling pinning:
 
 ```java
 @Override
@@ -156,27 +155,27 @@ public ElementMatcher.Junction<ClassLoader> classLoaderMatcher() {
 }
 ```
 
-Another floor + ceiling example — mongo 3.7 pins to 3.7.x–3.x:
+Another example pins mongo 3.7 to 3.7.x–3.x:
 
 ```java
 @Override
 public ElementMatcher.Junction<ClassLoader> classLoaderMatcher() {
   return hasClassesNamed(
       // added in 3.7
       "com.mongodb.MongoClientSettings$Builder",
-      // removed in 4.0 — replaced by com.mongodb.internal.async.SingleResultCallback
+      // removed in 4.0 (replaced by com.mongodb.internal.async.SingleResultCallback)
       "com.mongodb.async.SingleResultCallback");
 }
 ```
 
-Here `SingleResultCallback` was introduced in driver 3.0 but **removed in 4.0**. Its
-presence ensures the 3.7 module does not activate on 4.0+ classloaders. The comment must
-say `// removed in 4.0`, not `// added in 3.0` — the purpose is the upper bound.
+`SingleResultCallback` was introduced in driver 3.0 but **removed in 4.0**. Its presence keeps
+the 3.7 module from activating on 4.0+ classloaders. That is why the comment must say
+`// removed in 4.0`, not `// added in 3.0`.
 
-#### Pattern C: Exclude newer versions with native instrumentation
+#### Pattern C: Exclude Newer Versions with Native Instrumentation
 
-Use `.and(not(hasClassesNamed(...)))` to opt out when the library ships its own
-OpenTelemetry instrumentation:
+Use `.and(not(hasClassesNamed(...)))` when the newer library version ships its own
+OpenTelemetry instrumentation and the agent should opt out.
 
 ```java
 @Override
@@ -189,7 +188,7 @@ public ElementMatcher.Junction<ClassLoader> classLoaderMatcher() {
 }
 ```
 
-#### Pattern D: Exclude a version range (upper bound)
+#### Pattern D: Exclude an Upper Range
 
 ```java
 @Override
@@ -205,49 +204,43 @@ public ElementMatcher.Junction<ClassLoader> classLoaderMatcher() {
 
 - **Do NOT add `classLoaderMatcher()` for optimization.** The "skip when library is absent"
   optimization belongs on `TypeInstrumentation.classLoaderOptimization()`, not here.
-  `classLoaderMatcher()` is only for **version-boundary detection** — cases where muzzle
-  alone cannot distinguish the target version range. Most modules do not need it.
-- **Do NOT flag modules that lack a `classLoaderMatcher()` override.** The default (`any()`)
-  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 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
-  first appeared, which is irrelevant and misleading; the purpose is the upper bound.
-  Conversely, do use `// added in` for a **negated** exclusion class, because the class's
-  first appearance is exactly what begins the excluded version range.
-  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, 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.
+  `classLoaderMatcher()` is only for **version-boundary detection**. Most modules do not need
+  it.
+- **Do NOT flag modules that omit `classLoaderMatcher()`.** The default (`any()`) is correct
+  when muzzle can detect the version boundary on its own. Only flag a missing override when
+  the module truly depends on an added or removed landmark class that muzzle does not inspect.
+- **Version comments are required on landmark classes.** For multi-class checks, or whenever
+  the landmark version differs from the module's base version, every `hasClassesNamed()` call
+  needs a role comment. 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 placing the only string on its own line.
+  - Positive floor class → `// added in X.Y`, optionally with brief, non-duplicative context
+    such as `renamed from javax` or `backported to 2.12.3`.
+  - Positive ceiling class → `// removed in Y.Z`, optionally with brief, non-duplicative
+    context such as `replaced by jakarta variant` or `moved to internal.async`.
+  - Negated exclusion class → `// added in Y.Z`, optionally with brief, non-duplicative
+    context such as `native OTel support` or `shaded into core module`.
+  - Single positive class serving as both floor and ceiling → include both boundaries, for
+    example `// added in X.Y, removed in Y.Z`.
+  Do not use `// added in` for a **positive** ceiling class, because the upper bound depends on
+  when that class disappeared, not when it first appeared. Conversely, do use `// added in`
+  for a **negated** exclusion class, because its first appearance is what starts the excluded
+  range.
+- **Single-class landmark comments are required.** When a single-class `hasClassesNamed(...)`
+  check establishes the module's lower bound, or when a single-class negated check establishes
+  an excluded upper range, use 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 too,
+  for example `// added in X.Y, removed in Y.Z`. First validate the stated boundaries from
+  repository or upstream evidence; do not infer them from the module name alone.
+- Prefer **one landmark class** per version boundary whenever possible.
 - Pair with **muzzle config** (`assertInverse.set(true)`) for full coverage.
-- Use `hasClassesNamed(...)` (from `AgentElementMatchers`) — not raw ByteBuddy matchers.
+- Use `hasClassesNamed(...)` from `AgentElementMatchers`, not raw ByteBuddy matchers.
 - `classLoaderMatcher()` runs against **every class loader** in the JVM before type matching
-  begins. It must be fast: use only `hasClassesNamed(...)` (a simple class-presence check).
-  Never use hierarchy matchers (`extendsClass(...)`, `implementsInterface(...)`) here.
+  begins, so it must stay fast. Use only `hasClassesNamed(...)`, which is a simple
+  class-presence check. Never use hierarchy matchers such as `extendsClass(...)` or
+  `implementsInterface(...)` here.
 
 ## TypeInstrumentation