Add HTTP server tests for non standard methods (#9446)

open-telemetry · Sep 15, 2023 · 3136916 · 3136916
1 parent 2b2c4ca
commit 3136916
Show file tree

Hide file tree

Showing 65 changed files with 495 additions and 143 deletions.
diff --git a/...src/main/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpServerRoute.java b/...src/main/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpServerRoute.java
@@ -31,13 +31,16 @@ public final class HttpServerRoute {
    */
   public static <REQUEST> ContextCustomizer<REQUEST> create(
       HttpServerAttributesGetter<REQUEST, ?> getter) {
-    return (context, request, startAttributes) -> {
-      if (HttpRouteState.fromContextOrNull(context) != null) {
-        return context;
-      }
-      String method = getter.getHttpRequestMethod(request);
-      return context.with(HttpRouteState.create(method, null, 0));
-    };
+    return builder(getter).build();
+  }
+
+  /**
+   * Returns a new {@link HttpServerRouteBuilder} that can be used to configure the {@link
+   * HttpServerRoute}.
+   */
+  public static <REQUEST> HttpServerRouteBuilder<REQUEST> builder(
+      HttpServerAttributesGetter<REQUEST, ?> getter) {
+    return new HttpServerRouteBuilder<>(getter);
   }
 
   private HttpServerRoute() {}
@@ -147,11 +150,8 @@ private static boolean isBetterRoute(HttpRouteState httpRouteState, String name)
 
   private static void updateSpanName(Span serverSpan, HttpRouteState httpRouteState, String route) {
     String method = httpRouteState.getMethod();
-    // method should never really be null - but in case it for some reason is, we'll rely on the
-    // span name extractor behavior
-    if (method != null) {
-      serverSpan.updateName(method + " " + route);
-    }
+    // method should never really be null
+    serverSpan.updateName(method + " " + route);
   }
 
   /**

diff --git a/...n/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpServerRouteBuilder.java b/...n/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpServerRouteBuilder.java
@@ -0,0 +1,65 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.instrumentation.api.instrumenter.http;
+
+import com.google.errorprone.annotations.CanIgnoreReturnValue;
+import io.opentelemetry.context.Context;
+import io.opentelemetry.instrumentation.api.instrumenter.ContextCustomizer;
+import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter;
+import io.opentelemetry.instrumentation.api.internal.HttpConstants;
+import io.opentelemetry.instrumentation.api.internal.HttpRouteState;
+import java.util.HashSet;
+import java.util.Set;
+
+/** A builder of {@link HttpSpanNameExtractor}. */
+public final class HttpServerRouteBuilder<REQUEST> {
+
+  final HttpServerAttributesGetter<REQUEST, ?> getter;
+  Set<String> knownMethods = HttpConstants.KNOWN_METHODS;
+
+  HttpServerRouteBuilder(HttpServerAttributesGetter<REQUEST, ?> getter) {
+    this.getter = getter;
+  }
+
+  /**
+   * Configures the customizer to recognize an alternative set of HTTP request methods.
+   *
+   * <p>By default, this customizer defines "known" methods as the ones listed in <a
+   * href="https://www.rfc-editor.org/rfc/rfc9110.html#name-methods">RFC9110</a> and the PATCH
+   * method defined in <a href="https://www.rfc-editor.org/rfc/rfc5789.html">RFC5789</a>. If an
+   * unknown method is encountered, the customizer will use the value {@value HttpConstants#_OTHER}
+   * instead.
+   *
+   * <p>Note: calling this method <b>overrides</b> the default known method sets completely; it does
+   * not supplement it.
+   *
+   * @param knownMethods A set of recognized HTTP request methods.
+   */
+  @CanIgnoreReturnValue
+  public HttpServerRouteBuilder<REQUEST> setKnownMethods(Set<String> knownMethods) {
+    this.knownMethods = new HashSet<>(knownMethods);
+    return this;
+  }
+
+  /**
+   * Returns a {@link ContextCustomizer} that initializes an {@link HttpServerRoute} in the {@link
+   * Context} returned from {@link Instrumenter#start(Context, Object)}. The returned customizer is
+   * configured with the settings of this {@link HttpServerRouteBuilder}.
+   */
+  public ContextCustomizer<REQUEST> build() {
+    Set<String> knownMethods = new HashSet<>(this.knownMethods);
+    return (context, request, startAttributes) -> {
+      if (HttpRouteState.fromContextOrNull(context) != null) {
+        return context;
+      }
+      String method = getter.getHttpRequestMethod(request);
+      if (method == null || !knownMethods.contains(method)) {
+        method = "HTTP";
+      }
+      return context.with(HttpRouteState.create(method, null, 0));
+    };
+  }
+}
diff --git a/...test/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpServerRouteTest.java b/...test/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpServerRouteTest.java
@@ -6,6 +6,7 @@
 package io.opentelemetry.instrumentation.api.instrumenter.http;
 
 import static io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.assertThat;
+import static java.util.Collections.singletonList;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertNull;
 import static org.mockito.Mockito.when;
@@ -14,6 +15,7 @@
 import io.opentelemetry.context.Context;
 import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter;
 import io.opentelemetry.sdk.testing.junit5.OpenTelemetryExtension;
+import java.util.HashSet;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.ExtendWith;
@@ -33,7 +35,10 @@ class HttpServerRouteTest {
   void setUp() {
     instrumenter =
         Instrumenter.<String, Void>builder(testing.getOpenTelemetry(), "test", s -> s)
-            .addContextCustomizer(HttpServerRoute.create(getter))
+            .addContextCustomizer(
+                HttpServerRoute.builder(getter)
+                    .setKnownMethods(new HashSet<>(singletonList("GET")))
+                    .build())
             .buildInstrumenter();
   }
 
@@ -158,7 +163,7 @@ void shouldNotUpdateRoute_worseMatch() {
   }
 
   @Test
-  void shouldNotUpdateSpanName_noMethod() {
+  void shouldUseHttp_noMethod() {
     when(getter.getHttpRequestMethod("test")).thenReturn(null);
 
     Context context = instrumenter.start(Context.root(), "test");
@@ -169,6 +174,23 @@ void shouldNotUpdateSpanName_noMethod() {
     instrumenter.end(context, "test", null, null);
 
     assertEquals("/get/:id", HttpServerRoute.get(context));
-    assertThat(testing.getSpans()).satisfiesExactly(span -> assertThat(span).hasName("test"));
+    assertThat(testing.getSpans())
+        .satisfiesExactly(span -> assertThat(span).hasName("HTTP /get/:id"));
+  }
+
+  @Test
+  void shouldUseHttp_unknownMethod() {
+    when(getter.getHttpRequestMethod("test")).thenReturn("POST");
+
+    Context context = instrumenter.start(Context.root(), "test");
+    assertNull(HttpServerRoute.get(context));
+
+    HttpServerRoute.update(context, HttpServerRouteSource.SERVER, "/get/:id");
+
+    instrumenter.end(context, "test", null, null);
+
+    assertEquals("/get/:id", HttpServerRoute.get(context));
+    assertThat(testing.getSpans())
+        .satisfiesExactly(span -> assertThat(span).hasName("HTTP /get/:id"));
   }
 }
diff --git a/.../io/opentelemetry/javaagent/instrumentation/akkahttp/server/AkkaHttpServerSingletons.java b/.../io/opentelemetry/javaagent/instrumentation/akkahttp/server/AkkaHttpServerSingletons.java
@@ -40,7 +40,10 @@ public final class AkkaHttpServerSingletons {
                     .setKnownMethods(CommonConfig.get().getKnownHttpRequestMethods())
                     .build())
             .addOperationMetrics(HttpServerMetrics.get())
-            .addContextCustomizer(HttpServerRoute.create(httpAttributesGetter));
+            .addContextCustomizer(
+                HttpServerRoute.builder(httpAttributesGetter)
+                    .setKnownMethods(CommonConfig.get().getKnownHttpRequestMethods())
+                    .build());
     if (CommonConfig.get().shouldEmitExperimentalHttpServerMetrics()) {
       builder.addOperationMetrics(HttpServerExperimentalMetrics.get());
     }

diff --git a/...ntelemetry/javaagent/instrumentation/akkahttp/AbstractHttpServerInstrumentationTest.scala b/...ntelemetry/javaagent/instrumentation/akkahttp/AbstractHttpServerInstrumentationTest.scala
@@ -35,5 +35,7 @@ abstract class AbstractHttpServerInstrumentationTest
           t != ServerEndpoint.EXCEPTION
       }
     )
+    // instrumentation does not create a span at all
+    options.disableTestNonStandardHttpMethod
   }
 }
