Add attributes to netty connection failure span (open-telemetry#3115)

robododge · Jun 17, 2021 · 9f9ce5c · 9f9ce5c
1 parent 6647230
commit 9f9ce5c
Show file tree

Hide file tree

Showing 13 changed files with 94 additions and 52 deletions.
diff --git a/...ntelemetry/javaagent/instrumentation/netty/v3_8/ChannelFutureListenerInstrumentation.java b/...ntelemetry/javaagent/instrumentation/netty/v3_8/ChannelFutureListenerInstrumentation.java
@@ -12,7 +12,6 @@
 import static net.bytebuddy.matcher.ElementMatchers.named;
 import static net.bytebuddy.matcher.ElementMatchers.takesArgument;
 
-import io.opentelemetry.api.trace.SpanKind;
 import io.opentelemetry.context.Context;
 import io.opentelemetry.context.Scope;
 import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
@@ -70,8 +69,7 @@ public static Scope activateScope(@Advice.Argument(0) ChannelFuture future) {
       }
       Scope parentScope = parentContext.makeCurrent();
       if (channelTraceContext.createConnectionSpan()) {
-        Context errorContext = tracer().startSpan("CONNECT", SpanKind.CLIENT);
-        tracer().endExceptionally(errorContext, cause);
+        tracer().connectionFailure(parentContext, future.getChannel(), cause);
       }
       return parentScope;
     }

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
@@ -7,18 +7,23 @@
 
 import static io.opentelemetry.api.trace.SpanKind.CLIENT;
 import static io.opentelemetry.javaagent.instrumentation.netty.v3_8.client.NettyResponseInjectAdapter.SETTER;
+import static io.opentelemetry.semconv.trace.attributes.SemanticAttributes.NetTransportValues.IP_TCP;
+import static io.opentelemetry.semconv.trace.attributes.SemanticAttributes.NetTransportValues.IP_UDP;
 import static org.jboss.netty.handler.codec.http.HttpHeaders.Names.HOST;
 
 import io.opentelemetry.api.trace.SpanBuilder;
 import io.opentelemetry.context.Context;
 import io.opentelemetry.context.propagation.TextMapSetter;
 import io.opentelemetry.instrumentation.api.tracer.HttpClientTracer;
 import io.opentelemetry.instrumentation.api.tracer.net.NetPeerAttributes;
+import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
 import java.net.InetSocketAddress;
 import java.net.URI;
 import java.net.URISyntaxException;
 import org.checkerframework.checker.nullness.qual.Nullable;
+import org.jboss.netty.channel.Channel;
 import org.jboss.netty.channel.ChannelHandlerContext;
+import org.jboss.netty.channel.socket.DatagramChannel;
 import org.jboss.netty.handler.codec.http.HttpHeaders;
 import org.jboss.netty.handler.codec.http.HttpRequest;
 import org.jboss.netty.handler.codec.http.HttpResponse;
@@ -46,6 +51,17 @@ public Context startSpan(Context parentContext, ChannelHandlerContext ctx, HttpR
     return context;
   }
 
+  public void connectionFailure(Context parentContext, Channel channel, Throwable throwable) {
+    SpanBuilder spanBuilder = spanBuilder(parentContext, "CONNECT", CLIENT);
+    spanBuilder.setAttribute(
+        SemanticAttributes.NET_TRANSPORT, channel instanceof DatagramChannel ? IP_UDP : IP_TCP);
+    NetPeerAttributes.INSTANCE.setNetPeer(
+        spanBuilder, (InetSocketAddress) channel.getRemoteAddress());
+
+    Context context = withClientSpan(parentContext, spanBuilder.startSpan());
+    tracer().endExceptionally(context, throwable);
+  }
+
   @Override
   protected String method(HttpRequest httpRequest) {
     return httpRequest.getMethod().getName();

diff --git a/instrumentation/netty/netty-3.8/javaagent/src/test/groovy/Netty38ClientTest.groovy b/instrumentation/netty/netty-3.8/javaagent/src/test/groovy/Netty38ClientTest.groovy
@@ -108,7 +108,7 @@ class Netty38ClientTest extends HttpClientTest<Request> implements AgentTestTrai
   }
 
   @Override
