fix broken http.route bridge

open-telemetry · Apr 20, 2022 · 02733bc · 02733bc
1 parent 3124065
commit 02733bc
Show file tree

Hide file tree

Showing 10 changed files with 302 additions and 185 deletions.
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
@@ -7,9 +7,9 @@
 
 import io.opentelemetry.api.trace.Span;
 import io.opentelemetry.context.Context;
-import io.opentelemetry.context.ContextKey;
 import io.opentelemetry.instrumentation.api.instrumenter.ContextCustomizer;
 import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter;
+import io.opentelemetry.instrumentation.api.internal.HttpRouteState;
 import io.opentelemetry.instrumentation.api.server.ServerSpan;
 import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
 import javax.annotation.Nullable;
@@ -25,32 +25,21 @@
  */
 public final class HttpRouteHolder {
 
-  private static final ContextKey<HttpRouteHolder> KEY =
-      ContextKey.named("opentelemetry-http-server-route-key");
-
   /**
    * Returns a {@link ContextCustomizer} that initializes a {@link HttpRouteHolder} in the {@link
    * Context} returned from {@link Instrumenter#start(Context, Object)}.
    */
   public static <REQUEST> ContextCustomizer<REQUEST> get() {
     return (context, request, startAttributes) -> {
-      if (context.get(KEY) != null) {
+      if (HttpRouteState.fromContextOrNull(context) != null) {
         return context;
       }
-      return context.with(KEY, new HttpRouteHolder());
+      return context.with(HttpRouteState.create(0, null));
     };
   }
 
-  private volatile int updatedBySourceOrder = 0;
-  @Nullable private volatile String route;
-
   private HttpRouteHolder() {}
 
-  private HttpRouteHolder(int updatedBySourceOrder, @Nullable String route) {
-    this.updatedBySourceOrder = updatedBySourceOrder;
-    this.route = route;
-  }
-
   /**
    * Updates the {@code http.route} attribute in the received {@code context}.
    *
@@ -112,8 +101,8 @@ public static <T, U> void updateHttpRoute(
     if (serverSpan == null) {
       return;
     }
-    HttpRouteHolder httpRouteHolder = context.get(KEY);
-    if (httpRouteHolder == null) {
+    HttpRouteState httpRouteState = HttpRouteState.fromContextOrNull(context);
+    if (httpRouteState == null) {
       String httpRoute = httpRouteGetter.get(context, arg1, arg2);
       if (httpRoute != null && !httpRoute.isEmpty()) {
         // update both span name and attribute, since there's no HttpRouteHolder in the context
@@ -125,27 +114,26 @@ public static <T, U> void updateHttpRoute(
     // 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 == httpRouteHolder.updatedBySourceOrder;
-    if (source.order > httpRouteHolder.updatedBySourceOrder || 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 || httpRouteHolder.isBetterRoute(route))) {
+          && (!onlyIfBetterRoute || isBetterRoute(httpRouteState, 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;
+        httpRouteState.update(context, source.order, route);
       }
     }
   }
 
   // This is used when setting route from a servlet filter to pick the most descriptive (longest)
   // route.
-  private boolean isBetterRoute(String name) {
-    String route = this.route;
+  private static boolean isBetterRoute(HttpRouteState httpRouteState, String name) {
+    String route = httpRouteState.getRoute();
     int routeLength = route == null ? 0 : route.length();
     return name.length() > routeLength;
   }
@@ -156,24 +144,8 @@ private boolean isBetterRoute(String name) {
    */
   @Nullable
   static String getRoute(Context context) {
-    HttpRouteHolder httpRouteHolder = context.get(KEY);
-    return httpRouteHolder == null ? null : httpRouteHolder.route;
-  }
-
-  @SuppressWarnings("unused") // used by the opentelemetry-api bridge instrumentation
-  static HttpRouteHolder internalCreate(int updatedBySourceOrder, @Nullable String route) {
-    return new HttpRouteHolder(updatedBySourceOrder, route);
-  }
-
-  @SuppressWarnings("unused") // used by the opentelemetry-api bridge instrumentation
-  int internalGetUpdatedBySourceOrder() {
-    return updatedBySourceOrder;
-  }
-
-  @SuppressWarnings("unused") // used by the opentelemetry-api bridge instrumentation
-  @Nullable
-  String internalGetRoute() {
-    return route;
+    HttpRouteState httpRouteState = HttpRouteState.fromContextOrNull(context);
+    return httpRouteState == null ? null : httpRouteState.getRoute();
   }
 
   private static final class OneArgAdapter<T> implements HttpRouteBiGetter<T, HttpRouteGetter<T>> {

diff --git a/...i-semconv/src/main/java/io/opentelemetry/instrumentation/api/internal/HttpRouteState.java b/...i-semconv/src/main/java/io/opentelemetry/instrumentation/api/internal/HttpRouteState.java
@@ -0,0 +1,61 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.instrumentation.api.internal;
+
+import io.opentelemetry.context.Context;
+import io.opentelemetry.context.ContextKey;
+import io.opentelemetry.context.ImplicitContextKeyed;
+import javax.annotation.Nullable;
+
+/**
+ * This class is internal and is hence not for public use. Its APIs are unstable and can change at
+ * any time.
+ */
+public final class HttpRouteState implements ImplicitContextKeyed {
+
+  private static final ContextKey<HttpRouteState> KEY =
+      ContextKey.named("opentelemetry-http-server-route-key");
+
+  @Nullable
+  public static HttpRouteState fromContextOrNull(Context context) {
+    return context.get(KEY);
+  }
+
+  public static HttpRouteState create(int updatedBySourceOrder, @Nullable String route) {
+    return new HttpRouteState(updatedBySourceOrder, route);
+  }
+
+  private volatile int updatedBySourceOrder;
+  @Nullable private volatile String route;
+
+  private HttpRouteState(int updatedBySourceOrder, @Nullable String route) {
+    this.updatedBySourceOrder = updatedBySourceOrder;
+    this.route = route;
+  }
+
+  @Override
+  public Context storeInContext(Context context) {
+    return context.with(KEY, this);
+  }
+
+  public int getUpdatedBySourceOrder() {
+    return updatedBySourceOrder;
+  }
+
+  @Nullable
+  public String getRoute() {
+    return route;
+  }
+
+  public void update(
+      @SuppressWarnings("unused")
+          Context context, // context is used by the javaagent bridge instrumentation
+      int updatedBySourceOrder,
+      String route) {
+    this.updatedBySourceOrder = updatedBySourceOrder;
+    this.route = route;
+  }
+}
diff --git a/...try/javaagent/instrumentation/opentelemetryapi/OpenTelemetryApiInstrumentationModule.java b/...try/javaagent/instrumentation/opentelemetryapi/OpenTelemetryApiInstrumentationModule.java
@@ -10,6 +10,7 @@
 import com.google.auto.service.AutoService;
 import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule;
 import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
+import io.opentelemetry.javaagent.instrumentation.opentelemetryapi.instrumentationapi.HttpRouteStateInstrumentation;
 import java.util.List;
 
 @AutoService(InstrumentationModule.class)
@@ -24,6 +25,8 @@ public List<TypeInstrumentation> typeInstrumentations() {
         new ContextInstrumentation(),
         new ContextStorageWrappersInstrumentation(),
         new OpenTelemetryInstrumentation(),
-        new SpanInstrumentation());
+        new SpanInstrumentation(),
+        // instrumentation-api specific instrumentation
+        new HttpRouteStateInstrumentation());
   }
 }
