Merge branch 'main' into prefactor-db-info

Author: trask

.github/agents/code-review-and-fix.agent.md

@@ -94,10 +94,13 @@ For each file in scope:
 5. Do not flag a non-capturing lambda or method reference as an unnecessary allocation,
    because on modern JDKs these are typically cached at the call site rather than
    allocated on every invocation.
-6. Flag a missing `// added in X.Y` comment on a single-class lower-bound
-   `hasClassesNamed(...)` check in `classLoaderMatcher()` only after validating that the
-   stated version is factually correct from repository or upstream evidence; the comment
-   documents why the matcher exists and which version boundary it enforces.
+6. Flag a missing version-boundary comment on a single-class `hasClassesNamed(...)`
+   check in `classLoaderMatcher()` only after validating the stated boundary from
+   repository or upstream evidence. Use `// added in X.Y` for a pure lower bound.
+   If the same positive class also provides the upper bound because it disappears in a
+   newer sibling version, preserve or request `removed in Y.Z` too. For negated checks
+   such as `not(hasClassesNamed(...))`, use `// added in Y.Z` because the class's first
+   appearance is what starts the excluded version range.
 7. Prevent duplicates:
    - If equivalent `REVIEW:` already exists above the same line, do not add another.
 

.github/agents/code-review.agent.md

@@ -15,7 +15,9 @@ When a "Knowledge File" is listed, load it from `knowledge/` before reviewing th
 | --- | --- | --- | --- |
 | General | Logic, correctness, reliability, safety, copy/paste mistakes, incorrect comments | Always | — |
 | Style | Style guide | Always | — |
+| Style | Uppercase field names should reflect semantic constants or immutable value constants such as `Duration` timeouts/intervals, not simply `static final` | Always | — |
 | Naming | Getter naming (`get` / `is`) | Always | — |
+| Naming | Boolean/collection option naming (`set*Enabled`, `setCapture*`, `setEmitExperimental*`) | `Experimental` or `TelemetryBuilder` setter changes | `library-patterns.md` |
 | Naming | Module/package naming | New or renamed modules/packages | `module-naming.md` |
 | Javaagent | Advice patterns | `@Advice` classes | `javaagent-advice-patterns.md` |
 | Javaagent | Module structure patterns | `InstrumentationModule`, `TypeInstrumentation`, `Singletons` | `javaagent-module-patterns.md` |
@@ -30,6 +32,7 @@ When a "Knowledge File" is listed, load it from `knowledge/` before reviewing th
 | Build | Gradle conventions, muzzle, test tasks, plugins | `build.gradle.kts`, `settings.gradle.kts` | `gradle-conventions.md` |
 | Build | `testcontainersBuildService` declaration | Testcontainers dependency without `usesService` | `gradle-conventions.md` |
 | Style | Prefer instance creation over singletons for stateless interface impls (except on hot paths or Kotlin `object` declarations) | `TextMapGetter`, `TextMapSetter`, `*AttributesGetter`, `AttributesExtractor`, `SpanNameExtractor`, `HttpServerResponseMutator`, enum/static singletons | — |
+| Style | Prefer `value == null` / `value != null` over `null == value` / `null != value` | Null comparisons | — |
 | Style | No unnecessary explicit type witnesses on generic method calls (`Collections.<String>emptyList()`) | Java generic method calls with explicit type parameters | — |
 | Style | Remove redundant null guards on attribute puts | `AttributesBuilder.put`, `onStart`, `onEnd`, attribute extraction methods | — |
 | General | No redundant `ByteBuffer.duplicate()` on `Value.getValue()` | `Value.getValue()` with `BYTES` type, `ByteBuffer` handling | — |

.github/agents/knowledge/general-rules.md

@@ -12,7 +12,7 @@ be **static nested classes** inside the instrumentation class, not standalone to
 
 ```java
 // ✅ Correct: nested inside instrumentation class
-public class MyInstrumentation implements TypeInstrumentation {
+class MyInstrumentation implements TypeInstrumentation {
   // ...
 
   @SuppressWarnings("unused")

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

@@ -82,10 +82,20 @@ 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:
+Place a comment **above each class name string** stating its version role and matcher shape:
 
-- **Floor class** (proves "at least version X"): use `// added in X.Y`.
-- **Ceiling class** (proves "not yet version Y"): use `// removed in Y.Z`.
+- **Positive floor class** in `hasClassesNamed(...)` (proves "at least version X"):
+  use `// added in X.Y`.
+- **Positive ceiling class** in `hasClassesNamed(...)` (proves "not yet version Y"):
+  use `// removed in Y.Z`.
+- **Negated exclusion class** in `not(hasClassesNamed(...))` or
+  `.and(not(hasClassesNamed(...)))`: use `// added in Y.Z`, because the class's presence
+  starts excluding `Y.Z+`.
+
+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.
 
 **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
