Code review sweep (run 24961972815)

[otelbot]

Automated code review sweep walked the following modules in order
and stopped after accumulating at least 10 modified files:

• geode-1.4:javaagent
• google-http-client-1.19:javaagent
• grails-3.0:javaagent
• graphql-java-12.0:javaagent
• graphql-java-12.0:library
• graphql-java-20.0:javaagent
• graphql-java-20.0:library
• graphql-java-common-12.0:library
• graphql-java-common-12.0:testing
• grizzly-2.3:javaagent
• grpc-1.6:javaagent
• grpc-1.6:library
• grpc-1.6:testing

---

Module: geode-1.4:javaagent

Module path: instrumentation/geode-1.4/javaagent

Summary

Added the missing declarative_name mapping in instrumentation/geode-1.4/metadata.yaml so the Geode module's documented DB query sanitization config follows the required metadata.yaml format and passes declarative-config validation.

Applied Changes

Config

File: metadata.yaml:8
Change: Added `declarative_name: java.common.db.query_sanitization.enabled` to the `otel.instrumentation.common.db.query-sanitization.enabled` entry.
Reason: `metadata-yaml-format.md` requires each instrumentation `metadata.yaml` config entry to declare its `declarative_name`; comparable DB instrumentations already use `java.common.db.query_sanitization.enabled` for this shared config.

Module: google-http-client-1.19:javaagent

Module path: instrumentation/google-http-client-1.19/javaagent

Summary

Applied a safe nullability-accuracy fix in GoogleHttpRequestInstrumentation for the google-http-client-1.19 javaagent review scope.

Applied Changes

Nullability

File: GoogleHttpRequestInstrumentation.java:76
Change: Annotated the advice helper and `@Advice` exit parameters with `@Nullable` where ByteBuddy can supply `null`, including the `HttpResponse` return value and `Throwable` parameters passed through `AdviceScope`.
Reason: Repository nullability guidance in `knowledge/general-rules.md` requires annotating parameters that can actually be `null`; here `@Advice.Return` is `null` on exceptional exit and `@Advice.Thrown` is `null` on successful exit, so the helper signatures should reflect that runtime flow.

Module: grails-3.0:javaagent

Module path: instrumentation/grails-3.0/javaagent

Summary

Reviewed instrumentation/grails-3.0/javaagent and instrumentation/grails-3.0/metadata.yaml; no safe repository-guideline fixes were needed, and :instrumentation:grails-3.0:javaagent:check passed both normally and with -PtestLatestDeps=true.

Applied Changes

No safe automated changes were applied.

Module: graphql-java-12.0:javaagent

Module path: instrumentation/graphql-java/graphql-java-12.0/javaagent

Summary

Reviewed instrumentation/graphql-java/graphql-java-12.0/javaagent and its module metadata.yaml; no deterministic repository-guideline fixes were needed.

Applied Changes

No safe automated changes were applied.

Module: graphql-java-12.0:library

Module path: instrumentation/graphql-java/graphql-java-12.0/library

Summary

Reviewed instrumentation/graphql-java/graphql-java-12.0/library and its module metadata.yaml; no safe repository-guideline fixes were needed.

Applied Changes

No safe automated changes were applied.

Module: graphql-java-20.0:javaagent

Module path: instrumentation/graphql-java/graphql-java-20.0/javaagent

Summary

Applied one safe review fix in graphql-java-20.0/javaagent: GraphqlInstrumentation now uses the repository-standard helper-injection InitAdvice shape for its none()-matched advice instead of a never-run @Advice.Return bridge.

Applied Changes

Javaagent

File: GraphqlInstrumentation.java:38
Change: Replaced the `none()`-matched `AddInstrumentationAdvice` return-bridging stub with an `InitAdvice` helper-registration stub that only references `GraphqlSingletons` for injection.
Reason: `knowledge/javaagent-advice-patterns.md` says helper-injection-only advice selected with `none()` should use a dummy init-style advice and that `suppress = Throwable.class` and a fake `@Advice.Return` bridge are meaningless when the advice never runs.

Module: graphql-java-20.0:library

Module path: instrumentation/graphql-java/graphql-java-20.0/library

Summary

Applied 1 safe review fix in instrumentation/graphql-java/graphql-java-20.0/library and confirmed the module metadata.yaml configs are wired in code; no unresolved issues remain.

Applied Changes

Style

File: Graphql20OpenTelemetryInstrumentationState.java:31
Change: Reduced `setContextForPath(...)` and `getParentContextForPath(...)` from `public` to package-private.
Reason: The style guide requires minimal necessary visibility. Both helpers are only used from the same package, so package-private matches the intended scope better than `public`.

Module: graphql-java-common-12.0:library

Module path: instrumentation/graphql-java/graphql-java-common-12.0/library

Summary

Applied 1 safe correctness fix in graphql-java-common-12.0 by guarding the shared GraphQL request span status extractor against a nullable ExecutionResult.

Applied Changes

General

