Refactor AttributesExtractor so that it extracts route from Context

instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/AttributesExtractor.java

@@ -7,6 +7,7 @@
 
 import io.opentelemetry.api.common.AttributeKey;
 import io.opentelemetry.api.common.AttributesBuilder;
+import io.opentelemetry.context.Context;
 import io.opentelemetry.instrumentation.api.instrumenter.db.DbAttributesExtractor;
 import io.opentelemetry.instrumentation.api.instrumenter.http.HttpClientAttributesExtractor;
 import io.opentelemetry.instrumentation.api.instrumenter.net.NetServerAttributesExtractor;
@@ -25,21 +26,56 @@
  * @see NetServerAttributesExtractor
  */
 public interface AttributesExtractor<REQUEST, RESPONSE> {
+
+  // TODO: use new methods everywhere
+
+  /**
+   * Extracts attributes from the {@link Context} and the {@link REQUEST} into the {@link
+   * AttributesBuilder} at the beginning of a request.
+   */
+  default void onStart(AttributesBuilder attributes, Context parentContext, REQUEST request) {
+    onStart(attributes, request);
+  }
+
   /**
    * Extracts attributes from the {@link REQUEST} into the {@link AttributesBuilder} at the
    * beginning of a request.
    */
-  void onStart(AttributesBuilder attributes, REQUEST request);
+  // * @deprecated Use {@link #onStart(AttributesBuilder, Context, Object)} instead.
+  // @Deprecated
+  default void onStart(AttributesBuilder attributes, REQUEST request) {
+    throw new UnsupportedOperationException(
+        "This method variant is deprecated and will be removed in the next minor release.");
+  }
+
+  /**
+   * Extracts attributes from the {@link Context}, the {@link REQUEST} and either {@link RESPONSE}
+   * or {@code error} into the {@link AttributesBuilder} at the end of a request.
+   */
+  default void onEnd(
+      AttributesBuilder attributes,
+      Context context,
+      REQUEST request,
+      @Nullable RESPONSE response,
+      @Nullable Throwable error) {
+    onEnd(attributes, request, response, error);
+  }
 
   /**
    * Extracts attributes from the {@link REQUEST} and either {@link RESPONSE} or {@code error} into
    * the {@link AttributesBuilder} at the end of a request.
    */
-  void onEnd(
+  // * @deprecated Use {@link #onEnd(AttributesBuilder, Context, Object, Object, Throwable)}
+  // instead.
+  // @Deprecated
+  default void onEnd(
       AttributesBuilder attributes,
       REQUEST request,
       @Nullable RESPONSE response,
-      @Nullable Throwable error);
+      @Nullable Throwable error) {
+    throw new UnsupportedOperationException(
+        "This method variant is deprecated and will be removed in the next minor release.");
+  }
 
   /**
    * Sets the {@code value} with the given {@code key} to the {@link AttributesBuilder} if {@code

instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/Instrumenter.java

@@ -171,7 +171,7 @@ public Context start(Context parentContext, REQUEST request) {
 
     UnsafeAttributes attributesBuilder = new UnsafeAttributes();
     for (AttributesExtractor<? super REQUEST, ? super RESPONSE> extractor : attributesExtractors) {
-      extractor.onStart(attributesBuilder, request);
+      extractor.onStart(attributesBuilder, parentContext, request);
     }
     Attributes attributes = attributesBuilder;
 
@@ -221,7 +221,7 @@ public void end(
 
     UnsafeAttributes attributes = new UnsafeAttributes();
     for (AttributesExtractor<? super REQUEST, ? super RESPONSE> extractor : attributesExtractors) {
-      extractor.onEnd(attributes, request, response, error);
+      extractor.onEnd(attributes, context, request, response, error);
     }
     span.setAllAttributes(attributes);
 

instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpClientAttributesExtractor.java

@@ -6,6 +6,7 @@
 package io.opentelemetry.instrumentation.api.instrumenter.http;
 
 import io.opentelemetry.api.common.AttributesBuilder;
+import io.opentelemetry.context.Context;
 import io.opentelemetry.instrumentation.api.config.Config;
 import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
 import javax.annotation.Nullable;
@@ -49,18 +50,19 @@ private HttpClientAttributesExtractor(
   }
 
   @Override
-  public void onStart(AttributesBuilder attributes, REQUEST request) {
-    super.onStart(attributes, request);
+  public void onStart(AttributesBuilder attributes, Context parentContext, REQUEST request) {
+    super.onStart(attributes, parentContext, request);
     set(attributes, SemanticAttributes.HTTP_URL, getter.url(request));
   }
 
   @Override
   public void onEnd(
       AttributesBuilder attributes,
+      Context context,
       REQUEST request,
       @Nullable RESPONSE response,
       @Nullable Throwable error) {
-    super.onEnd(attributes, request, response, error);
+    super.onEnd(attributes, context, request, response, error);
     set(attributes, SemanticAttributes.HTTP_FLAVOR, getter.flavor(request, response));
   }
 }

instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpCommonAttributesExtractor.java

@@ -9,6 +9,7 @@
 import static io.opentelemetry.instrumentation.api.instrumenter.http.HttpHeaderAttributes.responseAttributeKey;
 
 import io.opentelemetry.api.common.AttributesBuilder;
+import io.opentelemetry.context.Context;
 import io.opentelemetry.instrumentation.api.instrumenter.AttributesExtractor;
 import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
 import java.util.List;
@@ -32,7 +33,7 @@ abstract class HttpCommonAttributesExtractor<
   }
 
   @Override
-  public void onStart(AttributesBuilder attributes, REQUEST request) {
+  public void onStart(AttributesBuilder attributes, Context parentContext, REQUEST request) {
     set(attributes, SemanticAttributes.HTTP_METHOD, getter.method(request));
     set(attributes, SemanticAttributes.HTTP_USER_AGENT, userAgent(request));
 
@@ -47,6 +48,7 @@ public void onStart(AttributesBuilder attributes, REQUEST request) {
   @Override
   public void onEnd(
       AttributesBuilder attributes,
+      Context context,
       REQUEST request,
       @Nullable RESPONSE response,
       @Nullable Throwable error) {

instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpRouteHolder.java

@@ -20,7 +20,7 @@
  *
  * <p>Usually the route is not accessible when the request processing starts; and needs to be set
  * later, after the instrumented operation starts. This class provides several static methods that
- * allow the isntrumentation author to provide the matching HTTP route to the instrumentation when
+ * allow the instrumentation author to provide the matching HTTP route to the instrumentation when
  * it is discovered.
  */
 public final class HttpRouteHolder {
@@ -52,7 +52,7 @@ private HttpRouteHolder() {}
    * <p>If there is a server span in the context, and the context has been customized with a {@link
    * HttpRouteHolder}, then this method will update the route using the provided {@code httpRoute}
    * if and only if the last {@link HttpRouteSource} to update the route using this method has
-   * strictly lower priority than the provided {@link HttpRouteSource}, and the pased value is
+   * strictly lower priority than the provided {@link HttpRouteSource}, and the passed value is
    * non-null.
    *
    * <p>If there is a server span in the context, and the context has NOT been customized with a
@@ -111,7 +111,9 @@ public static <T, U> void updateHttpRoute(
     if (httpRouteHolder == null) {
       String httpRoute = httpRouteGetter.get(context, arg1, arg2);
       if (httpRoute != null && !httpRoute.isEmpty()) {
-        updateSpanData(serverSpan, httpRoute);
+        // update both span and name, since there's no HttpRouteHolder in the context
+        serverSpan.updateName(httpRoute);
+        serverSpan.setAttribute(SemanticAttributes.HTTP_ROUTE, httpRoute);
       }
       return;
     }
@@ -124,20 +126,17 @@ public static <T, U> void updateHttpRoute(
       if (route != null
           && !route.isEmpty()
           && (!onlyIfBetterRoute || httpRouteHolder.isBetterRoute(route))) {
-        updateSpanData(serverSpan, route);
+
+        // update just the span name - the attribute will be picked up by the
+        // HttpServerAttributesExtractor at the end of request processing
+        serverSpan.updateName(route);
+
         httpRouteHolder.updatedBySourceOrder = source.order;
         httpRouteHolder.route = route;
       }
     }
   }
 
-  // TODO: instead of calling setAttribute() consider storing the route in context end retrieving it
-  // in the AttributesExtractor
-  private static void updateSpanData(Span serverSpan, String route) {
-    serverSpan.updateName(route);
-    serverSpan.setAttribute(SemanticAttributes.HTTP_ROUTE, route);
-  }
-
   // This is used when setting route from a servlet filter to pick the most descriptive (longest)
   // route.
   private boolean isBetterRoute(String name) {
@@ -147,11 +146,11 @@ private boolean isBetterRoute(String name) {
   }
 
   /**
-   * Returns the {@code http.route} attribute value that's stored in the passed {@code context}, or
-   * null if it was not set before.
+   * Returns the {@code http.route} attribute value that's stored in the {@code context}, or null if
+   * it was not set before.
    */
   @Nullable
-  public static String getRoute(Context context) {
+  static String getRoute(Context context) {
     HttpRouteHolder httpRouteHolder = context.get(CONTEXT_KEY);
     return httpRouteHolder == null ? null : httpRouteHolder.route;
   }

instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpServerAttributesExtractor.java

@@ -9,8 +9,10 @@
 import static io.opentelemetry.instrumentation.api.instrumenter.http.ForwarderHeaderParser.extractForwardedFor;
 
 import io.opentelemetry.api.common.AttributesBuilder;
+import io.opentelemetry.context.Context;
 import io.opentelemetry.instrumentation.api.config.Config;
 import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
+import java.util.function.Function;
 import javax.annotation.Nullable;
 
 /**
@@ -42,18 +44,24 @@ public static <REQUEST, RESPONSE> HttpServerAttributesExtractor<REQUEST, RESPONS
   public static <REQUEST, RESPONSE> HttpServerAttributesExtractor<REQUEST, RESPONSE> create(
       HttpServerAttributesGetter<REQUEST, RESPONSE> getter,
       CapturedHttpHeaders capturedHttpHeaders) {
-    return new HttpServerAttributesExtractor<>(getter, capturedHttpHeaders);
+    return new HttpServerAttributesExtractor<>(
+        getter, capturedHttpHeaders, HttpRouteHolder::getRoute);
   }
 
-  private HttpServerAttributesExtractor(
+  private final Function<Context, String> httpRouteHolderGetter;
+
+  // visible for tests
+  HttpServerAttributesExtractor(
       HttpServerAttributesGetter<REQUEST, RESPONSE> getter,
-      CapturedHttpHeaders capturedHttpHeaders) {
+      CapturedHttpHeaders capturedHttpHeaders,
+      Function<Context, String> httpRouteHolderGetter) {
     super(getter, capturedHttpHeaders);
+    this.httpRouteHolderGetter = httpRouteHolderGetter;
   }
 
   @Override
-  public void onStart(AttributesBuilder attributes, REQUEST request) {
-    super.onStart(attributes, request);
+  public void onStart(AttributesBuilder attributes, Context parentContext, REQUEST request) {
+    super.onStart(attributes, parentContext, request);
 
     set(attributes, SemanticAttributes.HTTP_FLAVOR, getter.flavor(request));
     set(attributes, SemanticAttributes.HTTP_SCHEME, getter.scheme(request));
@@ -66,12 +74,14 @@ public void onStart(AttributesBuilder attributes, REQUEST request) {
   @Override
   public void onEnd(
       AttributesBuilder attributes,
+      Context context,
       REQUEST request,
       @Nullable RESPONSE response,
       @Nullable Throwable error) {
 
-    super.onEnd(attributes, request, response, error);
+    super.onEnd(attributes, context, request, response, error);
     set(attributes, SemanticAttributes.HTTP_SERVER_NAME, getter.serverName(request, response));
+    set(attributes, SemanticAttributes.HTTP_ROUTE, httpRouteHolderGetter.apply(context));
   }
 
   @Nullable

instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpServerMetrics.java

@@ -18,9 +18,7 @@
 import io.opentelemetry.instrumentation.api.annotations.UnstableApi;
 import io.opentelemetry.instrumentation.api.instrumenter.RequestListener;
 import io.opentelemetry.instrumentation.api.instrumenter.RequestMetrics;
-import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
 import java.util.concurrent.TimeUnit;
-import java.util.function.Function;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -51,14 +49,8 @@ public static RequestMetrics get() {
 
   private final LongUpDownCounter activeRequests;
   private final DoubleHistogram duration;
-  private final Function<Context, String> httpRouteHolderGetter;
 
   private HttpServerMetrics(Meter meter) {
-    this(meter, HttpRouteHolder::getRoute);
-  }
-
-  // visible for tests
-  HttpServerMetrics(Meter meter, Function<Context, String> httpRouteHolderGetter) {
     activeRequests =
         meter
             .upDownCounterBuilder("http.server.active_requests")
@@ -72,8 +64,6 @@ private HttpServerMetrics(Meter meter) {
             .setUnit("ms")
             .setDescription("The duration of the inbound HTTP request")
             .build();
-
-    this.httpRouteHolderGetter = httpRouteHolderGetter;
   }
 
   @Override
@@ -96,19 +86,10 @@ public void end(Context context, Attributes endAttributes, long endNanos) {
     activeRequests.add(-1, applyActiveRequestsView(state.startAttributes()));
     duration.record(
         (endNanos - state.startTimeNanos()) / NANOS_PER_MS,
-        applyServerDurationView(state.startAttributes(), addHttpRoute(context, endAttributes)),
+        applyServerDurationView(state.startAttributes(), endAttributes),
         context);
   }
 
-  // TODO: the http.route should be extracted by the AttributesExtractor, HttpServerMetrics should
-  // not access the context to get it
-  private Attributes addHttpRoute(Context context, Attributes endAttributes) {
-    String route = httpRouteHolderGetter.apply(context);
-    return route == null
-        ? endAttributes
-        : endAttributes.toBuilder().put(SemanticAttributes.HTTP_ROUTE, route).build();
-  }
-
   @AutoValue
   abstract static class State {
 

instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpClientAttributesExtractorTest.java

@@ -14,6 +14,7 @@
 import io.opentelemetry.api.common.AttributeKey;
 import io.opentelemetry.api.common.Attributes;
 import io.opentelemetry.api.common.AttributesBuilder;
+import io.opentelemetry.context.Context;
 import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
 import java.util.HashMap;
 import java.util.List;
@@ -109,7 +110,7 @@ void normal() {
                 singletonList("Custom-Request-Header"), singletonList("Custom-Response-Header")));
 
     AttributesBuilder attributes = Attributes.builder();
-    extractor.onStart(attributes, request);
+    extractor.onStart(attributes, Context.root(), request);
     assertThat(attributes.build())
         .containsOnly(
             entry(SemanticAttributes.HTTP_METHOD, "POST"),
@@ -119,7 +120,7 @@ void normal() {
                 AttributeKey.stringArrayKey("http.request.header.custom_request_header"),
                 asList("123", "456")));
 
-    extractor.onEnd(attributes, request, response, null);
+    extractor.onEnd(attributes, Context.root(), request, response, null);
     assertThat(attributes.build())
         .containsOnly(
             entry(SemanticAttributes.HTTP_METHOD, "POST"),
@@ -151,10 +152,10 @@ void invalidStatusCode() {
             new TestHttpClientAttributesGetter(), CapturedHttpHeaders.empty());
 
     AttributesBuilder attributes = Attributes.builder();
-    extractor.onStart(attributes, request);
+    extractor.onStart(attributes, Context.root(), request);
     assertThat(attributes.build()).isEmpty();
 
-    extractor.onEnd(attributes, request, response, null);
+    extractor.onEnd(attributes, Context.root(), request, response, null);
     assertThat(attributes.build()).isEmpty();
   }
 }