Convert Spring Web MVC library instrumentation to Instrumenter API (#4258)

* Convert Spring Web MVC library instrumentation to Instrumenter API

* Apply suggestions from code review

Co-authored-by: Lauri Tulmin <tulmin@gmail.com>

* improve the README a bit

* StatusCodeExtractor

Co-authored-by: Lauri Tulmin <tulmin@gmail.com>

instrumentation/spring/spring-boot-autoconfigure/src/main/java/io/opentelemetry/instrumentation/spring/autoconfigure/webmvc/WebMvcFilterAutoConfiguration.java

@@ -6,7 +6,8 @@
 package io.opentelemetry.instrumentation.spring.autoconfigure.webmvc;
 
 import io.opentelemetry.api.OpenTelemetry;
-import io.opentelemetry.instrumentation.spring.webmvc.WebMvcTracingFilter;
+import io.opentelemetry.instrumentation.spring.webmvc.SpringWebMvcTracing;
+import javax.servlet.Filter;
 import org.springframework.boot.autoconfigure.condition.ConditionalOnClass;
 import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty;
 import org.springframework.boot.context.properties.EnableConfigurationProperties;
@@ -14,7 +15,7 @@ import org.springframework.context.annotation.Bean;
 import org.springframework.context.annotation.Configuration;
 import org.springframework.web.filter.OncePerRequestFilter;
 
-/** Configures {@link WebMvcTracingFilter} for tracing. */
+/** Configures {@link SpringWebMvcTracing} for tracing. */
 @Configuration
 @EnableConfigurationProperties(WebMvcProperties.class)
 @ConditionalOnProperty(prefix = "otel.springboot.web", name = "enabled", matchIfMissing = true)
@@ -22,7 +23,7 @@ import org.springframework.web.filter.OncePerRequestFilter;
 public class WebMvcFilterAutoConfiguration {
 
   @Bean
-  public WebMvcTracingFilter otelWebMvcTracingFilter(OpenTelemetry openTelemetry) {
-    return new WebMvcTracingFilter(openTelemetry);
+  public Filter otelWebMvcTracingFilter(OpenTelemetry openTelemetry) {
+    return SpringWebMvcTracing.create(openTelemetry).newServletFilter();
   }
 }

instrumentation/spring/spring-boot-autoconfigure/src/test/java/io/opentelemetry/instrumentation/spring/autoconfigure/webmvc/WebMvcFilterAutoConfigurationTest.java

@@ -9,7 +9,7 @@ import static org.assertj.core.api.Assertions.assertThat;
 
 import io.opentelemetry.api.GlobalOpenTelemetry;
 import io.opentelemetry.instrumentation.spring.autoconfigure.OpenTelemetryAutoConfiguration;
-import io.opentelemetry.instrumentation.spring.webmvc.WebMvcTracingFilter;
+import javax.servlet.Filter;
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.DisplayName;
 import org.junit.jupiter.api.Test;
@@ -36,8 +36,7 @@ class WebMvcFilterAutoConfigurationTest {
         .withPropertyValues("otel.springboot.web.enabled=true")
         .run(
             context ->
-                assertThat(context.getBean("otelWebMvcTracingFilter", WebMvcTracingFilter.class))
-                    .isNotNull());
+                assertThat(context.getBean("otelWebMvcTracingFilter", Filter.class)).isNotNull());
   }
 
   @Test
@@ -53,7 +52,6 @@ class WebMvcFilterAutoConfigurationTest {
   void noProperty() {
     this.contextRunner.run(
         context ->
-            assertThat(context.getBean("otelWebMvcTracingFilter", WebMvcTracingFilter.class))
-                .isNotNull());
+            assertThat(context.getBean("otelWebMvcTracingFilter", Filter.class)).isNotNull());
   }
 }

instrumentation/spring/spring-webmvc-3.1/library/README.md

