Code review sweep (run 25260462600) (open-telemetry#18518)

Co-authored-by: otelbot <197425009+otelbot@users.noreply.github.com>
Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>

Author: otelbot[bot]

.github/agents/knowledge/README.md

@@ -15,7 +15,7 @@ Load only files relevant to the current scope to reduce noise and avoid over-con
 | `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`, `VirtualField`, `CallDepth` |
-| `javaagent-singletons-patterns.md` | `*Singletons` holder classes, singleton accessors, callers of singleton accessors/fields |
+| `javaagent-singletons-patterns.md` | `*Singletons`, `*SpanNaming`, and similar 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, test method signatures and throws clauses, resource cleanup patterns, attribute assertion patterns, `satisfies()` lambda usage |

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

@@ -21,7 +21,7 @@ When a "Knowledge File" is listed, load it from `knowledge/` before reviewing th
 | 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` | `javaagent-module-patterns.md` |
-| Javaagent | Singletons patterns | `*Singletons` holder classes, singleton accessors, callers of singleton accessors/fields | `javaagent-singletons-patterns.md` |
+| Javaagent | Singletons patterns | `*Singletons`, `*SpanNaming`, and similar 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` |

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

@@ -2,11 +2,13 @@
 
 ## Quick Reference
 
-- Use when: reviewing `*Singletons` holder classes and their callers
+- Use when: reviewing `*Singletons`, `*SpanNaming`, and similar holder classes and their callers
 - Review focus: field/accessor naming, eager initialization, singleton accessor call sites
 
 Javaagent modules keep shared `Instrumenter` instances and related collaborators in a dedicated
-`Singletons` holder class such as `MyLibrarySingletons`.
+`Singletons` holder class such as `MyLibrarySingletons`. Some modules also use focused helper
+holders such as `*ServerSpanNaming` for shared span-name or route-name collaborators; apply the
+same accessor and call-site rules when these classes expose stored singleton fields.
 
 ## Rules
 
@@ -18,8 +20,10 @@ Javaagent modules keep shared `Instrumenter` instances and related collaborators
 - For exported collaborators, keep the field `private`, use a lower camel case field name, and
   give the accessor method the exact same name as the field. Do not prefix these accessors with
   `get`. This rule applies only to zero-arg methods that directly return a stored singleton
-  field; methods that take arguments or compute a value are not singleton accessors and keep
-  their normal names (including `get*` when appropriate).
+  field. It applies regardless of whether the holder class is named `*Singletons`,
+  `*ServerSpanNaming`, or another focused holder name. Methods that take arguments or compute a
+  value are not singleton accessors and keep their normal names (including `get*` when
+  appropriate).
   - `instrumenter` -> `instrumenter()`
   - `helper` -> `helper()`
   - `setter` -> `setter()`
@@ -32,7 +36,9 @@ Javaagent modules keep shared `Instrumenter` instances and related collaborators
   - `RESPONSE_STATUS` stays `RESPONSE_STATUS`
 - Callers should static import only exported singleton accessors and uppercase constant-like
   fields, and use those members unqualified: accessors for lower camel collaborators, fields for
-  uppercase constant-like members.
+  uppercase constant-like members. This includes route/span naming accessors such as
+  `serverSpanName()` when they simply return a stored `HttpServerRouteGetter` or similar
+  collaborator.
 - Keep verb-named helper methods as verbs when they perform work instead of returning a stored
   field. These methods are not singleton accessors and should not be static imported under this
   rule.

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

@@ -97,6 +97,20 @@
   `equalTo(AttributeKey<Long>, int)` overload, so `equalTo(longKey("iteration"), iteration)` is
   preferred over `equalTo(longKey("iteration"), (long) iteration)`.
 
+## Metric Assertions
+
+- Prefer `InstrumentationExtension.waitAndAssertMetrics(...)` for metric assertions. It waits
+  until the supplied assertion passes, so fixed sleeps such as `Thread.sleep(100)`
+  usually unnecessary and make tests slower and more fragile.
+- `DbConnectionPoolMetricsAssertions.assertConnectionPoolEmitsMetrics()` already uses
+  `waitAndAssertMetrics(...)` for each expected pool metric. Keep the pool state being asserted
+  valid until this method returns; for example, if the assertion expects a `used` connection point,
+  keep the borrowed connection open until after the assertion.
+- After removing a metric-producing source or unregistering an observable callback, call
+  `testing().clearData()` and then use `waitAndAssertMetrics(..., AbstractIterableAssert::isEmpty)`
+  for absence checks. Do not add an exporter-interval sleep before or after `clearData()` solely to
+  wait for metrics; the test runners force-flush metrics when reading them.
+
 ## Attribute Assertion `satisfies()` Lambda Parameters
 
 **Attribute-assertion `satisfies()` lambda parameters are `AbstractAssert` instances, not raw

instrumentation/clickhouse/clickhouse-client-common-0.5/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/clickhouse/client/common/v0_5/ClickHouseAttributesGetter.java

@@ -15,8 +15,7 @@
 import java.util.function.Function;
 import javax.annotation.Nullable;
 
-final class ClickHouseAttributesGetter
-    implements SqlClientAttributesGetter<ClickHouseDbRequest, Void> {
+class ClickHouseAttributesGetter implements SqlClientAttributesGetter<ClickHouseDbRequest, Void> {
 
   private final Function<Throwable, String> errorCodeExtractor;
 

instrumentation/play/play-ws/play-ws-common-1.0/testing/src/main/java/io/opentelemetry/javaagent/instrumentation/playws/common/v1_0/PlayWsClientBaseTest.java

@@ -44,9 +44,9 @@ abstract class PlayWsClientBaseTest<REQUEST> extends AbstractHttpClientTest<REQU
   @RegisterExtension static final AutoCleanupExtension cleanup = AutoCleanupExtension.create();
 
   private static ActorSystem system;
-  protected static AsyncHttpClient asyncHttpClient;
-  protected static AsyncHttpClient asyncHttpClientWithReadTimeout;
-  protected static ActorMaterializer materializer;
+  static AsyncHttpClient asyncHttpClient;
+  static AsyncHttpClient asyncHttpClientWithReadTimeout;
+  static ActorMaterializer materializer;
 
   @BeforeAll
   static void setupHttpClient() {

instrumentation/vibur-dbcp-11.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/viburdbcp/v11_0/ViburDbcpInstrumentationModule.java

@@ -24,9 +24,8 @@ public ViburDbcpInstrumentationModule() {
   public ElementMatcher.Junction<ClassLoader> classLoaderMatcher() {
     // in 11.0, the ViburDBCPDataSource#getPool() method signature was changed - this is detected by
     // muzzle
-    return hasClassesNamed(
-        // added in 10.0
-        "org.vibur.dbcp.ViburConfig");
+    // added in 10.0
+    return hasClassesNamed("org.vibur.dbcp.ViburConfig");
   }
 
   @Override

instrumentation/vibur-dbcp-11.0/testing/src/main/java/io/opentelemetry/instrumentation/viburdbcp/AbstractViburInstrumentationTest.java

@@ -6,7 +6,6 @@
 package io.opentelemetry.instrumentation.viburdbcp;
 
 import static io.opentelemetry.instrumentation.api.internal.SemconvStability.emitStableDatabaseSemconv;
-import static java.util.concurrent.TimeUnit.MILLISECONDS;
 import static org.mockito.Mockito.when;
 
 import io.opentelemetry.instrumentation.testing.junit.InstrumentationExtension;
@@ -35,7 +34,7 @@ public abstract class AbstractViburInstrumentationTest {
   protected abstract void shutdown(ViburDBCPDataSource viburDataSource);
 
   @Test
-  void shouldReportMetrics() throws SQLException, InterruptedException {
+  void shouldReportMetrics() throws SQLException {
     // given
     when(dataSourceMock.getConnection()).thenReturn(connectionMock);
 
@@ -47,8 +46,6 @@ void shouldReportMetrics() throws SQLException, InterruptedException {
 
     // when
     Connection viburConnection = viburDataSource.getConnection();
-    MILLISECONDS.sleep(100);
-    viburConnection.close();
 
     // then
     DbConnectionPoolMetricsAssertions.create(testing(), INSTRUMENTATION_NAME, "testPool")
@@ -62,15 +59,14 @@ void shouldReportMetrics() throws SQLException, InterruptedException {
         .assertConnectionPoolEmitsMetrics();
 
     // when
+    viburConnection.close();
+
     // this one too shouldn't cause any problems when called more than once
     viburDataSource.close();
     viburDataSource.close();
     shutdown(viburDataSource);
 
-    // sleep exporter interval
-    Thread.sleep(100);
     testing().clearData();
-    Thread.sleep(100);
 
     // then
     String countMetricName =

instrumentation/wicket-8.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/wicket/v8_0/RequestHandlerExecutorInstrumentation.java

@@ -6,6 +6,8 @@
 package io.opentelemetry.javaagent.instrumentation.wicket.v8_0;
 
 import static io.opentelemetry.instrumentation.api.semconv.http.HttpServerRouteSource.CONTROLLER;
+import static io.opentelemetry.javaagent.instrumentation.wicket.v8_0.WicketServerSpanNaming.serverSpanName;
+import static io.opentelemetry.javaagent.instrumentation.wicket.v8_0.WicketServerSpanNaming.serverSpanNameResource;
 import static net.bytebuddy.matcher.ElementMatchers.named;
 import static net.bytebuddy.matcher.ElementMatchers.takesArgument;
 
@@ -43,14 +45,14 @@ public static void onEnter(@Advice.Argument(0) IRequestHandler handler) {
         HttpServerRoute.update(
             Java8BytecodeBridge.currentContext(),
             CONTROLLER,
-            WicketServerSpanNaming.getServerSpanName(),
+            serverSpanName(),
             (IPageClassRequestHandler) handler);
       }
       if (handler instanceof ResourceReferenceRequestHandler) {
         HttpServerRoute.update(
             Java8BytecodeBridge.currentContext(),
             CONTROLLER,
-            WicketServerSpanNaming.getServerSpanNameResource(),
+            serverSpanNameResource(),
             (ResourceReferenceRequestHandler) handler);
       }
     }

instrumentation/wicket-8.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/wicket/v8_0/WicketServerSpanNaming.java

@@ -34,11 +34,11 @@ public class WicketServerSpanNaming {
             return ServletContextPath.prepend(context, filterPath + "/" + resourceName);
           };
 
-  public static HttpServerRouteGetter<IPageClassRequestHandler> getServerSpanName() {
+  public static HttpServerRouteGetter<IPageClassRequestHandler> serverSpanName() {
     return serverSpanName;
   }
 
-  public static HttpServerRouteGetter<ResourceReferenceRequestHandler> getServerSpanNameResource() {
+  public static HttpServerRouteGetter<ResourceReferenceRequestHandler> serverSpanNameResource() {
     return serverSpanNameResource;
   }