diff --git a/...opentelemetry/javaagent/instrumentation/opentelemetryapi/context/AgentContextStorage.java b/...opentelemetry/javaagent/instrumentation/opentelemetryapi/context/AgentContextStorage.java
@@ -10,17 +10,11 @@
 import application.io.opentelemetry.api.baggage.Baggage;
 import application.io.opentelemetry.api.trace.Span;
 import application.io.opentelemetry.context.Context;
-import application.io.opentelemetry.context.ContextKey;
 import application.io.opentelemetry.context.ContextStorage;
 import application.io.opentelemetry.context.Scope;
-import io.opentelemetry.javaagent.instrumentation.opentelemetryapi.baggage.BaggageBridging;
-import io.opentelemetry.javaagent.instrumentation.opentelemetryapi.trace.Bridging;
 import java.lang.invoke.MethodHandle;
 import java.lang.invoke.MethodHandles;
 import java.lang.invoke.MethodType;
-import java.util.ArrayList;
-import java.util.Collections;
-import java.util.List;
 import java.util.function.Function;
 import java.util.logging.Logger;
 
@@ -145,34 +139,6 @@ public static Context newContextWrapper(
   static final io.opentelemetry.context.ContextKey<Context> APPLICATION_CONTEXT =
       io.opentelemetry.context.ContextKey.named("otel-context");
 
-  static final List<ContextKeyBridge<?, ?>> CONTEXT_KEY_BRIDGES;
-
-  static {
-    List<ContextKeyBridge<?, ?>> bridges = new ArrayList<>();
-    try {
-      bridges.add(
-          new ContextKeyBridge<Span, io.opentelemetry.api.trace.Span>(
-              "application.io.opentelemetry.api.trace.SpanContextKey",
-              "io.opentelemetry.api.trace.SpanContextKey",
-              Bridging::toApplication,
-              Bridging::toAgentOrNull));
-    } catch (Throwable ignored) {
-      // reflection error; in practice should never happen, we can ignore it
-    }
-    try {
-      bridges.add(
-          new ContextKeyBridge<>(
-              "application.io.opentelemetry.api.baggage.BaggageContextKey",
-              "io.opentelemetry.api.baggage.BaggageContextKey",
-              BaggageBridging::toApplication,
-              BaggageBridging::toAgent));
-    } catch (Throwable ignored) {
-      // reflection error; in practice should never happen, we can ignore it
-    }
-    bridges.addAll(InstrumentationApiContextBridging.instrumentationApiBridges());
-    CONTEXT_KEY_BRIDGES = Collections.unmodifiableList(bridges);
-  }
-
   @Override
   public Scope attach(Context toAttach) {
     io.opentelemetry.context.Context currentAgentContext =
@@ -224,60 +190,4 @@ public void close() throws Exception {
       ((AutoCloseable) agentStorage).close();
     }
   }
-
-  static class AgentContextWrapper implements Context {
-    final io.opentelemetry.context.Context agentContext;
-    final Context applicationContext;
-
-    AgentContextWrapper(io.opentelemetry.context.Context agentContext) {
-      this(agentContext, agentContext.get(APPLICATION_CONTEXT));
-    }
-
-    AgentContextWrapper(io.opentelemetry.context.Context agentContext, Context applicationContext) {
-      if (applicationContext instanceof AgentContextWrapper) {
-        throw new IllegalStateException("Expected unwrapped context");
-      }
-      this.agentContext = agentContext;
-      this.applicationContext = applicationContext;
-    }
-
-    io.opentelemetry.context.Context toAgentContext() {
-      if (agentContext.get(APPLICATION_CONTEXT) == applicationContext) {
-        return agentContext;
-      }
-      return agentContext.with(APPLICATION_CONTEXT, applicationContext);
-    }
-
-    @Override
-    public <V> V get(ContextKey<V> key) {
-      for (ContextKeyBridge<?, ?> bridge : CONTEXT_KEY_BRIDGES) {
-        V value = bridge.get(this, key);
-        if (value != null) {
-          return value;
-        }
-      }
-
-      return applicationContext.get(key);
-    }
-
-    @Override
-    public <V> Context with(ContextKey<V> k1, V v1) {
-      for (ContextKeyBridge<?, ?> bridge : CONTEXT_KEY_BRIDGES) {
-        Context context = bridge.with(this, k1, v1);
-        if (context != null) {
-          return context;
-        }
-      }
-      return new AgentContextWrapper(agentContext, applicationContext.with(k1, v1));
-    }
-
-    @Override
-    public String toString() {
-      return "AgentContextWrapper{agentContext="
-          + agentContext
-          + ", applicationContext="
-          + applicationContext
-          + "}";
-    }
-  }
 }