diff --git a/...t/java/io/opentelemetry/javaagent/instrumentation/armeria/v1_3/ArmeriaHttpServerTest.java b/...t/java/io/opentelemetry/javaagent/instrumentation/armeria/v1_3/ArmeriaHttpServerTest.java
@@ -29,5 +29,6 @@ protected void configure(HttpServerTestOptions options) {
     options.setHasResponseCustomizer(
         endpoint -> ServerEndpoint.NOT_FOUND != endpoint && ServerEndpoint.EXCEPTION != endpoint);
     options.setTestHttpPipelining(false);
+    options.setResponseCodeOnNonStandardHttpMethod(405);
   }
 }
diff --git a/.../src/main/java/io/opentelemetry/instrumentation/armeria/v1_3/ArmeriaTelemetryBuilder.java b/.../src/main/java/io/opentelemetry/instrumentation/armeria/v1_3/ArmeriaTelemetryBuilder.java
@@ -24,6 +24,7 @@
 import io.opentelemetry.instrumentation.api.instrumenter.http.HttpServerExperimentalMetrics;
 import io.opentelemetry.instrumentation.api.instrumenter.http.HttpServerMetrics;
 import io.opentelemetry.instrumentation.api.instrumenter.http.HttpServerRoute;
+import io.opentelemetry.instrumentation.api.instrumenter.http.HttpServerRouteBuilder;
 import io.opentelemetry.instrumentation.api.instrumenter.http.HttpSpanNameExtractor;
 import io.opentelemetry.instrumentation.api.instrumenter.http.HttpSpanNameExtractorBuilder;
 import io.opentelemetry.instrumentation.api.instrumenter.http.HttpSpanStatusExtractor;
