Code review sweep (run 25130600248) (#18414)

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/testing-general-patterns.md

@@ -48,10 +48,13 @@
 - In JUnit tests, when an `AutoCloseable` is intended to remain live for most or all of the test
   and only needs cleanup at test end, prefer `AutoCleanupExtension` with `deferCleanup(...)`
   over wrapping most of the test body in try-with-resources.
-- Use `AutoCleanupExtension.deferAfterAll(...)` only for cleanup actions registered
-  dynamically (from inside test methods or other runtime-conditional setup). For a
-  fixed set of class-scoped fields known at class-init time, use a plain `@AfterAll`
-  that closes each field directly.
+- For class-scoped cleanup, prefer `AutoCleanupExtension.deferAfterAll(...)`
+  registered next to the resource's construction in `@BeforeAll`. The same
+  applies to per-test cleanup with `deferCleanup(...)` registered in
+  `@BeforeEach` or in the test body. This keeps creation and cleanup together
+  and avoids a separate `@AfterAll` / `@AfterEach` that re-references and
+  null-checks the field. Prefer a plain `@AfterAll` / `@AfterEach` when
+  null-checking the field is not needed for correct cleanup.
 - Reuse an existing `cleanup` extension when one is already in scope.
   Otherwise, add a `@RegisterExtension` field when the deferred-cleanup pattern improves
   clarity or avoids wrapping most of the test body.

instrumentation/aws-sdk/aws-sdk-1.11/testing/src/main/java/io/opentelemetry/instrumentation/awssdk/v1_11/AbstractBaseAwsClientTest.java

@@ -41,15 +41,15 @@
 
 @SuppressWarnings("deprecation") // using deprecated semconv
 public abstract class AbstractBaseAwsClientTest {
+  protected static final AWSStaticCredentialsProvider credentialsProvider =
+      new AWSStaticCredentialsProvider(new AnonymousAWSCredentials());
+  protected static final MockWebServerExtension server = new MockWebServerExtension();
+  protected static AwsClientBuilder.EndpointConfiguration endpoint;
+
   protected abstract InstrumentationExtension testing();
 
   protected abstract boolean hasRequestId();
 
-  protected static MockWebServerExtension server = new MockWebServerExtension();
-  protected static AwsClientBuilder.EndpointConfiguration endpoint;
-  protected static final AWSStaticCredentialsProvider credentialsProvider =
-      new AWSStaticCredentialsProvider(new AnonymousAWSCredentials());
-
   @BeforeAll
   static void setUp() {
     System.setProperty(SDKGlobalConfiguration.ACCESS_KEY_SYSTEM_PROPERTY, "my-access-key");

instrumentation/aws-sdk/aws-sdk-1.11/testing/src/main/java/io/opentelemetry/instrumentation/awssdk/v1_11/AbstractLambdaClientTest.java

@@ -120,6 +120,28 @@ protected boolean hasRequestId() {
     return false;
   }
 
+  @ParameterizedTest
+  @MethodSource("provideArguments")
+  void testSendRequestWithMockedResponse(
+      String operation,
+      String method,
+      String responseBody,
+      List<AttributeAssertion> additionalAttributes,
+      Function<AWSLambda, Object> call)
+      throws ReflectiveOperationException {
+    AWSLambdaClientBuilder clientBuilder = AWSLambdaClientBuilder.standard();
+    AWSLambda client =
+        configureClient(clientBuilder)
+            .withEndpointConfiguration(endpoint)
+            .withCredentials(credentialsProvider)
+            .build();
+
+    server.enqueue(HttpResponse.of(HttpStatus.OK, MediaType.PLAIN_TEXT_UTF_8, responseBody));
+    Object response = call.apply(client);
+    assertRequestWithMockedResponse(
+        response, client, "AWSLambda", operation, method, additionalAttributes);
+  }
+
   private static Stream<Arguments> provideArguments() {
     return Stream.of(
         Arguments.of(
@@ -163,26 +185,4 @@ private static Stream<Arguments> provideArguments() {
                     c.getFunction(
                         new GetFunctionRequest().withFunctionName("lambda-function-name-foo"))));
   }
-
-  @ParameterizedTest
-  @MethodSource("provideArguments")
-  void testSendRequestWithMockedResponse(
-      String operation,
-      String method,
-      String responseBody,
-      List<AttributeAssertion> additionalAttributes,
-      Function<AWSLambda, Object> call)
-      throws ReflectiveOperationException {
-    AWSLambdaClientBuilder clientBuilder = AWSLambdaClientBuilder.standard();
-    AWSLambda client =
-        configureClient(clientBuilder)
-            .withEndpointConfiguration(endpoint)
-            .withCredentials(credentialsProvider)
-            .build();
-
-    server.enqueue(HttpResponse.of(HttpStatus.OK, MediaType.PLAIN_TEXT_UTF_8, responseBody));
-    Object response = call.apply(client);
-    assertRequestWithMockedResponse(
-        response, client, "AWSLambda", operation, method, additionalAttributes);
-  }
 }

instrumentation/aws-sdk/aws-sdk-1.11/testing/src/main/java/io/opentelemetry/instrumentation/awssdk/v1_11/AbstractS3ClientTest.java

@@ -47,11 +47,11 @@
 @SuppressWarnings("deprecation") // using deprecated semconv
 public abstract class AbstractS3ClientTest extends AbstractBaseAwsClientTest {
 
-  public abstract AmazonS3ClientBuilder configureClient(AmazonS3ClientBuilder client);
-
   private final AmazonS3ClientBuilder clientBuilder =
       AmazonS3ClientBuilder.standard().withPathStyleAccessEnabled(true);
 
+  public abstract AmazonS3ClientBuilder configureClient(AmazonS3ClientBuilder client);
+
   @Override
   protected boolean hasRequestId() {
     return false;

instrumentation/aws-sdk/aws-sdk-1.11/testing/src/main/java/io/opentelemetry/instrumentation/awssdk/v1_11/AbstractSqsSuppressReceiveSpansTest.java

@@ -47,15 +47,15 @@
 @SuppressWarnings("deprecation") // using deprecated semconv
 public abstract class AbstractSqsSuppressReceiveSpansTest {
 
+  private static int sqsPort;
+  private static SQSRestServer sqsRestServer;
+  private static AmazonSQSAsync sqsClient;
+
   protected abstract InstrumentationExtension testing();
 
   protected abstract AmazonSQSAsyncClientBuilder configureClient(
       AmazonSQSAsyncClientBuilder client);
 
-  private static int sqsPort;
-  private static SQSRestServer sqsRestServer;
-  private static AmazonSQSAsync sqsClient;
-
   @BeforeEach
   void setUp() {
     sqsPort = PortUtils.findOpenPort();

instrumentation/aws-sdk/aws-sdk-1.11/testing/src/main/java/io/opentelemetry/instrumentation/awssdk/v1_11/AbstractSqsTracingTest.java

@@ -61,15 +61,15 @@
 @SuppressWarnings("deprecation") // using deprecated semconv
 public abstract class AbstractSqsTracingTest {
 
+  private static int sqsPort;
+  private static SQSRestServer sqsRestServer;
+  private static AmazonSQSAsync sqsClient;
+
   protected abstract InstrumentationExtension testing();
 
   protected abstract AmazonSQSAsyncClientBuilder configureClient(
       AmazonSQSAsyncClientBuilder client);
 
-  private static int sqsPort;
-  private static SQSRestServer sqsRestServer;
-  private static AmazonSQSAsync sqsClient;
-
   @BeforeEach
   void setUp() {
     sqsPort = PortUtils.findOpenPort();

instrumentation/aws-sdk/aws-sdk-2.2/javaagent/build.gradle.kts

@@ -9,6 +9,7 @@ muzzle {
     group.set("software.amazon.awssdk")
     module.set("aws-core")
     versions.set("[2.2.0,)")
+    assertInverse.set(true)
     // Used by all SDK services, the only case it isn't is an SDK extension such as a custom HTTP
     // client, which is not target of instrumentation anyways.
     extraDependency("software.amazon.awssdk:protocol-core")
@@ -36,6 +37,7 @@ muzzle {
     group.set("software.amazon.awssdk")
     module.set("sqs")
     versions.set("[2.2.0,)")
+    assertInverse.set(true)
     // Used by all SDK services, the only case it isn't is an SDK extension such as a custom HTTP
     // client, which is not target of instrumentation anyways.
     extraDependency("software.amazon.awssdk:protocol-core")

instrumentation/aws-sdk/aws-sdk-2.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/awssdk/v2_2/BedrockRuntimeInstrumentationModule.java

@@ -46,7 +46,7 @@ public void doTransform(TypeTransformer transformer) {
 
   @SuppressWarnings("unused")
   public static class RegisterAdvice {
-    @Advice.OnMethodExit(suppress = Throwable.class, inline = false)
+    @Advice.OnMethodExit(inline = false)
     public static void onExit() {
       // (indirectly) using BedrockRuntimeImpl class here to make sure it is available from
       // BedrockRuntimeAccess (injected into app classloader) and checked by Muzzle

instrumentation/aws-sdk/aws-sdk-2.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/awssdk/v2_2/LambdaInstrumentationModule.java

@@ -39,7 +39,7 @@ public void doTransform(TypeTransformer transformer) {
 
   @SuppressWarnings("unused")
   public static class RegisterAdvice {
-    @Advice.OnMethodExit(suppress = Throwable.class, inline = false)
+    @Advice.OnMethodExit(inline = false)
     public static void onExit() {
       // (indirectly) using LambdaImpl class here to make sure it is available from LambdaAccess
       // (injected into app classloader) and checked by Muzzle

instrumentation/aws-sdk/aws-sdk-2.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/awssdk/v2_2/SnsInstrumentationModule.java

@@ -36,7 +36,7 @@ public void doTransform(TypeTransformer transformer) {
 
   @SuppressWarnings("unused")
   public static class RegisterAdvice {
-    @Advice.OnMethodExit(suppress = Throwable.class, inline = false)
+    @Advice.OnMethodExit(inline = false)
     public static void onExit() {
       // (indirectly) using SnsImpl class here to make sure it is available from SnsAccess
       // (injected into app classloader) and checked by Muzzle