Refactor and rename HttpClientResend and HttpRouteHolder (#9280)

Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
open-telemetry · Aug 26, 2023 · d7a34f9 · d7a34f9
1 parent 2ebf413
commit d7a34f9
Show file tree

Hide file tree

Showing 94 changed files with 649 additions and 455 deletions.
diff --git a/...telemetry/instrumentation/api/instrumenter/http/HttpClientAttributesExtractorBuilder.java b/...telemetry/instrumentation/api/instrumenter/http/HttpClientAttributesExtractorBuilder.java
@@ -35,7 +35,7 @@ public final class HttpClientAttributesExtractorBuilder<REQUEST, RESPONSE> {
   List<String> capturedRequestHeaders = emptyList();
   List<String> capturedResponseHeaders = emptyList();
   Set<String> knownMethods = HttpConstants.KNOWN_METHODS;
-  ToIntFunction<Context> resendCountIncrementer = HttpClientResend::getAndIncrement;
+  ToIntFunction<Context> resendCountIncrementer = HttpClientResendCount::getAndIncrement;
 
   HttpClientAttributesExtractorBuilder(
       HttpClientAttributesGetter<REQUEST, RESPONSE> httpAttributesGetter,

diff --git a/...i/instrumenter/http/HttpClientResend.java → ...trumenter/http/HttpClientResendCount.java b/...i/instrumenter/http/HttpClientResend.java → ...trumenter/http/HttpClientResendCount.java
@@ -11,12 +11,12 @@
 import java.util.concurrent.atomic.AtomicIntegerFieldUpdater;
 
 /** A helper that keeps track of the count of the HTTP request resend attempts. */
-public final class HttpClientResend {
+public final class HttpClientResendCount {
 
-  private static final ContextKey<HttpClientResend> KEY =
+  private static final ContextKey<HttpClientResendCount> KEY =
       ContextKey.named("opentelemetry-http-client-resend-key");
-  private static final AtomicIntegerFieldUpdater<HttpClientResend> resendsUpdater =
-      AtomicIntegerFieldUpdater.newUpdater(HttpClientResend.class, "resends");
+  private static final AtomicIntegerFieldUpdater<HttpClientResendCount> resendsUpdater =
+      AtomicIntegerFieldUpdater.newUpdater(HttpClientResendCount.class, "resends");
 
   /**
    * Initializes the HTTP request resend counter.
@@ -29,16 +29,20 @@ public static Context initialize(Context context) {
     if (context.get(KEY) != null) {
       return context;
     }
-    return context.with(KEY, new HttpClientResend());
+    return context.with(KEY, new HttpClientResendCount());
   }
 
+  /**
+   * Returns the count of the already made attempts to send an HTTP request; 0 if this is the first
+   * send attempt.
+   */
   public static int get(Context context) {
-    HttpClientResend resend = context.get(KEY);
+    HttpClientResendCount resend = context.get(KEY);
     return resend == null ? 0 : resend.resends;
   }
 
   static int getAndIncrement(Context context) {
-    HttpClientResend resend = context.get(KEY);
+    HttpClientResendCount resend = context.get(KEY);
     if (resend == null) {
       return 0;
     }
@@ -48,5 +52,5 @@ static int getAndIncrement(Context context) {
   @SuppressWarnings("unused") // it actually is used by the resendsUpdater
   private volatile int resends = 0;
 
-  private HttpClientResend() {}
+  private HttpClientResendCount() {}
 }
diff --git a/...c/main/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpRouteBiGetter.java b/...c/main/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpRouteBiGetter.java
@@ -5,17 +5,12 @@
 
 package io.opentelemetry.instrumentation.api.instrumenter.http;
 
-import io.opentelemetry.context.Context;
-import javax.annotation.Nullable;
-
-/** An interface for getting the {@code http.route} attribute. */
+/**
+ * An interface for getting the {@code http.route} attribute.
+ *
+ * @deprecated This class is deprecated and will be removed in the 2.0 release. Use {@link
+ *     HttpServerRouteBiGetter} instead.
+ */
+@Deprecated
 @FunctionalInterface
-public interface HttpRouteBiGetter<T, U> {
-
-  /**
-   * Returns the {@code http.route} attribute extracted from {@code context}, {@code arg1} and
-   * {@code arg2}; or {@code null} if it was not found.
-   */
-  @Nullable
-  String get(Context context, T arg1, U arg2);
-}
+public interface HttpRouteBiGetter<T, U> extends HttpServerRouteBiGetter<T, U> {}
diff --git a/...src/main/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpRouteGetter.java b/...src/main/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpRouteGetter.java
@@ -5,17 +5,12 @@
 
 package io.opentelemetry.instrumentation.api.instrumenter.http;
 
-import io.opentelemetry.context.Context;
-import javax.annotation.Nullable;
-
-/** An interface for getting the {@code http.route} attribute. */
+/**
+ * An interface for getting the {@code http.route} attribute.
+ *
+ * @deprecated This class is deprecated and will be removed in the 2.0 release. Use {@link
+ *     HttpServerRouteGetter} instead.
+ */
+@Deprecated
 @FunctionalInterface
-public interface HttpRouteGetter<T> {
-
-  /**
-   * Returns the {@code http.route} attribute extracted from {@code context} and {@code arg}; or
-   * {@code null} if it was not found.
-   */
-  @Nullable
-  String get(Context context, T arg);
-}
+public interface HttpRouteGetter<T> extends HttpServerRouteGetter<T> {}
diff --git a/...src/main/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpRouteHolder.java b/...src/main/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpRouteHolder.java
@@ -5,13 +5,9 @@
 
 package io.opentelemetry.instrumentation.api.instrumenter.http;
 
-import io.opentelemetry.api.trace.Span;
 import io.opentelemetry.context.Context;
 import io.opentelemetry.instrumentation.api.instrumenter.ContextCustomizer;
 import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter;
-import io.opentelemetry.instrumentation.api.instrumenter.LocalRootSpan;
-import io.opentelemetry.instrumentation.api.internal.HttpRouteState;
-import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
 import javax.annotation.Nullable;
 
 /**
@@ -22,7 +18,11 @@
  * later, after the instrumented operation starts. This class provides several static methods that
  * allow the instrumentation author to provide the matching HTTP route to the instrumentation when
  * it is discovered.
+ *
+ * @deprecated This class is deprecated and will be removed in the 2.0 release. Use {@link
+ *     HttpServerRoute} instead.
  */
+@Deprecated
 public final class HttpRouteHolder {
 
   /**
@@ -31,13 +31,7 @@ public final class HttpRouteHolder {
    */
   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 HttpServerRoute.create(getter);
   }
 
   private HttpRouteHolder() {}
@@ -57,7 +51,7 @@ private HttpRouteHolder() {}
    */
   public static void updateHttpRoute(
       Context context, HttpRouteSource source, @Nullable String httpRoute) {
-    updateHttpRoute(context, source, ConstantAdapter.INSTANCE, httpRoute);
+    HttpServerRoute.update(context, source.toHttpServerRouteSource(), httpRoute);
   }
 
   /**
@@ -75,7 +69,7 @@ public static void updateHttpRoute(
    */
   public static <T> void updateHttpRoute(
       Context context, HttpRouteSource source, HttpRouteGetter<T> httpRouteGetter, T arg1) {
-    updateHttpRoute(context, source, OneArgAdapter.getInstance(), arg1, httpRouteGetter);
+    HttpServerRoute.update(context, source.toHttpServerRouteSource(), httpRouteGetter, arg1);
   }
 
   /**
@@ -97,92 +91,6 @@ public static <T, U> void updateHttpRoute(
       HttpRouteBiGetter<T, U> httpRouteGetter,
       T arg1,
       U arg2) {
-    Span serverSpan = LocalRootSpan.fromContextOrNull(context);
-    // even if the server span is not sampled, we have to continue - we need to compute the
-    // http.route properly so that it can be captured by the server metrics
-    if (serverSpan == null) {
-      return;
-    }
-    HttpRouteState httpRouteState = HttpRouteState.fromContextOrNull(context);
-    if (httpRouteState == null) {
-      // TODO: remove this branch?
-      String httpRoute = httpRouteGetter.get(context, arg1, arg2);
-      if (httpRoute != null && !httpRoute.isEmpty()) {
-        // update just the attribute - without http.method we can't compute a proper span name here
-        serverSpan.setAttribute(SemanticAttributes.HTTP_ROUTE, httpRoute);
-      }
-      return;
-    }
-    // special case for servlet filters, even when we have a route from previous filter see whether
-    // the new route is better and if so use it instead
-    boolean onlyIfBetterRoute =
-        !source.useFirst && source.order == httpRouteState.getUpdatedBySourceOrder();
-    if (source.order > httpRouteState.getUpdatedBySourceOrder() || onlyIfBetterRoute) {
-      String route = httpRouteGetter.get(context, arg1, arg2);
-      if (route != null
-          && !route.isEmpty()
-          && (!onlyIfBetterRoute || isBetterRoute(httpRouteState, route))) {
-
-        // update just the span name - the attribute will be picked up by the
-        // HttpServerAttributesExtractor at the end of request processing
-        updateSpanName(serverSpan, httpRouteState, route);
-
-        httpRouteState.update(context, source.order, route);
-      }
-    }
-  }
-
-  // This is used when setting route from a servlet filter to pick the most descriptive (longest)
-  // route.
-  private static boolean isBetterRoute(HttpRouteState httpRouteState, String name) {
-    String route = httpRouteState.getRoute();
-    int routeLength = route == null ? 0 : route.length();
-    return name.length() > routeLength;
-  }
-
-  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);
-    }
-  }
-
-  /**
-   * Returns the {@code http.route} attribute value that's stored in the {@code context}, or null if
-   * it was not set before.
-   */
-  @Nullable
-  static String getRoute(Context context) {
-    HttpRouteState httpRouteState = HttpRouteState.fromContextOrNull(context);
-    return httpRouteState == null ? null : httpRouteState.getRoute();
-  }
-
-  private static final class OneArgAdapter<T> implements HttpRouteBiGetter<T, HttpRouteGetter<T>> {
-
-    private static final OneArgAdapter<Object> INSTANCE = new OneArgAdapter<>();
-
-    @SuppressWarnings("unchecked")
-    static <T> OneArgAdapter<T> getInstance() {
-      return (OneArgAdapter<T>) INSTANCE;
-    }
-
-    @Override
-    @Nullable
-    public String get(Context context, T arg, HttpRouteGetter<T> httpRouteGetter) {
-      return httpRouteGetter.get(context, arg);
-    }
-  }
-
-  private static final class ConstantAdapter implements HttpRouteGetter<String> {
-
-    private static final ConstantAdapter INSTANCE = new ConstantAdapter();
-
-    @Nullable
-    @Override
-    public String get(Context context, String route) {
-      return route;
-    }
+    HttpServerRoute.update(context, source.toHttpServerRouteSource(), httpRouteGetter, arg1, arg2);
   }
 }
diff --git a/...src/main/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpRouteSource.java b/...src/main/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpRouteSource.java
@@ -5,7 +5,13 @@
 
 package io.opentelemetry.instrumentation.api.instrumenter.http;
 
-/** Represents the source that provided the {@code http.route} attribute. */
+/**
+ * Represents the source that provided the {@code http.route} attribute.
+ *
+ * @deprecated This class is deprecated and will be removed in the 2.0 release. Use {@link
+ *     HttpServerRouteSource} instead.
+ */
+@Deprecated
 public enum HttpRouteSource {
   // for servlet filters we try to find the best name which isn't necessarily from the first
   // filter that is called
@@ -27,4 +33,18 @@ public enum HttpRouteSource {
     this.order = order;
     this.useFirst = useFirst;
   }
+
+  HttpServerRouteSource toHttpServerRouteSource() {
+    switch (this) {
+      case FILTER:
+        return HttpServerRouteSource.SERVER_FILTER;
+      case SERVLET:
+        return HttpServerRouteSource.SERVER;
+      case CONTROLLER:
+        return HttpServerRouteSource.CONTROLLER;
+      case NESTED_CONTROLLER:
+        return HttpServerRouteSource.NESTED_CONTROLLER;
+    }
+    throw new IllegalStateException("Unsupported value " + this);
+  }
 }
diff --git a/...telemetry/instrumentation/api/instrumenter/http/HttpServerAttributesExtractorBuilder.java b/...telemetry/instrumentation/api/instrumenter/http/HttpServerAttributesExtractorBuilder.java
@@ -37,7 +37,7 @@ public final class HttpServerAttributesExtractorBuilder<REQUEST, RESPONSE> {
   List<String> capturedRequestHeaders = emptyList();
   List<String> capturedResponseHeaders = emptyList();
   Set<String> knownMethods = HttpConstants.KNOWN_METHODS;
-  Function<Context, String> httpRouteGetter = HttpRouteHolder::getRoute;
+  Function<Context, String> httpRouteGetter = HttpServerRoute::get;
 
   HttpServerAttributesExtractorBuilder(
       HttpServerAttributesGetter<REQUEST, RESPONSE> httpAttributesGetter,