@@ -62,6 +63,9 @@ public final class ArmeriaTelemetryBuilder {
   private final HttpSpanNameExtractorBuilder<RequestContext> httpServerSpanNameExtractorBuilder =
       HttpSpanNameExtractor.builder(ArmeriaHttpServerAttributesGetter.INSTANCE);
 
+  private final HttpServerRouteBuilder<RequestContext> httpServerRouteBuilder =
+      HttpServerRoute.builder(ArmeriaHttpServerAttributesGetter.INSTANCE);
+
   private Function<
           SpanStatusExtractor<RequestContext, RequestLog>,
           ? extends SpanStatusExtractor<? super RequestContext, ? super RequestLog>>
@@ -175,6 +179,7 @@ public ArmeriaTelemetryBuilder setKnownMethods(Set<String> knownMethods) {
     httpServerAttributesExtractorBuilder.setKnownMethods(knownMethods);
     httpClientSpanNameExtractorBuilder.setKnownMethods(knownMethods);
     httpServerSpanNameExtractorBuilder.setKnownMethods(knownMethods);
+    httpServerRouteBuilder.setKnownMethods(knownMethods);
     return this;
   }
 
@@ -233,7 +238,7 @@ public ArmeriaTelemetry build() {
                 HttpSpanStatusExtractor.create(serverAttributesGetter)))
         .addAttributesExtractor(httpServerAttributesExtractorBuilder.build())
         .addOperationMetrics(HttpServerMetrics.get())
-        .addContextCustomizer(HttpServerRoute.create(serverAttributesGetter));
+        .addContextCustomizer(httpServerRouteBuilder.build());
 
     if (peerService != null) {
       clientInstrumenterBuilder.addAttributesExtractor(

diff --git a/...ry/src/test/java/io/opentelemetry/instrumentation/armeria/v1_3/ArmeriaHttpServerTest.java b/...ry/src/test/java/io/opentelemetry/instrumentation/armeria/v1_3/ArmeriaHttpServerTest.java
@@ -9,6 +9,7 @@
 import io.opentelemetry.instrumentation.testing.junit.InstrumentationExtension;
 import io.opentelemetry.instrumentation.testing.junit.http.AbstractHttpServerTest;
 import io.opentelemetry.instrumentation.testing.junit.http.HttpServerInstrumentationExtension;
+import io.opentelemetry.instrumentation.testing.junit.http.HttpServerTestOptions;
 import java.util.Collections;
 import org.junit.jupiter.api.extension.RegisterExtension;
 
@@ -28,4 +29,11 @@ protected ServerBuilder configureServer(ServerBuilder sb) {
             .build()
             .newServiceDecorator());
   }
+
+  @Override
+  protected void configure(HttpServerTestOptions options) {
+    super.configure(options);
+    // library instrumentation does not create a span at all
+    options.disableTestNonStandardHttpMethod();
+  }
 }