@@ -10,7 +10,7 @@ Replace `SPRING_VERSION` with the version of spring you're using.
  - `Minimum version: 3.1`
 
 Replace `OPENTELEMETRY_VERSION` with the latest stable [release](https://mvnrepository.com/artifact/io.opentelemetry).
- - `Minimum version: 0.8.0`
+ - `Minimum version: 1.7.0`
 
 For Maven add to your `pom.xml`:
 
@@ -60,14 +60,16 @@ implementation("org.springframework:spring-webmvc:SPRING_VERSION")
 
 ### Features
 
-#### WebMvcTracingFilter
+#### SpringWebMvcTracing
 
-WebMvcTracingFilter adds OpenTelemetry server spans to requests processed by request dispatch, on any spring servlet container. An example is shown below:
+`SpringWebMvcTracing` adds OpenTelemetry server spans to requests processed by request dispatch, on any spring servlet container. An example is shown below:
 
-##### Usage
+##### Usage in Spring Boot
+
+Spring Boot allows servlet `Filter`s to be registered as beans:
 
 ```java
-import io.opentelemetry.instrumentation.spring.webmvc.WebMvcTracingFilter
+import io.opentelemetry.instrumentation.spring.webmvc.SpringWebMvcTracing;
 import io.opentelemetry.api.trace.Tracer;
 
 import org.springframework.beans.factory.annotation.Autowired;
@@ -79,12 +81,12 @@ import org.springframework.web.client.RestTemplate;
 public class WebMvcTracingFilterConfig {
 
    @Bean
-   public WebMvcTracingFilter webMvcTracingFilter(Tracer tracer) {
-      return new WebMvcTracingFilter(tracer);
+   public Filter webMvcTracingFilter(OpenTelemetry openTelemetry) {
+      return SpringWebMvcTracing.create(openTelemetry).newServletFilter();
    }
 }
 ```
 
 ### Starter Guide
 
-Check out the opentelemetry [quick start](https://github.com/open-telemetry/opentelemetry-java/blob/master/QUICKSTART.md) to learn more about OpenTelemetry instrumentation.
+Check out the OpenTelemetry [quick start](https://github.com/open-telemetry/opentelemetry-java/blob/master/QUICKSTART.md) to learn more about OpenTelemetry instrumentation.

instrumentation/spring/spring-webmvc-3.1/library/src/main/java/io/opentelemetry/instrumentation/spring/webmvc/SpringWebMvcHttpAttributesExtractor.java

@@ -0,0 +1,96 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.instrumentation.spring.webmvc;
+
+import io.opentelemetry.instrumentation.api.instrumenter.http.HttpServerAttributesExtractor;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+import org.checkerframework.checker.nullness.qual.Nullable;
+
+final class SpringWebMvcHttpAttributesExtractor
+    extends HttpServerAttributesExtractor<HttpServletRequest, HttpServletResponse> {
+  @Override
+  protected @Nullable String method(HttpServletRequest request) {
+    return request.getMethod();
+  }
+
+  @Override
+  protected @Nullable String userAgent(HttpServletRequest request) {
+    return request.getHeader("user-agent");
+  }
+
+  @Override
+  protected @Nullable Long requestContentLength(
+      HttpServletRequest request, @Nullable HttpServletResponse response) {
+    return null;
+  }
+
+  @Override
+  protected @Nullable Long requestContentLengthUncompressed(
+      HttpServletRequest request, @Nullable HttpServletResponse response) {
+    return null;
+  }
+
+  @Override
+  protected @Nullable String flavor(
+      HttpServletRequest request, @Nullable HttpServletResponse response) {
+    return request.getProtocol();
+  }
+
+  @Override
+  protected @Nullable Integer statusCode(HttpServletRequest request, HttpServletResponse response) {
+    // set in StatusCodeExtractor
+    return null;
+  }
+
+  @Override
+  protected @Nullable Long responseContentLength(
+      HttpServletRequest request, HttpServletResponse response) {
+    return null;
+  }
+
+  @Override
+  protected @Nullable Long responseContentLengthUncompressed(
+      HttpServletRequest request, HttpServletResponse response) {
+    return null;
+  }
+
+  @Override
+  protected @Nullable String url(HttpServletRequest request) {
+    return null;
+  }
+
+  @Override
+  protected @Nullable String target(HttpServletRequest request) {
+    String target = request.getRequestURI();
+    String queryString = request.getQueryString();
+    if (queryString != null) {
+      target += "?" + queryString;
+    }
+    return target;
+  }
+
+  @Override
+  protected @Nullable String host(HttpServletRequest request) {
+    return request.getHeader("host");
+  }
+
+  @Override
+  protected @Nullable String route(HttpServletRequest request) {
+    return null;
+  }
+
+  @Override
+  protected @Nullable String scheme(HttpServletRequest request) {
+    return request.getScheme();
+  }
+
+  @Override
+  protected @Nullable String serverName(
+      HttpServletRequest request, @Nullable HttpServletResponse response) {
+    return null;
+  }
+}

instrumentation/spring/spring-webmvc-3.1/library/src/main/java/io/opentelemetry/instrumentation/spring/webmvc/SpringWebMvcNetAttributesExtractor.java

@@ -0,0 +1,37 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.instrumentation.spring.webmvc;
+
+import io.opentelemetry.instrumentation.api.instrumenter.net.NetAttributesExtractor;
+import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+import org.checkerframework.checker.nullness.qual.Nullable;
+
+final class SpringWebMvcNetAttributesExtractor
+    extends NetAttributesExtractor<HttpServletRequest, HttpServletResponse> {
+  @Override
+  public String transport(HttpServletRequest request) {
+    return SemanticAttributes.NetTransportValues.IP_TCP;
+  }
+
+  @Override
+  public @Nullable String peerName(
+      HttpServletRequest request, @Nullable HttpServletResponse response) {
+    return request.getRemoteHost();
+  }
+
+  @Override
+  public Integer peerPort(HttpServletRequest request, @Nullable HttpServletResponse response) {
+    return request.getRemotePort();
+  }
+
+  @Override
+  public @Nullable String peerIp(
+      HttpServletRequest request, @Nullable HttpServletResponse response) {
+    return request.getRemoteAddr();
+  }
+}

instrumentation/spring/spring-webmvc-3.1/library/src/main/java/io/opentelemetry/instrumentation/spring/webmvc/SpringWebMvcServerTracer.java

@@ -1,79 +0,0 @@
-/*
- * Copyright The OpenTelemetry Authors
- * SPDX-License-Identifier: Apache-2.0
- */
-
-package io.opentelemetry.instrumentation.spring.webmvc;
-
-import io.opentelemetry.api.OpenTelemetry;
-import io.opentelemetry.context.Context;
-import io.opentelemetry.context.propagation.TextMapGetter;
-import io.opentelemetry.instrumentation.api.tracer.HttpServerTracer;
-import io.opentelemetry.instrumentation.servlet.javax.JavaxHttpServletRequestGetter;
-import javax.servlet.http.HttpServletRequest;
-import javax.servlet.http.HttpServletResponse;
-
-class SpringWebMvcServerTracer
-    extends HttpServerTracer<
-        HttpServletRequest, HttpServletResponse, HttpServletRequest, HttpServletRequest> {
-
-  SpringWebMvcServerTracer(OpenTelemetry openTelemetry) {
-    super(openTelemetry);
-  }
-
-  @Override
-  protected Integer peerPort(HttpServletRequest request) {
-    return request.getRemotePort();
-  }
-
-  @Override
-  protected String peerHostIp(HttpServletRequest request) {
-    return request.getRemoteAddr();
-  }
-
-  @Override
-  protected TextMapGetter<HttpServletRequest> getGetter() {
-    return JavaxHttpServletRequestGetter.GETTER;
-  }
-
-  @Override
-  protected String url(HttpServletRequest request) {
-    return request.getRequestURI();
-  }
-
-  @Override
-  protected String method(HttpServletRequest request) {
-    return request.getMethod();
-  }
-
-  @Override
-  protected String requestHeader(HttpServletRequest httpServletRequest, String name) {
-    return httpServletRequest.getHeader(name);
-  }
-
-  @Override
-  protected int responseStatus(HttpServletResponse httpServletResponse) {
-    return httpServletResponse.getStatus();
-  }
-
-  @Override
-  protected void attachServerContext(Context context, HttpServletRequest request) {
-    request.setAttribute(CONTEXT_ATTRIBUTE, context);
-  }
-
-  @Override
-  protected String flavor(HttpServletRequest connection, HttpServletRequest request) {
-    return connection.getProtocol();
-  }
-
-  @Override
-  public Context getServerContext(HttpServletRequest request) {
-    Object context = request.getAttribute(CONTEXT_ATTRIBUTE);
-    return context instanceof Context ? (Context) context : null;
-  }
-
-  @Override
-  protected String getInstrumentationName() {
-    return "io.opentelemetry.spring-webmvc-3.1";
-  }
-}

instrumentation/spring/spring-webmvc-3.1/library/src/main/java/io/opentelemetry/instrumentation/spring/webmvc/SpringWebMvcTracing.java

@@ -0,0 +1,40 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.instrumentation.spring.webmvc;
+
+import io.opentelemetry.api.OpenTelemetry;
+import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter;
+import javax.servlet.Filter;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+
+/** Entrypoint for tracing Spring Web MVC apps. */
+public final class SpringWebMvcTracing {
+
+  /** Returns a new {@link SpringWebMvcTracing} configured with the given {@link OpenTelemetry}. */
+  public static SpringWebMvcTracing create(OpenTelemetry openTelemetry) {
+    return newBuilder(openTelemetry).build();
+  }
+
+  /**
+   * Returns a new {@link SpringWebMvcTracingBuilder} configured with the given {@link
+   * OpenTelemetry}.
+   */
+  public static SpringWebMvcTracingBuilder newBuilder(OpenTelemetry openTelemetry) {
+    return new SpringWebMvcTracingBuilder(openTelemetry);
+  }
+
+  private final Instrumenter<HttpServletRequest, HttpServletResponse> instrumenter;
+
+  SpringWebMvcTracing(Instrumenter<HttpServletRequest, HttpServletResponse> instrumenter) {
+    this.instrumenter = instrumenter;
+  }
+
+  /** Returns a new {@link Filter} that generates telemetry for received HTTP requests. */
+  public Filter newServletFilter() {
+    return new WebMvcTracingFilter(instrumenter);
+  }
+}

instrumentation/spring/spring-webmvc-3.1/library/src/main/java/io/opentelemetry/instrumentation/spring/webmvc/SpringWebMvcTracingBuilder.java

@@ -0,0 +1,66 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.instrumentation.spring.webmvc;
+
+import io.opentelemetry.api.OpenTelemetry;
+import io.opentelemetry.instrumentation.api.instrumenter.AttributesExtractor;
+import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter;
+import io.opentelemetry.instrumentation.api.instrumenter.http.HttpServerMetrics;
+import io.opentelemetry.instrumentation.api.instrumenter.http.HttpSpanNameExtractor;
+import io.opentelemetry.instrumentation.api.instrumenter.http.HttpSpanStatusExtractor;
+import io.opentelemetry.instrumentation.servlet.javax.JavaxHttpServletRequestGetter;
+import java.util.ArrayList;
+import java.util.List;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+
+/** A builder of {@link SpringWebMvcTracing}. */
+public final class SpringWebMvcTracingBuilder {
+
+  private static final String INSTRUMENTATION_NAME = "io.opentelemetry.spring-webmvc-3.1";
+
+  private final OpenTelemetry openTelemetry;
+  private final List<AttributesExtractor<HttpServletRequest, HttpServletResponse>>
+      additionalExtractors = new ArrayList<>();
+
+  SpringWebMvcTracingBuilder(OpenTelemetry openTelemetry) {
+    this.openTelemetry = openTelemetry;
+  }
+
+  /**
+   * Adds an additional {@link AttributesExtractor} to invoke to set attributes to instrumented
+   * items.
+   */
+  public SpringWebMvcTracingBuilder addAttributesExtractor(
+      AttributesExtractor<HttpServletRequest, HttpServletResponse> attributesExtractor) {
+    additionalExtractors.add(attributesExtractor);
+    return this;
+  }
+
+  /**
+   * Returns a new {@link SpringWebMvcTracing} with the settings of this {@link
+   * SpringWebMvcTracingBuilder}.
+   */
+  public SpringWebMvcTracing build() {
+    SpringWebMvcHttpAttributesExtractor httpAttributesExtractor =
+        new SpringWebMvcHttpAttributesExtractor();
+
+    Instrumenter<HttpServletRequest, HttpServletResponse> instrumenter =
+        Instrumenter.<HttpServletRequest, HttpServletResponse>newBuilder(
+                openTelemetry,
+                INSTRUMENTATION_NAME,
+                HttpSpanNameExtractor.create(httpAttributesExtractor))
+            .setSpanStatusExtractor(HttpSpanStatusExtractor.create(httpAttributesExtractor))
+            .addAttributesExtractor(httpAttributesExtractor)
+            .addAttributesExtractor(new StatusCodeExtractor())
+            .addAttributesExtractor(new SpringWebMvcNetAttributesExtractor())
+            .addAttributesExtractors(additionalExtractors)
+            .addRequestMetrics(HttpServerMetrics.get())
+            .newServerInstrumenter(JavaxHttpServletRequestGetter.GETTER);
+
+    return new SpringWebMvcTracing(instrumenter);
+  }
+}

instrumentation/spring/spring-webmvc-3.1/library/src/main/java/io/opentelemetry/instrumentation/spring/webmvc/StatusCodeExtractor.java

@@ -0,0 +1,43 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.instrumentation.spring.webmvc;
+
+import io.opentelemetry.api.common.AttributesBuilder;
+import io.opentelemetry.instrumentation.api.instrumenter.AttributesExtractor;
+import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+import org.checkerframework.checker.nullness.qual.Nullable;
+
+final class StatusCodeExtractor
+    extends AttributesExtractor<HttpServletRequest, HttpServletResponse> {
+
+  @Override
+  protected void onStart(AttributesBuilder attributes, HttpServletRequest httpServletRequest) {}
+
+  @Override
+  protected void onEnd(
+      AttributesBuilder attributes,
+      HttpServletRequest httpServletRequest,
+      @Nullable HttpServletResponse response,
+      @Nullable Throwable error) {
+    if (response != null) {
+      long statusCode;
+      // if response is not committed and there is a throwable set status to 500 /
+      // INTERNAL_SERVER_ERROR, due to servlet spec
+      // https://javaee.github.io/servlet-spec/downloads/servlet-4.0/servlet-4_0_FINAL.pdf:
+      // "If a servlet generates an error that is not handled by the error page mechanism as
+      // described above, the container must ensure to send a response with status 500."
+      if (!response.isCommitted() && error != null) {
+        statusCode = 500;
+      } else {
+        statusCode = response.getStatus();
+      }
+
+      set(attributes, SemanticAttributes.HTTP_STATUS_CODE, statusCode);
+    }
+  }
+}

instrumentation/spring/spring-webmvc-3.1/library/src/main/java/io/opentelemetry/instrumentation/spring/webmvc/WebMvcTracingFilter.java

@@ -5,9 +5,9 @@
 
 package io.opentelemetry.instrumentation.spring.webmvc;
 
-import io.opentelemetry.api.OpenTelemetry;
 import io.opentelemetry.context.Context;
 import io.opentelemetry.context.Scope;
+import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter;
 import java.io.IOException;
 import javax.servlet.FilterChain;
 import javax.servlet.ServletException;
@@ -16,26 +16,31 @@ import javax.servlet.http.HttpServletResponse;
 import org.springframework.core.Ordered;
 import org.springframework.web.filter.OncePerRequestFilter;
 
-public class WebMvcTracingFilter extends OncePerRequestFilter implements Ordered {
+final class WebMvcTracingFilter extends OncePerRequestFilter implements Ordered {
 
-  private static final String FILTER_CLASS = "WebMVCTracingFilter";
-  private static final String FILTER_METHOD = "doFilterInternal";
-  private final SpringWebMvcServerTracer tracer;
+  private final Instrumenter<HttpServletRequest, HttpServletResponse> instrumenter;
 
-  public WebMvcTracingFilter(OpenTelemetry openTelemetry) {
-    this.tracer = new SpringWebMvcServerTracer(openTelemetry);
+  WebMvcTracingFilter(Instrumenter<HttpServletRequest, HttpServletResponse> instrumenter) {
+    this.instrumenter = instrumenter;
   }
 
   @Override
   public void doFilterInternal(
       HttpServletRequest request, HttpServletResponse response, FilterChain filterChain)
       throws ServletException, IOException {
-    Context ctx = tracer.startSpan(request, request, request, FILTER_CLASS + "." + FILTER_METHOD);
-    try (Scope ignored = ctx.makeCurrent()) {
+
+    Context parentContext = Context.current();
+    if (!instrumenter.shouldStart(parentContext, request)) {
       filterChain.doFilter(request, response);
-      tracer.end(ctx, response);
+      return;
+    }
+
+    Context context = instrumenter.start(parentContext, request);
+    try (Scope ignored = context.makeCurrent()) {
+      filterChain.doFilter(request, response);
+      instrumenter.end(context, request, response, null);
     } catch (Throwable t) {
-      tracer.endExceptionally(ctx, t, response);
+      instrumenter.end(context, request, response, t);
       throw t;
     }
   }