File: OpenTelemetryInstrumentationHelper.java:67
Change: Added an `executionResult != null` guard before reading `executionResult.getErrors()` in `setSpanStatusExtractor()`.
Reason: The review checklist's `[General]` correctness rule requires fixing real defects, and this instrumenter callback can receive a nullable `ExecutionResult` on failed executions.

Module: graphql-java-common-12.0:testing

Module path: instrumentation/graphql-java/graphql-java-common-12.0/testing

Summary

Applied 1 safe review fix in instrumentation/graphql-java/graphql-java-common-12.0/testing; AbstractGraphqlTest now fails fast with requireNonNull(...) if schema.graphqls is missing, aligning with the review checklist's correctness and reliability rules for shared test infrastructure.

Applied Changes

General

File: AbstractGraphqlTest.java:19
Change: Wrapped `getResourceAsStream("schema.graphqls")` with `requireNonNull(...)` in `setup()` and added the matching static import.
Reason: `general-rules.md` requires fixing deterministic correctness and reliability issues; the shared test base should fail immediately with a clear missing-resource error instead of relying on an implicit `NullPointerException` from `InputStreamReader` construction.

Module: grizzly-2.3:javaagent

Module path: instrumentation/grizzly-2.3/javaagent

Summary

Reviewed all files under instrumentation/grizzly-2.3/javaagent and validated instrumentation/grizzly-2.3/metadata.yaml against the module’s current patterns; no safe repository-guideline fixes were needed.

Applied Changes

No safe automated changes were applied.

Module: grpc-1.6:javaagent

Module path: instrumentation/grpc-1.6/javaagent

Summary

Applied one safe grpc-1.6 javaagent review fix: build.gradle.kts now relies on the existing shared withType<Test>().configureEach classpath exclusion instead of repeating the same gRPC library filter in each custom test task. metadata.yaml was validated against actual config usage and needed no changes.

Applied Changes

Build

File: build.gradle.kts:52
Change: Removed the duplicated `classpath` filter blocks from `testExperimental`, `testStableSemconv`, and `testBothSemconv`, keeping the shared exclusion only in `withType<Test>().configureEach`.
Reason: Repository Gradle guidance prefers shared configuration for behavior that applies to every explicit `Test` task; repeating the same classpath mutation in each task is unnecessary duplication.

Module: grpc-1.6:library

Module path: instrumentation/grpc-1.6/library

Summary

Applied 1 safe review fix in instrumentation/grpc-1.6/library: GrpcTest.java now inlines non-semconv test attribute keys instead of storing them in class-level constants, matching the repository’s test-assertion guidance.

Applied Changes

Testing

File: GrpcTest.java:39
Change: Inlined `stringKey("customKey")` and `stringKey("customKey2")` at the `hasAttribute(...)` assertions and helper extractors, and removed the `CUSTOM_KEY` / `CUSTOM_KEY2` constants.
Reason: Repository testing guidance says non-semconv attribute keys in test assertions should use inline `AttributeKey` factory methods instead of class-level constants; removing the constants keeps the test aligned with that rule.

Module: grpc-1.6:testing

Module path: instrumentation/grpc-1.6/testing

Summary

Applied safe Testing fixes in instrumentation/grpc-1.6/testing by converting non-exact metric point assertions to hasAttributesSatisfyingExactly(...) and adding a shared helper so old client metric assertions still match both default and -PtestLatestDeps=true shapes. Required testing and library validations completed successfully; :instrumentation:grpc-1.6:javaagent:check -PtestLatestDeps=true still fails in an unrelated pre-existing test.

Applied Changes

Testing

File: AbstractGrpcStreamingTest.java:305
Change: Replaced non-exact client and server metric point assertions with `hasAttributesSatisfyingExactly(...)` and routed old client metric assertions through `addExtraClientMetricAttributes(...)`.
Reason: Repository `testing-general-patterns.md` requires exact attribute assertions because non-exact variants can ignore unexpected attributes; the helper keeps the assertion exact while matching the `testLatestDeps`-specific client metric shape.

File: AbstractGrpcTest.java:1719
Change: Converted remaining non-exact metric point assertions to `hasAttributesSatisfyingExactly(...)` and added `addExtraClientMetricAttributes(...)` for old client metrics that only include `NETWORK_TYPE` under `testLatestDeps`.
Reason: Repository `testing-general-patterns.md` prefers exact attribute assertions, and the shared helper makes that fix deterministic across both dependency modes instead of relying on permissive matching.

Unresolved Items

File: GrpcTest.java
Reason: `:instrumentation:grpc-1.6:javaagent:check -PtestLatestDeps=true` still fails in `clientCallAfterServerCompleted()` because the test now observes `StatusRuntimeException: CANCELLED: io.grpc.Context was cancelled without error` instead of the expected `null`; this failure is outside the modified shared testing sources and remained after the review fixes passed `testing` and `library` validation.

---

Download code review diagnostics

[trask]

this is already under withType<Test>().configureEach