diff --git a/...ain/java/io/opentelemetry/instrumentation/armeria/v1_3/AbstractArmeriaHttpServerTest.java b/...ain/java/io/opentelemetry/instrumentation/armeria/v1_3/AbstractArmeriaHttpServerTest.java
@@ -178,13 +178,13 @@ protected void stopServer(Server server) {
   @Override
   protected void configure(HttpServerTestOptions options) {
     options.setExpectedHttpRoute(
-        endpoint -> {
+        (endpoint, method) -> {
           if (endpoint == ServerEndpoint.NOT_FOUND) {
             // TODO(anuraaga): Revisit this when applying instrumenters to more libraries, Armeria
             // currently reports '/*' which is a fallback route.
             return "/*";
           }
-          return expectedHttpRoute(endpoint);
+          return expectedHttpRoute(endpoint, method);
         });
 
     options.setTestPathParam(true);

diff --git a/instrumentation/dropwizard/dropwizard-testing/src/test/groovy/DropwizardTest.groovy b/instrumentation/dropwizard/dropwizard-testing/src/test/groovy/DropwizardTest.groovy
@@ -83,14 +83,14 @@ class DropwizardTest extends HttpServerTest<DropwizardTestSupport> implements Ag
   }
 
   @Override
-  String expectedHttpRoute(ServerEndpoint endpoint) {
+  String expectedHttpRoute(ServerEndpoint endpoint, String method) {
     switch (endpoint) {
       case NOT_FOUND:
         return getContextPath() + "/*"
       case PATH_PARAM:
         return getContextPath() + "/path/{id}/param"
       default:
-        return super.expectedHttpRoute(endpoint)
+        return super.expectedHttpRoute(endpoint, method)
     }
   }
 

diff --git a/...src/test/scala/io/opentelemetry/javaagent/instrumentation/finatra/FinatraServerTest.scala b/...src/test/scala/io/opentelemetry/javaagent/instrumentation/finatra/FinatraServerTest.scala
@@ -56,6 +56,7 @@ class FinatraServerTest extends AbstractHttpServerTest[HttpServer] {
       override def test(endpoint: ServerEndpoint): Boolean =
         endpoint != ServerEndpoint.NOT_FOUND
     })
+    options.setResponseCodeOnNonStandardHttpMethod(400)
   }
 
   override protected def assertHandlerSpan(

diff --git a/instrumentation/grails-3.0/javaagent/build.gradle.kts b/instrumentation/grails-3.0/javaagent/build.gradle.kts
@@ -65,12 +65,16 @@ configurations.testRuntimeClasspath {
   }
 }
 
+val latestDepTest = findProperty("testLatestDeps") as Boolean
+
 tasks {
   val testStableSemconv by registering(Test::class) {
     jvmArgs("-Dotel.semconv-stability.opt-in=http")
   }
 
   withType<Test>().configureEach {
+    systemProperty("testLatestDeps", latestDepTest)
+
     // required on jdk17
     jvmArgs("--add-opens=java.base/java.lang=ALL-UNNAMED")
     jvmArgs("-XX:+IgnoreUnrecognizedVMOptions")

diff --git a/instrumentation/grails-3.0/javaagent/src/test/java/test/GrailsTest.java b/instrumentation/grails-3.0/javaagent/src/test/java/test/GrailsTest.java
@@ -19,6 +19,7 @@
 import grails.boot.config.GrailsAutoConfiguration;
 import io.opentelemetry.api.common.Attributes;
 import io.opentelemetry.api.trace.SpanKind;
+import io.opentelemetry.instrumentation.api.internal.HttpConstants;
 import io.opentelemetry.instrumentation.testing.junit.InstrumentationExtension;
 import io.opentelemetry.instrumentation.testing.junit.http.AbstractHttpServerTest;
 import io.opentelemetry.instrumentation.testing.junit.http.HttpServerInstrumentationExtension;
@@ -42,6 +43,8 @@
 
 public class GrailsTest extends AbstractHttpServerTest<ConfigurableApplicationContext> {
 
+  static final boolean testLatestDeps = Boolean.getBoolean("testLatestDeps");
+
   @RegisterExtension
   static final InstrumentationExtension testing = HttpServerInstrumentationExtension.forAgent();
 
@@ -64,6 +67,7 @@ protected void configure(HttpServerTestOptions options) {
     options.setHasErrorPageSpans(
         endpoint -> endpoint == ERROR || endpoint == EXCEPTION || endpoint == NOT_FOUND);
     options.setTestPathParam(true);
+    options.setResponseCodeOnNonStandardHttpMethod(testLatestDeps ? 200 : 501);
   }
 
   @SpringBootApplication
@@ -106,7 +110,12 @@ private static Class<?> load(String name) {
   }
 
   @Override
-  public String expectedHttpRoute(ServerEndpoint endpoint) {
+  public String expectedHttpRoute(ServerEndpoint endpoint, String method) {
+    if (HttpConstants._OTHER.equals(method)) {
+      return testLatestDeps
+          ? getContextPath() + "/test" + endpoint.getPath()
+          : getContextPath() + "/*";
+    }
     if (PATH_PARAM.equals(endpoint)) {
       return getContextPath() + "/test/path";
     } else if (QUERY_PARAM.equals(endpoint)) {

diff --git a/...t/src/main/java/io/opentelemetry/javaagent/instrumentation/grizzly/GrizzlySingletons.java b/...t/src/main/java/io/opentelemetry/javaagent/instrumentation/grizzly/GrizzlySingletons.java
@@ -54,7 +54,10 @@ public final class GrizzlySingletons {
                         .init(context))
             .addContextCustomizer(
                 (context, httpRequestPacket, startAttributes) -> GrizzlyErrorHolder.init(context))
-            .addContextCustomizer(HttpServerRoute.create(httpAttributesGetter))
+            .addContextCustomizer(
+                HttpServerRoute.builder(httpAttributesGetter)
+                    .setKnownMethods(CommonConfig.get().getKnownHttpRequestMethods())
+                    .build())
             .buildServerInstrumenter(HttpRequestHeadersGetter.INSTANCE);
   }
 

diff --git a/...rumentation/jaxrs/jaxrs-common/testing/src/main/groovy/AbstractJaxRsHttpServerTest.groovy b/...rumentation/jaxrs/jaxrs-common/testing/src/main/groovy/AbstractJaxRsHttpServerTest.groovy
@@ -229,7 +229,6 @@ abstract class AbstractJaxRsHttpServerTest<S> extends HttpServerTest<S> implemen
                   String traceID = null,
                   String parentID = null,
                   String method = "GET",
-                  Long responseContentLength = null,
                   ServerEndpoint endpoint = SUCCESS,
                   String spanID = null) {
     serverSpan(trace, index, traceID, parentID, spanID, method,

diff --git a/...0/library/src/main/kotlin/io/opentelemetry/instrumentation/ktor/v1_0/KtorServerTracing.kt b/...0/library/src/main/kotlin/io/opentelemetry/instrumentation/ktor/v1_0/KtorServerTracing.kt
@@ -40,6 +40,8 @@ class KtorServerTracing private constructor(
 
     internal val httpSpanNameExtractorBuilder = HttpSpanNameExtractor.builder(KtorHttpServerAttributesGetter.INSTANCE)
 
+    internal val httpServerRouteBuilder = HttpServerRoute.builder(KtorHttpServerAttributesGetter.INSTANCE)
+
     internal var statusExtractor:
       (SpanStatusExtractor<ApplicationRequest, ApplicationResponse>) -> SpanStatusExtractor<in ApplicationRequest, in ApplicationResponse> = { a -> a }
 
@@ -77,6 +79,7 @@ class KtorServerTracing private constructor(
     fun setKnownMethods(knownMethods: Set<String>) {
       httpAttributesExtractorBuilder.setKnownMethods(knownMethods)
       httpSpanNameExtractorBuilder.setKnownMethods(knownMethods)
+      httpServerRouteBuilder.setKnownMethods(knownMethods)
     }
 
     internal fun isOpenTelemetryInitialized(): Boolean = this::openTelemetry.isInitialized
@@ -124,7 +127,7 @@ class KtorServerTracing private constructor(
         setSpanStatusExtractor(configuration.statusExtractor(HttpSpanStatusExtractor.create(httpAttributesGetter)))
         addAttributesExtractor(configuration.httpAttributesExtractorBuilder.build())
         addOperationMetrics(HttpServerMetrics.get())
-        addContextCustomizer(HttpServerRoute.create(httpAttributesGetter))
+        addContextCustomizer(configuration.httpServerRouteBuilder.build())
       }
 
       val instrumenter = InstrumenterUtil.buildUpstreamInstrumenter(