-  boolean hasClientSpanAttributes(URI uri) {
+  boolean hasClientSpanHttpAttributes(URI uri) {
     switch (uri.toString()) {
       case "http://localhost:61/": // unopened port
       case "http://www.google.com:81/": // dropped request

diff --git a/...ntelemetry/javaagent/instrumentation/netty/v4_0/ChannelFutureListenerInstrumentation.java b/...ntelemetry/javaagent/instrumentation/netty/v4_0/ChannelFutureListenerInstrumentation.java
@@ -13,7 +13,6 @@
 import static net.bytebuddy.matcher.ElementMatchers.takesArgument;
 
 import io.netty.channel.ChannelFuture;
-import io.opentelemetry.api.trace.SpanKind;
 import io.opentelemetry.context.Context;
 import io.opentelemetry.context.Scope;
 import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
@@ -62,8 +61,7 @@ public static Scope activateScope(@Advice.Argument(0) ChannelFuture future) {
 
       Scope parentScope = parentContext.makeCurrent();
       if (tracer().shouldStartSpan(parentContext)) {
-        Context errorContext = tracer().startSpan("CONNECT", SpanKind.CLIENT);
-        tracer().endExceptionally(errorContext, cause);
+        tracer().connectionFailure(parentContext, future.channel(), cause);
       }
       return parentScope;
     }

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
@@ -8,8 +8,12 @@
 import static io.netty.handler.codec.http.HttpHeaders.Names.HOST;
 import static io.opentelemetry.api.trace.SpanKind.CLIENT;
 import static io.opentelemetry.javaagent.instrumentation.netty.common.client.NettyResponseInjectAdapter.SETTER;
+import static io.opentelemetry.semconv.trace.attributes.SemanticAttributes.NetTransportValues.IP_TCP;
+import static io.opentelemetry.semconv.trace.attributes.SemanticAttributes.NetTransportValues.IP_UDP;
 
+import io.netty.channel.Channel;
 import io.netty.channel.ChannelHandlerContext;
+import io.netty.channel.socket.DatagramChannel;
 import io.netty.handler.codec.http.HttpHeaders;
 import io.netty.handler.codec.http.HttpRequest;
 import io.netty.handler.codec.http.HttpResponse;
@@ -18,6 +22,7 @@
 import io.opentelemetry.context.propagation.TextMapSetter;
 import io.opentelemetry.instrumentation.api.tracer.HttpClientTracer;
 import io.opentelemetry.instrumentation.api.tracer.net.NetPeerAttributes;
+import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
 import java.net.InetSocketAddress;
 import java.net.URI;
 import java.net.URISyntaxException;
@@ -46,6 +51,16 @@ public Context startSpan(Context parentContext, ChannelHandlerContext ctx, HttpR
     return context;
   }
 
+  public void connectionFailure(Context parentContext, Channel channel, Throwable throwable) {
+    SpanBuilder spanBuilder = spanBuilder(parentContext, "CONNECT", CLIENT);
+    spanBuilder.setAttribute(
+        SemanticAttributes.NET_TRANSPORT, channel instanceof DatagramChannel ? IP_UDP : IP_TCP);
+    NetPeerAttributes.INSTANCE.setNetPeer(spanBuilder, (InetSocketAddress) channel.remoteAddress());
+
+    Context context = withClientSpan(parentContext, spanBuilder.startSpan());
+    tracer().endExceptionally(context, throwable);
+  }
+
   @Override
   protected String method(HttpRequest httpRequest) {
     return httpRequest.getMethod().name();

diff --git a/instrumentation/netty/netty-4.0/javaagent/src/test/groovy/Netty40ClientTest.groovy b/instrumentation/netty/netty-4.0/javaagent/src/test/groovy/Netty40ClientTest.groovy
@@ -92,7 +92,7 @@ class Netty40ClientTest extends HttpClientTest<DefaultFullHttpRequest> implement
   }
 
   @Override
-  boolean hasClientSpanAttributes(URI uri) {
+  boolean hasClientSpanHttpAttributes(URI uri) {
     switch (uri.toString()) {
       case "http://localhost:61/": // unopened port
       case "http://www.google.com:81/": // dropped request

diff --git a/...ntelemetry/javaagent/instrumentation/netty/v4_1/ChannelFutureListenerInstrumentation.java b/...ntelemetry/javaagent/instrumentation/netty/v4_1/ChannelFutureListenerInstrumentation.java
@@ -63,8 +63,7 @@ public static Scope activateScope(@Advice.Argument(0) ChannelFuture future) {
 
       Scope parentScope = parentContext.makeCurrent();
       if (tracer().shouldStartSpan(parentContext, SpanKind.CLIENT)) {
-        Context errorContext = tracer().startSpan("CONNECT", SpanKind.CLIENT);
-        tracer().endExceptionally(errorContext, cause);
+        tracer().connectionFailure(parentContext, future.channel(), cause);
       }
       return parentScope;
     }

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
@@ -8,8 +8,12 @@
 import static io.netty.handler.codec.http.HttpHeaderNames.HOST;
 import static io.opentelemetry.api.trace.SpanKind.CLIENT;
 import static io.opentelemetry.javaagent.instrumentation.netty.common.client.NettyResponseInjectAdapter.SETTER;
+import static io.opentelemetry.semconv.trace.attributes.SemanticAttributes.NetTransportValues.IP_TCP;
+import static io.opentelemetry.semconv.trace.attributes.SemanticAttributes.NetTransportValues.IP_UDP;
 
+import io.netty.channel.Channel;
 import io.netty.channel.ChannelHandlerContext;
+import io.netty.channel.socket.DatagramChannel;
 import io.netty.handler.codec.http.HttpHeaders;
 import io.netty.handler.codec.http.HttpRequest;
 import io.netty.handler.codec.http.HttpResponse;
@@ -18,6 +22,7 @@
 import io.opentelemetry.context.propagation.TextMapSetter;
 import io.opentelemetry.instrumentation.api.tracer.HttpClientTracer;
 import io.opentelemetry.instrumentation.api.tracer.net.NetPeerAttributes;
+import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
 import java.net.InetSocketAddress;
 import java.net.URI;
 import java.net.URISyntaxException;
@@ -46,6 +51,16 @@ public Context startSpan(Context parentContext, ChannelHandlerContext ctx, HttpR
     return context;
   }
 
+  public void connectionFailure(Context parentContext, Channel channel, Throwable throwable) {
+    SpanBuilder spanBuilder = spanBuilder(parentContext, "CONNECT", CLIENT);
+    spanBuilder.setAttribute(
+        SemanticAttributes.NET_TRANSPORT, channel instanceof DatagramChannel ? IP_UDP : IP_TCP);
+    NetPeerAttributes.INSTANCE.setNetPeer(spanBuilder, (InetSocketAddress) channel.remoteAddress());
+
+    Context context = withClientSpan(parentContext, spanBuilder.startSpan());
+    tracer().endExceptionally(context, throwable);
+  }
+
   @Override
   protected String method(HttpRequest httpRequest) {
     return httpRequest.method().name();

diff --git a/instrumentation/netty/netty-4.1/javaagent/src/test/groovy/Netty41ClientTest.groovy b/instrumentation/netty/netty-4.1/javaagent/src/test/groovy/Netty41ClientTest.groovy
@@ -110,7 +110,7 @@ class Netty41ClientTest extends HttpClientTest<DefaultFullHttpRequest> implement
   }
 
   @Override
-  boolean hasClientSpanAttributes(URI uri) {
+  boolean hasClientSpanHttpAttributes(URI uri) {
     switch (uri.toString()) {
       case "http://localhost:61/": // unopened port
       case "http://www.google.com:81/": // dropped request

diff --git a/instrumentation/ratpack-1.4/javaagent/src/test/groovy/client/RatpackHttpClientTest.groovy b/instrumentation/ratpack-1.4/javaagent/src/test/groovy/client/RatpackHttpClientTest.groovy
@@ -99,7 +99,7 @@ class RatpackHttpClientTest extends HttpClientTest<Void> implements AgentTestTra
   }
 
   @Override
-  boolean hasClientSpanAttributes(URI uri) {
+  boolean hasClientSpanHttpAttributes(URI uri) {
     switch (uri.toString()) {
       case "http://localhost:61/": // unopened port
       case "http://www.google.com:81/": // dropped request

diff --git a/...tty/reactor-netty-0.9/javaagent/src/test/groovy/AbstractReactorNettyHttpClientTest.groovy b/...tty/reactor-netty-0.9/javaagent/src/test/groovy/AbstractReactorNettyHttpClientTest.groovy
@@ -84,7 +84,7 @@ abstract class AbstractReactorNettyHttpClientTest extends HttpClientTest<HttpCli
   }
 
   @Override
-  boolean hasClientSpanAttributes(URI uri) {
+  boolean hasClientSpanHttpAttributes(URI uri) {
     switch (uri.toString()) {
       case "http://localhost:61/": // unopened port
       case "http://www.google.com:81/": // dropped request

diff --git a/...tty/reactor-netty-1.0/javaagent/src/test/groovy/AbstractReactorNettyHttpClientTest.groovy b/...tty/reactor-netty-1.0/javaagent/src/test/groovy/AbstractReactorNettyHttpClientTest.groovy
@@ -84,7 +84,7 @@ abstract class AbstractReactorNettyHttpClientTest extends HttpClientTest<HttpCli
   }
 
   @Override
-  boolean hasClientSpanAttributes(URI uri) {
+  boolean hasClientSpanHttpAttributes(URI uri) {
     switch (uri.toString()) {
       case "http://localhost:61/": // unopened port
       case "http://www.google.com:81/": // dropped request

diff --git a/...g-common/src/main/groovy/io/opentelemetry/instrumentation/test/base/HttpClientTest.groovy b/...g-common/src/main/groovy/io/opentelemetry/instrumentation/test/base/HttpClientTest.groovy
@@ -821,6 +821,7 @@ abstract class HttpClientTest<REQUEST> extends InstrumentationSpecification {
   void clientSpan(TraceAssert trace, int index, Object parentSpan, String method = "GET", URI uri = server.address.resolve("/success"), Integer responseCode = 200, Throwable exception = null, String httpFlavor = "1.1") {
     def userAgent = userAgent()
     def extraAttributes = extraAttributes()
+    def hasClientSpanHttpAttributes = hasClientSpanHttpAttributes(uri)
     trace.span(index) {
       if (parentSpan == null) {
         hasNoParent()
@@ -835,25 +836,25 @@ abstract class HttpClientTest<REQUEST> extends InstrumentationSpecification {
       } else if (responseCode >= 400) {
         status ERROR
       }
-      if (hasClientSpanAttributes(uri)) {
-        attributes {
-          "${SemanticAttributes.NET_TRANSPORT.key}" IP_TCP
-          if (uri.port == UNUSABLE_PORT || uri.host == "192.0.2.1" || (uri.host == "www.google.com" && uri.port == 81)) {
-            // TODO(anuraaga): For theses cases, there isn't actually a peer so we shouldn't be
-            // filling in peer information but some instrumentation does so based on the URL itself
-            // which is present in HTTP attributes. We should fix this.
-            "${SemanticAttributes.NET_PEER_NAME.key}" { it == null || it == uri.host }
-            "${SemanticAttributes.NET_PEER_PORT.key}" { it == null || it == uri.port || (uri.scheme == "https" && it == 443) }
-          } else {
-            "${SemanticAttributes.NET_PEER_NAME.key}" uri.host
-            "${SemanticAttributes.NET_PEER_PORT.key}" uri.port > 0 ? uri.port : { it == null || it == 443 }
-          }
-          if (uri.host == "www.google.com") {
-            // unpredictable IP address (or can be none if no connection is made, see comment above)
-            "${SemanticAttributes.NET_PEER_IP.key}" { it == null || it instanceof String }
-          } else {
-            "${SemanticAttributes.NET_PEER_IP.key}" { it == null || it == "127.0.0.1" || it == uri.host  } // Optional
-          }
+      attributes {
+        "${SemanticAttributes.NET_TRANSPORT.key}" IP_TCP
+        if (uri.port == UNUSABLE_PORT || uri.host == "192.0.2.1" || (uri.host == "www.google.com" && uri.port == 81)) {
+          // TODO(anuraaga): For theses cases, there isn't actually a peer so we shouldn't be
+          // filling in peer information but some instrumentation does so based on the URL itself
+          // which is present in HTTP attributes. We should fix this.
+          "${SemanticAttributes.NET_PEER_NAME.key}" { it == null || it == uri.host }
+          "${SemanticAttributes.NET_PEER_PORT.key}" { it == null || it == uri.port || (uri.scheme == "https" && it == 443) }
+        } else {
+          "${SemanticAttributes.NET_PEER_NAME.key}" uri.host
+          "${SemanticAttributes.NET_PEER_PORT.key}" uri.port > 0 ? uri.port : { it == null || it == 443 }
+        }
+        if (uri.host == "www.google.com") {
+          // unpredictable IP address (or can be none if no connection is made, see comment above)
+          "${SemanticAttributes.NET_PEER_IP.key}" { it == null || it instanceof String }
+        } else {
+          "${SemanticAttributes.NET_PEER_IP.key}" { it == null || it == "127.0.0.1" || it == uri.host  } // Optional
+        }
+        if (hasClientSpanHttpAttributes) {
           "${SemanticAttributes.HTTP_URL.key}" { it == "${uri}" || it == "${removeFragment(uri)}" }
           "${SemanticAttributes.HTTP_METHOD.key}" method
           if (uri.host == "www.google.com") {
@@ -864,25 +865,25 @@ abstract class HttpClientTest<REQUEST> extends InstrumentationSpecification {
           if (userAgent) {
             "${SemanticAttributes.HTTP_USER_AGENT.key}" { it.startsWith(userAgent) }
           }
-          if (responseCode) {
-            "${SemanticAttributes.HTTP_STATUS_CODE.key}" responseCode
-          }
+        }
+        if (responseCode) {
+          "${SemanticAttributes.HTTP_STATUS_CODE.key}" responseCode
+        }
 
-          if (extraAttributes.contains(SemanticAttributes.HTTP_HOST)) {
-            "${SemanticAttributes.HTTP_HOST}" { it == uri.host || it == "${uri.host}:${uri.port}" }
-          }
-          if (extraAttributes.contains(SemanticAttributes.HTTP_REQUEST_CONTENT_LENGTH)) {
-            "${SemanticAttributes.HTTP_REQUEST_CONTENT_LENGTH}" Long
-          }
-          if (extraAttributes.contains(SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH)) {
-            "${SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH}" Long
-          }
-          if (extraAttributes.contains(SemanticAttributes.HTTP_SCHEME)) {
-            "${SemanticAttributes.HTTP_SCHEME}" uri.scheme
-          }
-          if (extraAttributes.contains(SemanticAttributes.HTTP_TARGET)) {
-            "${SemanticAttributes.HTTP_TARGET}" uri.path + "${uri.query != null ? "?${uri.query}" : ""}"
-          }
+        if (extraAttributes.contains(SemanticAttributes.HTTP_HOST)) {
+          "${SemanticAttributes.HTTP_HOST}" { it == uri.host || it == "${uri.host}:${uri.port}" }
+        }
+        if (extraAttributes.contains(SemanticAttributes.HTTP_REQUEST_CONTENT_LENGTH)) {
+          "${SemanticAttributes.HTTP_REQUEST_CONTENT_LENGTH}" Long
+        }
+        if (extraAttributes.contains(SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH)) {
+          "${SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH}" Long
+        }
+        if (extraAttributes.contains(SemanticAttributes.HTTP_SCHEME)) {
+          "${SemanticAttributes.HTTP_SCHEME}" uri.scheme
+        }
+        if (extraAttributes.contains(SemanticAttributes.HTTP_TARGET)) {
+          "${SemanticAttributes.HTTP_TARGET}" uri.path + "${uri.query != null ? "?${uri.query}" : ""}"
         }
       }
     }
@@ -921,7 +922,7 @@ abstract class HttpClientTest<REQUEST> extends InstrumentationSpecification {
     spanAssert.errorEvent(errorType, message)
   }
 
-  boolean hasClientSpanAttributes(URI uri) {
+  boolean hasClientSpanHttpAttributes(URI uri) {
     true
   }