diff --git a/.github/agents/knowledge/README.md b/.github/agents/knowledge/README.md index 3cb97fe6b3ab..dc937f3f39a5 100644 --- a/.github/agents/knowledge/README.md +++ b/.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 | diff --git a/.github/agents/knowledge/general-rules.md b/.github/agents/knowledge/general-rules.md index 2205d03dc099..2860e4170f46 100644 --- a/.github/agents/knowledge/general-rules.md +++ b/.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` | diff --git a/.github/agents/knowledge/javaagent-singletons-patterns.md b/.github/agents/knowledge/javaagent-singletons-patterns.md index bbfc0b120ac0..bc05261f7d4a 100644 --- a/.github/agents/knowledge/javaagent-singletons-patterns.md +++ b/.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. diff --git a/.github/agents/knowledge/testing-general-patterns.md b/.github/agents/knowledge/testing-general-patterns.md index 3ab19425b340..c70f5a1dfc43 100644 --- a/.github/agents/knowledge/testing-general-patterns.md +++ b/.github/agents/knowledge/testing-general-patterns.md @@ -97,6 +97,20 @@ `equalTo(AttributeKey, 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 diff --git a/instrumentation/clickhouse/clickhouse-client-common-0.5/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/clickhouse/client/common/v0_5/ClickHouseAttributesGetter.java b/instrumentation/clickhouse/clickhouse-client-common-0.5/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/clickhouse/client/common/v0_5/ClickHouseAttributesGetter.java index 2c56a077fdcc..c7538a4cea27 100644 --- a/instrumentation/clickhouse/clickhouse-client-common-0.5/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/clickhouse/client/common/v0_5/ClickHouseAttributesGetter.java +++ b/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 { +class ClickHouseAttributesGetter implements SqlClientAttributesGetter { private final Function errorCodeExtractor; diff --git a/instrumentation/play/play-ws/play-ws-common-1.0/testing/src/main/java/io/opentelemetry/javaagent/instrumentation/playws/common/v1_0/PlayWsClientBaseTest.java b/instrumentation/play/play-ws/play-ws-common-1.0/testing/src/main/java/io/opentelemetry/javaagent/instrumentation/playws/common/v1_0/PlayWsClientBaseTest.java index bf9d2bc5c183..b5452af42e80 100644 --- a/instrumentation/play/play-ws/play-ws-common-1.0/testing/src/main/java/io/opentelemetry/javaagent/instrumentation/playws/common/v1_0/PlayWsClientBaseTest.java +++ b/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 extends AbstractHttpClientTest 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 diff --git a/instrumentation/vibur-dbcp-11.0/testing/src/main/java/io/opentelemetry/instrumentation/viburdbcp/AbstractViburInstrumentationTest.java b/instrumentation/vibur-dbcp-11.0/testing/src/main/java/io/opentelemetry/instrumentation/viburdbcp/AbstractViburInstrumentationTest.java index 0f2e12b0417d..ef91bd1e2eda 100644 --- a/instrumentation/vibur-dbcp-11.0/testing/src/main/java/io/opentelemetry/instrumentation/viburdbcp/AbstractViburInstrumentationTest.java +++ b/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 = diff --git a/instrumentation/wicket-8.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/wicket/v8_0/RequestHandlerExecutorInstrumentation.java b/instrumentation/wicket-8.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/wicket/v8_0/RequestHandlerExecutorInstrumentation.java index 094a79f530a7..f899bc255341 100644 --- a/instrumentation/wicket-8.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/wicket/v8_0/RequestHandlerExecutorInstrumentation.java +++ b/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); } } diff --git a/instrumentation/wicket-8.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/wicket/v8_0/WicketServerSpanNaming.java b/instrumentation/wicket-8.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/wicket/v8_0/WicketServerSpanNaming.java index f62e61edb071..63f89df33a68 100644 --- a/instrumentation/wicket-8.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/wicket/v8_0/WicketServerSpanNaming.java +++ b/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 getServerSpanName() { + public static HttpServerRouteGetter serverSpanName() { return serverSpanName; } - public static HttpServerRouteGetter getServerSpanNameResource() { + public static HttpServerRouteGetter serverSpanNameResource() { return serverSpanNameResource; } diff --git a/instrumentation/wicket-8.0/wicket10-testing/build.gradle.kts b/instrumentation/wicket-8.0/wicket10-testing/build.gradle.kts index a45771640a09..b819365bfc79 100644 --- a/instrumentation/wicket-8.0/wicket10-testing/build.gradle.kts +++ b/instrumentation/wicket-8.0/wicket10-testing/build.gradle.kts @@ -6,7 +6,6 @@ dependencies { library("org.apache.wicket:wicket:10.0.0") testImplementation(project(":instrumentation:wicket-8.0:common-testing")) - testImplementation("org.jsoup:jsoup:1.13.1") testImplementation("org.eclipse.jetty:jetty-server:11.0.0") testImplementation("org.eclipse.jetty:jetty-servlet:11.0.0") diff --git a/instrumentation/xxl-job/xxl-job-1.9.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/xxljob/v1_9_2/XxlJobSingletons.java b/instrumentation/xxl-job/xxl-job-1.9.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/xxljob/v1_9_2/XxlJobSingletons.java index cdb0b83735ad..e7e92a8767cd 100644 --- a/instrumentation/xxl-job/xxl-job-1.9.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/xxljob/v1_9_2/XxlJobSingletons.java +++ b/instrumentation/xxl-job/xxl-job-1.9.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/xxljob/v1_9_2/XxlJobSingletons.java @@ -31,7 +31,7 @@ public static XxlJobHelper helper() { return helper; } - @SuppressWarnings({"Unused", "ReturnValueIgnored"}) + @SuppressWarnings({"unused", "ReturnValueIgnored"}) private static void limitSupportedVersions() { // GLUE_POWERSHELL was added in 1.9.2. Using this constant here ensures that muzzle will disable // this instrumentation on earlier versions where this constant does not exist. diff --git a/instrumentation/xxl-job/xxl-job-common/testing/src/main/java/io/opentelemetry/instrumentation/xxljob/ReflectiveMethodsFactory.java b/instrumentation/xxl-job/xxl-job-common/testing/src/main/java/io/opentelemetry/instrumentation/xxljob/ReflectiveMethodsFactory.java index 508247c4f16a..1f95d64ed982 100644 --- a/instrumentation/xxl-job/xxl-job-common/testing/src/main/java/io/opentelemetry/instrumentation/xxljob/ReflectiveMethodsFactory.java +++ b/instrumentation/xxl-job/xxl-job-common/testing/src/main/java/io/opentelemetry/instrumentation/xxljob/ReflectiveMethodsFactory.java @@ -21,7 +21,7 @@ static Object getTarget() { static Method getMethod() { try { return ReflectObject.class.getMethod("echo", String.class); - } catch (Throwable t) { + } catch (Throwable ignored) { return null; } } @@ -30,7 +30,7 @@ static Method getMethod() { static Method getInitMethod() { try { return ReflectObject.class.getMethod("initMethod"); - } catch (Throwable t) { + } catch (Throwable ignored) { return null; } } @@ -39,7 +39,7 @@ static Method getInitMethod() { static Method getDestroyMethod() { try { return ReflectObject.class.getMethod("destroyMethod"); - } catch (Throwable t) { + } catch (Throwable ignored) { return null; } } diff --git a/instrumentation/zio/zio-2.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/zio/v2_0/TracingSupervisor.java b/instrumentation/zio/zio-2.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/zio/v2_0/TracingSupervisor.java index 3992149fab02..2bdc9c4e0d6b 100644 --- a/instrumentation/zio/zio-2.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/zio/v2_0/TracingSupervisor.java +++ b/instrumentation/zio/zio-2.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/zio/v2_0/TracingSupervisor.java @@ -15,7 +15,8 @@ import zio.ZIO; import zio.ZIO$; -@SuppressWarnings("unchecked") // fine +// ZIO's Supervisor API uses Scala generic signatures that javac cannot verify. +@SuppressWarnings("unchecked") public class TracingSupervisor extends Supervisor { public static final TracingSupervisor INSTANCE = new TracingSupervisor(); @@ -25,7 +26,8 @@ public class TracingSupervisor extends Supervisor { private TracingSupervisor() {} @Override - @SuppressWarnings("rawtypes") // fine + // The upstream Supervisor.value signature returns raw ZIO. + @SuppressWarnings("rawtypes") public ZIO value(Object trace) { return ZIO$.MODULE$.unit(); }