Merge branch 'main' into prefactor-db-info

Author: trask

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

@@ -134,6 +134,9 @@ Auto-fix boundaries:
 
 - Safe to fix:
   - import cleanup or direct style-guide conformance
+  - normalization of existing `@SuppressWarnings` syntax or placement, but preserve any
+    accurate explanatory comment attached to the suppression instead of deleting it as
+    style noise
   - obvious assertion API migrations (e.g., AssertJ preference) and idiomatic
     simplifications listed in `testing-general-patterns.md` § AssertJ Idiomatic
     Simplifications (e.g., `assertThat(list.size()).isEqualTo(N)` →
@@ -142,6 +145,8 @@ Auto-fix boundaries:
     lambdas where the lambda parameter is already an `AbstractAssert` (e.g.,
     `AbstractStringAssert`). Calls like `taskId.contains(jobName)` on the assert object are
     already valid AssertJ assertions; do not wrap them in `assertThat(...).isTrue()`
+  - AssertJ `.as(...)` descriptions and `.withFailMessage(...)` in tests — remove them
+    and prefer direct assertions whose failure output already exposes the unexpected values
   - deterministic semconv constant handling aligned with repository rules
   - missing test-task wiring patterns with clear canonical form
   - missing `testInstrumentation` cross-version references — when a javaagent module belongs
@@ -207,6 +212,16 @@ Auto-fix boundaries:
     to avoid allocating on every invocation. Only convert singletons used at
     registration/initialization time (e.g., `Instrumenter` builder chains, `Singletons`
     setup)
+  - try-with-resources wrapping most of a test body for an `AutoCloseable` that only
+    needs cleanup at test end — convert to `AutoCleanupExtension` with `deferCleanup(...)`.
+    Add a `@RegisterExtension static final AutoCleanupExtension cleanup =
+    AutoCleanupExtension.create();` field if one does not already exist, then replace
+    the try-with-resources with `cleanup.deferCleanup(resource);` and un-indent the body.
+    Keep try-with-resources for semantically scoped resources whose lifetime must end
+    mid-test (e.g., `Scope` / `Context.makeCurrent()`, `MockedStatic`, short-lived
+    readers/writers/streams/response bodies).
+    Do not apply this conversion in non-JUnit helper methods, `@BeforeAll`, or shared
+    setup code.
   - `hasAttributesSatisfying(...)` calls in test assertions — replace with
     `hasAttributesSatisfyingExactly(...)` because it is more precise (the non-exact
     variant silently ignores unexpected attributes)

.github/agents/knowledge/README.md

@@ -13,10 +13,11 @@ Load only files relevant to the current scope to reduce noise and avoid over-con
 | `general-rules.md` | Always — review checklist table and core rules enforced on every review |
 | `gradle-conventions.md` | `build.gradle.kts` or `settings.gradle.kts` changes, custom test task registration or wiring |
 | `javaagent-advice-patterns.md` | ByteBuddy `@Advice` class or advice-method changes |
-| `javaagent-module-patterns.md` | `InstrumentationModule`, `TypeInstrumentation`, `Singletons`, `VirtualField`, `CallDepth` |
+| `javaagent-module-patterns.md` | `InstrumentationModule`, `TypeInstrumentation`, `VirtualField`, `CallDepth` |
+| `javaagent-singletons-patterns.md` | `*Singletons` holder classes, singleton accessors, callers of singleton accessors/fields |
 | `library-patterns.md` | Library instrumentation telemetry, builder, getter, or setter pattern changes |
 | `module-naming.md` | New or renamed modules or packages; settings includes |
-| `testing-general-patterns.md` | Test files in scope — assertion style, attribute assertion patterns, `satisfies()` lambda usage |
+| `testing-general-patterns.md` | Test files in scope — assertion style, resource cleanup patterns, attribute assertion patterns, `satisfies()` lambda usage |
 | `testing-experimental-flags.md` | `testExperimental` task or experimental span-attribute assertions |
 | `testing-semconv-stability.md` | Semconv opt-in modes, `emitOld*`/`emitStable*`, `maybeStable`, Semconv test tasks |
 

.github/agents/knowledge/config-property-stability.md

@@ -52,7 +52,7 @@ Defined in [VERSIONING.md](../../../VERSIONING.md):
 Examples (flat ↔ YAML):
 
 - `otel.instrumentation.http.client.capture-request-headers` ↔ `request_captured_headers` — **stable**
-- `otel.instrumentation.common.experimental.db-sqlcommenter.enabled` ↔ `sqlcommenter/development: { enabled: true }` — **experimental**
+- `otel.instrumentation.common.db.experimental.sqlcommenter.enabled` ↔ `sqlcommenter/development: { enabled: true }` — **experimental**
 - `otel.instrumentation.http.client.emit-experimental-telemetry` ↔ `emit_experimental_telemetry/development: true` — **experimental**
 
 ## Deprecation Communication

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

@@ -17,14 +17,15 @@ When a "Knowledge File" is listed, load it from `knowledge/` before reviewing th
 | 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` |
+| Style | Prefer `e` for used exceptions, prefer `f` for used exceptions in nested catch clauses when an outer catch already uses `e`, allow `error` for specific `*Error` catch types, prefer `t` for used `Throwable` catch values, prefer `ignored` for intentionally unused catch parameters, and use `ignore` for nested intentionally unused catch parameters when `ignored` would shadow an outer catch variable | Catch clauses | — |
 | 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` |
+| Javaagent | Module structure patterns | `InstrumentationModule`, `TypeInstrumentation` | `javaagent-module-patterns.md` |
+| Javaagent | Singletons patterns | `*Singletons` holder classes, singleton accessors, callers of singleton accessors/fields | `javaagent-singletons-patterns.md` |
 | Javaagent | Incorrect `classLoaderMatcher()` | `classLoaderMatcher()` override that is redundant (muzzle already handles it) or missing when needed (muzzle cannot distinguish version range) | `javaagent-module-patterns.md` |
 | Semconv | Library vs javaagent semconv constant usage | Semconv constants/assertions | — |
 | Semconv | Dual semconv testing | `SemconvStability`, `maybeStable`, semconv Gradle tasks | `testing-semconv-stability.md` |
-| Testing | General test patterns | Test files in scope | `testing-general-patterns.md` |
+| Testing | General test patterns | Test files in scope — assertion style, resource cleanup, attribute assertions | `testing-general-patterns.md` |
 | Testing | Experimental flag tests | `testExperimental`, experimental attribute assertions, `experimental` flags in JVM args or system properties | `testing-experimental-flags.md` |
 | Library | TelemetryBuilder/getter/setter patterns | Library instrumentation classes | `library-patterns.md` |
 | API | Deprecation and breaking-change policy | Public API changes | `api-deprecation-policy.md` |
@@ -56,6 +57,21 @@ Flag real defects, including:
 
 Only flag substantive problems, not stylistic preference.
 
+## [Javaagent] Best-Effort Suppressed Failures
+
+When javaagent runtime code intentionally suppresses a `Throwable` to avoid breaking the
+application, do not silently swallow it.
+
+- Prefer `ExceptionLogger.logSuppressedError(...)` for suppressed failures in javaagent code.
+  It logs at `FINE` and matches the agent's default ByteBuddy advice suppression path in
+  `ExceptionHandlers.defaultExceptionHandler()`.
+- Use this for best-effort hooks such as response customizers, bootstrap fallbacks, or other
+  optional extension points where failure must not change application behavior.
+- Do not introduce ad-hoc local loggers for one-off suppressed javaagent failures when
+  `ExceptionLogger` is available from bootstrap.
+- Keep the message action-oriented and specific (for example, "Failed to customize Netty 4.1
+  HTTP server response").
+
 ## [Style] Style Guide
 
 Read and apply `docs/contributing/style-guide.md`.
@@ -73,11 +89,44 @@ Do not flag the following patterns (common false positives):
   The project disables javac's `-Xlint:deprecation` globally and uses a custom Error Prone
   check (`OtelDeprecatedApiUsage`) instead. Only add the annotation when it is actually
   required to fix an Error Prone error — not speculatively.
+- Preserve concise explanatory comments attached to existing `@SuppressWarnings` annotations
+  when they explain why the suppression exists (for example `// using deprecated semconv`
+  or `// using deprecated config property`). This repository has established precedent for
+  such comments, and test code disables Error Prone's `SuppressWarningsWithoutExplanation`
+  check only because enforcing it everywhere causes too many failures, not because these
+  comments are undesirable.
+- When normalizing or moving an existing `@SuppressWarnings`, keep any accurate explanatory
+  comment with it. Remove the comment only if it is incorrect, obsolete, or contradicted by
+  the code after your change.
 
 ## [Naming] Getter Naming
 
 Public API getters should use `get*` (or `is*` for booleans).
 
+## [Style] Catch Exception Variable Naming
+
+Prefer `e` as the exception variable name in catch clauses when the exception is
+used.
+
+For nested catch clauses, when an outer catch already uses `e`, prefer `f` as the
+inner exception variable name when the exception is used.
+
+`error` is also acceptable when the caught type is a specific `*Error` subtype.
+
+Prefer `t` for used `Throwable` catch values, including `catch (Throwable t)`.
+
+Do not apply these catch-variable naming preferences to method parameters,
+lambda parameters, fields, or other non-catch identifiers.
+
+If a catch parameter is intentionally unused, prefer `ignored` over `ignore`.
+
+For nested catch clauses, if `ignored` would shadow an outer catch variable,
+prefer `ignore` for the inner intentionally unused catch parameter.
+
+Both `ignored` and `ignore` are recognized by IntelliJ's `CatchMayIgnoreException`
+inspection as intentional unused-catch names. In test sources, IntelliJ also
+recognizes `expected` and `ok`.
+
 ## [Style] Prefer Instance Creation Over Singletons
 
 Stateless implementations of telemetry interfaces — `TextMapGetter`, `TextMapSetter`,

.github/agents/knowledge/gradle-conventions.md

@@ -51,18 +51,37 @@ Verify `build.gradle.kts` applies the correct plugin for the module type:
 | `library/` | `otel.library-instrumentation` |
 | `testing/` | `otel.java-conventions` |
 
-## Boolean Project Properties
+## Shared Gradle Project Properties
 
-For simple boolean Gradle project properties in `build.gradle.kts`, prefer the repository's most
-common pattern:
+In `build.gradle.kts`, prefer the shared `otelProps` extension for repository-wide Gradle project
+properties that are already modeled there, including:
+
+- `testLatestDeps`
+- `denyUnsafe`
+- `collectMetadata`
+- `testJavaVersion`
+- `testJavaVM`
+- `maxTestRetries`
+- `enableStrictContext`
+
+Examples:
 
 ```kotlin
-val myFlag = findProperty("myFlag") == "true"
-if (myFlag) {
+if (otelProps.testLatestDeps) {
   // ...
 }
+
+tasks.withType<Test>().configureEach {
+  systemProperty("collectMetadata", otelProps.collectMetadata)
+}
 ```
 
+For module-local one-off properties that are not part of `otelProps`, using `findProperty(...)`
+directly is still fine.
+
+`settings.gradle.kts` is the exception: it cannot use project extensions like `otelProps`, so
+direct `gradle.startParameter.projectProperties[...]` access is expected there.
+
 ## `testInstrumentation` Dependencies
 
 The `testInstrumentation` configuration declares which other javaagent instrumentation modules
@@ -175,6 +194,9 @@ When a module has custom test tasks (e.g., `testStableSemconv`), system properti
 args that apply to **all** test tasks should be set once in a `withType<Test>().configureEach`
 block, not repeated on each individual task.
 
+If a property or JVM arg is moved into `withType<Test>().configureEach`, remove any now-redundant
+copies from individual tasks unless a task intentionally overrides the shared value.
+
 When there is only one test task, `tasks.test { ... }` is fine — do not convert it to
 `withType<Test>().configureEach` and do not flag it.
 
@@ -186,7 +208,7 @@ should apply to all tasks. If so, move them to `withType<Test>().configureEach`.
 ```kotlin
 tasks {
   withType<Test>().configureEach {
-    systemProperty("collectMetadata", findProperty("collectMetadata"))
+    systemProperty("collectMetadata", otelProps.collectMetadata)
     // ... other properties common to all test tasks
   }
 
@@ -205,7 +227,7 @@ review**. Only verify correctness when they are already present.
 
 | Property | Type | Value |
 | --- | --- | --- |
-| `collectMetadata` | System property | Pass-through of the `collectMetadata` Gradle project property; defaults to `"false"` |
+| `collectMetadata` | System property | Pass-through of `otelProps.collectMetadata`; defaults to `false` |
 | `metadataConfig` | System property | A single `key=value` string describing the non-default configuration active during this test run |
 
 When already present, verify:

.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.