Move attributes to span builder for use by samplers (#2587)

* Move attributes to span builder for sampler use * Fix test * Remove unnecessary test
open-telemetry · Mar 17, 2021 · 0ce9ca5 · 0ce9ca5
1 parent ca1e1f6
commit 0ce9ca5
Show file tree

Hide file tree

Showing 8 changed files with 77 additions and 77 deletions.
diff --git a/...ation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpClientTracer.java b/...ation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpClientTracer.java
@@ -135,24 +135,36 @@ private Span internalStartSpan(
     if (startTimeNanos > 0) {
       spanBuilder.setStartTimestamp(startTimeNanos, TimeUnit.NANOSECONDS);
     }
+    onRequest(spanBuilder, request);
     Span span = spanBuilder.startSpan();
-    onRequest(span, request);
     return span;
   }
 
+  protected void onRequest(SpanBuilder spanBuilder, REQUEST request) {
+    onRequest(spanBuilder::setAttribute, request);
+  }
+
+  /**
+   * This method should only be used when the request is not yet available when {@link #startSpan}
+   * is called. Otherwise {@link #onRequest(SpanBuilder, Object)} should be used.
+   */
   protected void onRequest(Span span, REQUEST request) {
-    assert span != null;
+    onRequest(span::setAttribute, request);
+  }
+
+  private void onRequest(AttributeSetter setter, REQUEST request) {
+    assert setter != null;
     if (request != null) {
-      span.setAttribute(SemanticAttributes.NET_TRANSPORT, "IP.TCP");
-      span.setAttribute(SemanticAttributes.HTTP_METHOD, method(request));
-      span.setAttribute(SemanticAttributes.HTTP_USER_AGENT, requestHeader(request, USER_AGENT));
+      setter.setAttribute(SemanticAttributes.NET_TRANSPORT, "IP.TCP");
+      setter.setAttribute(SemanticAttributes.HTTP_METHOD, method(request));
+      setter.setAttribute(SemanticAttributes.HTTP_USER_AGENT, requestHeader(request, USER_AGENT));
 
-      setFlavor(span, request);
-      setUrl(span, request);
+      setFlavor(setter, request);
+      setUrl(setter, request);
     }
   }
 
-  private void setFlavor(Span span, REQUEST request) {
+  private void setFlavor(AttributeSetter setter, REQUEST request) {
     String flavor = flavor(request);
     if (flavor == null) {
       return;
@@ -163,15 +175,15 @@ private void setFlavor(Span span, REQUEST request) {
       flavor = flavor.substring(httpProtocolPrefix.length());
     }
 
-    span.setAttribute(SemanticAttributes.HTTP_FLAVOR, flavor);
+    setter.setAttribute(SemanticAttributes.HTTP_FLAVOR, flavor);
   }
 
-  private void setUrl(Span span, REQUEST request) {
+  private void setUrl(AttributeSetter setter, REQUEST request) {
     try {
       URI url = url(request);
       if (url != null) {
-        netPeerAttributes.setNetPeer(span, url.getHost(), null, url.getPort());
-        span.setAttribute(SemanticAttributes.HTTP_URL, url.toString());
+        netPeerAttributes.setNetPeer(setter, url.getHost(), null, url.getPort());
+        setter.setAttribute(SemanticAttributes.HTTP_URL, url.toString());
       }
     } catch (Exception e) {
       log.debug("Error tagging url", e);

diff --git a/...ation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpServerTracer.java b/...ation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpServerTracer.java
@@ -70,18 +70,17 @@ public Context startSpan(
     // whether to call end() or not on the Span in the returned Context
 
     Context parentContext = extract(request, getGetter());
-    SpanBuilder builder = spanBuilder(parentContext, spanName, SERVER);
+    SpanBuilder spanBuilder = spanBuilder(parentContext, spanName, SERVER);
 
     if (startTimestamp >= 0) {
-      builder.setStartTimestamp(startTimestamp, TimeUnit.NANOSECONDS);
+      spanBuilder.setStartTimestamp(startTimestamp, TimeUnit.NANOSECONDS);
     }
 
-    Span span = builder.startSpan();
-    onConnection(span, connection);
-    onRequest(span, request);
-    onConnectionAndRequest(span, connection, request);
+    onConnection(spanBuilder, connection);
+    onRequest(spanBuilder, request);
+    onConnectionAndRequest(spanBuilder, connection, request);
 
-    Context context = withServerSpan(parentContext, span);
+    Context context = withServerSpan(parentContext, spanBuilder.startSpan());
     context = customizeContext(context, request);
     attachServerContext(context, storage);
 
@@ -113,6 +112,7 @@ public void end(Context context, RESPONSE response, long timestamp) {
    * Convenience method. Delegates to {@link #endExceptionally(Context, Throwable, Object)}, passing
    * {@code response} value of {@code null}.
    */
+  @Override
   public void endExceptionally(Context context, Throwable throwable) {
     endExceptionally(context, throwable, null);
   }
@@ -153,21 +153,22 @@ public Span getServerSpan(STORAGE storage) {
   @Nullable
   public abstract Context getServerContext(STORAGE storage);
 
-  protected void onConnection(Span span, CONNECTION connection) {
+  protected void onConnection(SpanBuilder spanBuilder, CONNECTION connection) {
     // TODO: use NetPeerAttributes here
-    span.setAttribute(SemanticAttributes.NET_PEER_IP, peerHostIP(connection));
+    spanBuilder.setAttribute(SemanticAttributes.NET_PEER_IP, peerHostIP(connection));
     Integer port = peerPort(connection);
     // Negative or Zero ports might represent an unset/null value for an int type.  Skip setting.
     if (port != null && port > 0) {
-      span.setAttribute(SemanticAttributes.NET_PEER_PORT, (long) port);
+      spanBuilder.setAttribute(SemanticAttributes.NET_PEER_PORT, (long) port);
     }
   }
 
-  protected void onRequest(Span span, REQUEST request) {
-    span.setAttribute(SemanticAttributes.HTTP_METHOD, method(request));
-    span.setAttribute(SemanticAttributes.HTTP_USER_AGENT, requestHeader(request, USER_AGENT));
+  protected void onRequest(SpanBuilder spanBuilder, REQUEST request) {
+    spanBuilder.setAttribute(SemanticAttributes.HTTP_METHOD, method(request));
+    spanBuilder.setAttribute(
+        SemanticAttributes.HTTP_USER_AGENT, requestHeader(request, USER_AGENT));
 
-    setUrl(span, request);
+    setUrl(spanBuilder, request);
 
     // TODO set resource name from URL.
   }
@@ -183,20 +184,21 @@ protected void onRequest(Span span, REQUEST request) {
   which is the recommended value for http.target attribute. Therefore we cannot use any of the
   recommended combinations of attributes and are forced to use http.url.
    */
-  private void setUrl(Span span, REQUEST request) {
-    span.setAttribute(SemanticAttributes.HTTP_URL, url(request));
+  private void setUrl(SpanBuilder spanBuilder, REQUEST request) {
+    spanBuilder.setAttribute(SemanticAttributes.HTTP_URL, url(request));
   }
 
-  protected void onConnectionAndRequest(Span span, CONNECTION connection, REQUEST request) {
+  protected void onConnectionAndRequest(
+      SpanBuilder spanBuilder, CONNECTION connection, REQUEST request) {
     String flavor = flavor(connection, request);
     if (flavor != null) {
       // remove HTTP/ prefix to comply with semantic conventions
       if (flavor.startsWith("HTTP/")) {
         flavor = flavor.substring("HTTP/".length());
       }
-      span.setAttribute(SemanticAttributes.HTTP_FLAVOR, flavor);
+      spanBuilder.setAttribute(SemanticAttributes.HTTP_FLAVOR, flavor);
     }
-    span.setAttribute(SemanticAttributes.HTTP_CLIENT_IP, clientIP(connection, request));
+    spanBuilder.setAttribute(SemanticAttributes.HTTP_CLIENT_IP, clientIP(connection, request));
   }
 
   private String clientIP(CONNECTION connection, REQUEST request) {

diff --git a/...i/src/test/groovy/io/opentelemetry/instrumentation/api/tracer/HttpClientTracerTest.groovy b/...i/src/test/groovy/io/opentelemetry/instrumentation/api/tracer/HttpClientTracerTest.groovy
@@ -5,7 +5,7 @@
 
 package io.opentelemetry.instrumentation.api.tracer
 
-import io.opentelemetry.api.trace.Span
+
 import io.opentelemetry.context.propagation.TextMapSetter
 import io.opentelemetry.instrumentation.api.tracer.net.NetPeerAttributes
 import io.opentelemetry.semconv.trace.attributes.SemanticAttributes
@@ -132,23 +132,6 @@ class HttpClientTracerTest extends BaseTracerTest {
     null   | null
   }
 
-  def "test assert null span"() {
-    setup:
-    def tracer = newTracer()
-
-    when:
-    tracer.onRequest((Span) null, null)
-
-    then:
-    thrown(AssertionError)
-
-    when:
-    tracer.onResponse((Span) null, null)
-
-    then:
-    thrown(AssertionError)
-  }
-
   @Override
   def newTracer() {
     def netPeerAttributes = new NetPeerAttributes([

diff --git a/...t-2.2/src/main/java/io/opentelemetry/instrumentation/servlet/ServletHttpServerTracer.java b/...t-2.2/src/main/java/io/opentelemetry/instrumentation/servlet/ServletHttpServerTracer.java
@@ -6,6 +6,7 @@
 package io.opentelemetry.instrumentation.servlet;
 
 import io.opentelemetry.api.trace.Span;
+import io.opentelemetry.api.trace.SpanContext;
 import io.opentelemetry.context.Context;
 import io.opentelemetry.context.propagation.TextMapGetter;
 import io.opentelemetry.instrumentation.api.servlet.AppServerBridge;
@@ -27,7 +28,14 @@ public abstract class ServletHttpServerTracer<RESPONSE>
   private static final Logger log = LoggerFactory.getLogger(ServletHttpServerTracer.class);
 
   public Context startSpan(HttpServletRequest request, String spanName) {
-    return startSpan(request, request, request, spanName);
+    Context context = startSpan(request, request, request, spanName);
+
+    SpanContext spanContext = Span.fromContext(context).getSpanContext();
+    // we do this e.g. so that servlet containers can use these values in their access logs
+    request.setAttribute("traceId", spanContext.getTraceId());
+    request.setAttribute("spanId", spanContext.getSpanId());
+
+    return context;
   }
 
   @Override
@@ -108,15 +116,6 @@ protected String method(HttpServletRequest request) {
     return request.getMethod();
   }
 
-  @Override
-  public void onRequest(Span span, HttpServletRequest request) {
-    // we do this e.g. so that servlet containers can use these values in their access logs
-    request.setAttribute("traceId", span.getSpanContext().getTraceId());
-    request.setAttribute("spanId", span.getSpanContext().getSpanId());
-
-    super.onRequest(span, request);
-  }
-
   @Override
   protected TextMapGetter<HttpServletRequest> getGetter() {
     return HttpServletRequestGetter.GETTER;

diff --git a/...a/io/opentelemetry/javaagent/instrumentation/kubernetesclient/KubernetesClientTracer.java b/...a/io/opentelemetry/javaagent/instrumentation/kubernetesclient/KubernetesClientTracer.java
@@ -8,6 +8,7 @@
 import io.kubernetes.client.openapi.ApiException;
 import io.kubernetes.client.openapi.ApiResponse;
 import io.opentelemetry.api.trace.Span;
+import io.opentelemetry.api.trace.SpanBuilder;
 import io.opentelemetry.context.Context;
 import io.opentelemetry.context.propagation.TextMapSetter;
 import io.opentelemetry.instrumentation.api.config.Config;
@@ -89,11 +90,12 @@ public void onException(Context context, Throwable throwable) {
   }
 
   @Override
-  protected void onRequest(Span span, Request request) {
-    super.onRequest(span, request);
+  protected void onRequest(SpanBuilder spanBuilder, Request request) {
+    super.onRequest(spanBuilder, request);
     if (CAPTURE_EXPERIMENTAL_SPAN_ATTRIBUTES) {
       KubernetesRequestDigest digest = KubernetesRequestDigest.parse(request);
-      span.setAttribute("kubernetes-client.namespace", digest.getResourceMeta().getNamespace())
+      spanBuilder
+          .setAttribute("kubernetes-client.namespace", digest.getResourceMeta().getNamespace())
           .setAttribute("kubernetes-client.name", digest.getResourceMeta().getName());
     }
   }

diff --git a/...a/io/opentelemetry/javaagent/instrumentation/netty/v3_8/client/NettyHttpClientTracer.java b/...a/io/opentelemetry/javaagent/instrumentation/netty/v3_8/client/NettyHttpClientTracer.java
@@ -9,7 +9,7 @@
 import static io.opentelemetry.javaagent.instrumentation.netty.v3_8.client.NettyResponseInjectAdapter.SETTER;
 import static org.jboss.netty.handler.codec.http.HttpHeaders.Names.HOST;
 
-import io.opentelemetry.api.trace.Span;
+import io.opentelemetry.api.trace.SpanBuilder;
 import io.opentelemetry.context.Context;
 import io.opentelemetry.context.propagation.TextMapSetter;
 import io.opentelemetry.instrumentation.api.tracer.HttpClientTracer;
@@ -36,12 +36,12 @@ public static NettyHttpClientTracer tracer() {
   }
 
   public Context startSpan(Context parentContext, ChannelHandlerContext ctx, HttpRequest request) {
-    Span span = spanBuilder(parentContext, spanNameForRequest(request), CLIENT).startSpan();
-    onRequest(span, request);
+    SpanBuilder spanBuilder = spanBuilder(parentContext, spanNameForRequest(request), CLIENT);
+    onRequest(spanBuilder, request);
     NetPeerAttributes.INSTANCE.setNetPeer(
-        span, (InetSocketAddress) ctx.getChannel().getRemoteAddress());
+        spanBuilder, (InetSocketAddress) ctx.getChannel().getRemoteAddress());
 
-    Context context = withClientSpan(parentContext, span);
+    Context context = withClientSpan(parentContext, spanBuilder.startSpan());
     inject(context, request.headers(), SETTER);
     return context;
   }

diff --git a/...a/io/opentelemetry/javaagent/instrumentation/netty/v4_0/client/NettyHttpClientTracer.java b/...a/io/opentelemetry/javaagent/instrumentation/netty/v4_0/client/NettyHttpClientTracer.java
@@ -13,7 +13,7 @@
 import io.netty.handler.codec.http.HttpHeaders;
 import io.netty.handler.codec.http.HttpRequest;
 import io.netty.handler.codec.http.HttpResponse;
-import io.opentelemetry.api.trace.Span;
+import io.opentelemetry.api.trace.SpanBuilder;
 import io.opentelemetry.context.Context;
 import io.opentelemetry.context.propagation.TextMapSetter;
 import io.opentelemetry.instrumentation.api.tracer.HttpClientTracer;
@@ -36,11 +36,12 @@ public static NettyHttpClientTracer tracer() {
   }
 
   public Context startSpan(Context parentContext, ChannelHandlerContext ctx, HttpRequest request) {
-    Span span = spanBuilder(parentContext, spanNameForRequest(request), CLIENT).startSpan();
-    onRequest(span, request);
-    NetPeerAttributes.INSTANCE.setNetPeer(span, (InetSocketAddress) ctx.channel().remoteAddress());
+    SpanBuilder spanBuilder = spanBuilder(parentContext, spanNameForRequest(request), CLIENT);
+    onRequest(spanBuilder, request);
+    NetPeerAttributes.INSTANCE.setNetPeer(
+        spanBuilder, (InetSocketAddress) ctx.channel().remoteAddress());
 
-    Context context = withClientSpan(parentContext, span);
+    Context context = withClientSpan(parentContext, spanBuilder.startSpan());
     inject(context, request.headers(), SETTER);
     return context;
   }

diff --git a/...a/io/opentelemetry/javaagent/instrumentation/netty/v4_1/client/NettyHttpClientTracer.java b/...a/io/opentelemetry/javaagent/instrumentation/netty/v4_1/client/NettyHttpClientTracer.java
@@ -13,7 +13,7 @@
 import io.netty.handler.codec.http.HttpHeaders;
 import io.netty.handler.codec.http.HttpRequest;
 import io.netty.handler.codec.http.HttpResponse;
-import io.opentelemetry.api.trace.Span;
+import io.opentelemetry.api.trace.SpanBuilder;
 import io.opentelemetry.context.Context;
 import io.opentelemetry.context.propagation.TextMapSetter;
 import io.opentelemetry.instrumentation.api.tracer.HttpClientTracer;
@@ -36,11 +36,12 @@ public static NettyHttpClientTracer tracer() {
   }
 
   public Context startSpan(Context parentContext, ChannelHandlerContext ctx, HttpRequest request) {
-    Span span = spanBuilder(parentContext, spanNameForRequest(request), CLIENT).startSpan();
-    onRequest(span, request);
-    NetPeerAttributes.INSTANCE.setNetPeer(span, (InetSocketAddress) ctx.channel().remoteAddress());
+    SpanBuilder spanBuilder = spanBuilder(parentContext, spanNameForRequest(request), CLIENT);
+    onRequest(spanBuilder, request);
+    NetPeerAttributes.INSTANCE.setNetPeer(
+        spanBuilder, (InetSocketAddress) ctx.channel().remoteAddress());
 
-    Context context = withClientSpan(parentContext, span);
+    Context context = withClientSpan(parentContext, spanBuilder.startSpan());
     inject(context, request.headers(), SETTER);
     return context;
   }