@@ -147,9 +157,9 @@ OpenTelemetry instrumentation:
 ```java
 @Override
 public ElementMatcher.Junction<ClassLoader> classLoaderMatcher() {
-  // InternalRequest was introduced in 7.0.0
+  // added in 7.0.0
   return hasClassesNamed("org.elasticsearch.client.RestClient$InternalRequest")
-      // Instrumentation class was introduced in 8.10 (native OTel support)
+      // added in 8.10 (native OTel support)
       .and(not(hasClassesNamed(
           "co.elastic.clients.transport.instrumentation.Instrumentation")));
 }
@@ -160,9 +170,9 @@ public ElementMatcher.Junction<ClassLoader> classLoaderMatcher() {
 ```java
 @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
       .and(not(hasClassesNamed("com.azure.core.tracing.opentelemetry.OpenTelemetryTracer")));
 }
 ```
@@ -180,20 +190,27 @@ public ElementMatcher.Junction<ClassLoader> classLoaderMatcher() {
 - **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**:
-  - Floor class → `// added in X.Y` (class introduced in version X.Y).
-  - Ceiling class → `// removed in Y.Z` (class removed in version Y.Z, ensuring the
-    module does not activate on Y.Z+).
-  Do not use `// added in` for a ceiling class — that states when the class first appeared,
-  which is irrelevant and misleading; the purpose is the upper bound.
+  - 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+).
+  - 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 lower-bound comments are required.** When a single-class
-  `hasClassesNamed(...)` check exists solely to establish the module's lower bound, add an
-  inline `// added in X.Y` comment. The comment explains why the matcher exists and which
-  version boundary it enforces. First validate that `X.Y` is factually correct from
-  repository or upstream evidence; do not infer it from the module name alone. Flag a
-  missing comment in this case, and do not flag an existing one for removal unless the
-  comment is demonstrably wrong.
+- **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.
 - 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.
@@ -208,7 +225,7 @@ Each `TypeInstrumentation` class instruments a single target class.
 ### Required structure
 
 ```java
-public class MyClassInstrumentation implements TypeInstrumentation {
+class MyClassInstrumentation implements TypeInstrumentation {
 
   @Override
   public ElementMatcher<TypeDescription> typeMatcher() {

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

@@ -44,3 +44,32 @@ public final class MyLibraryTelemetry {
 - Builder setter methods return `this` and are annotated `@CanIgnoreReturnValue`.
 - The builder's constructor is package-private — only `Telemetry.builder()` creates it.
 - `build()` returns the `{Library}Telemetry` instance.
+
+## Boolean and Collection Option Naming
+
+Builder methods on `TelemetryBuilder` and `Experimental` classes must follow these
+naming patterns. Flag any new method that deviates.
+
+### Canonical patterns
+
+| Semantic | Pattern | Examples |
+| --- | --- | --- |
+| Feature on/off toggle | `set*Enabled(boolean)` | `setSqlCommenterEnabled`, `setQuerySanitizationEnabled`, `setDataSourceInstrumenterEnabled` |
+| Emit experimental signals | `setEmitExperimental*(boolean)` | `setEmitExperimentalTelemetry`, `setEmitExperimentalMetrics` |
+| Capture data (boolean) | `setCapture*(boolean)` | `setCaptureExperimentalSpanAttributes`, `setCaptureEnduserId`, `setCaptureQuery` |
+| Capture data (collection) | `setCapture*(Collection)` | `setCaptureRequestHeaders`, `setCaptureResponseHeaders`, `setCaptureRequestParameters` |
+
+### Rules
+
+- **`set*Enabled`** is for features that are turned on or off (sanitization, SQL commenter,
+  instrumenter toggles). The thing being named is a feature or processing step.
+- **`setCapture*`** is for options that control whether a piece of data is collected or
+  emitted (attributes, headers, query text, message content). Use the verb form for both
+  boolean and collection variants.
+- **`setEmitExperimental*`** is reserved for the `Experimental` class toggle that enables
+  all experimental telemetry for an instrumentation. Do not use on `TelemetryBuilder`.
+
+### Known exceptions
+
+- `setPreferJfrMetrics(boolean)` — selects JFR over JMX for overlapping metrics; it is a
+  strategy selector, not a feature toggle or data capture flag.

.github/agents/knowledge/library-patterns.md

@@ -39,6 +39,11 @@ Fluent assertion calls like `taskId.contains(jobName)` or `taskId.startsWith(pre
 already proper AssertJ assertions — they throw on failure. Do **not** wrap them in
 `assertThat(value.contains(x)).isTrue()`, which degrades the failure message.
 
+- For generic outer `satisfies(AttributeKey, lambda)` parameters, use `val`.
+  Do not use short generic alternatives like `k` or `v` for the outer parameter.
+- If a `satisfies(...)` lambda contains a nested inner lambda and a second parameter name is
+  required, keep the outer parameter as `val` and use `v` for the nested parameter.
+
 ## AssertJ Idiomatic Simplifications
 
 Prefer built-in AssertJ collection/list assertions over extracting values manually:

.github/agents/knowledge/testing-general-patterns.md

@@ -16,6 +16,14 @@ Keep scoped language or file-type rules in `.github/instructions/*.instructions.
 - Style guide and core conventions: `docs/contributing/style-guide.md`
 - Review and implementation knowledge by topic: `.github/agents/knowledge/README.md`
 
+## Knowledge Loading
+
+For coding, fix, and refactoring tasks, consult `.github/agents/knowledge/README.md`
+before making substantial changes.
+
+Use the knowledge index to load only the article(s) relevant to the current task.
+Do not load the entire knowledge folder by default.
+
 ## Gradle Execution Rules
 
 Never use `--rerun-tasks`. Use `--rerun` when needed.