Trace SSL handshakes in netty 4.1 (open-telemetry#4604)

* Trace SSL handshakes in netty 4.1 * Update testing-common/src/main/java/io/opentelemetry/instrumentation/testing/junit/http/HttpClientTestServer.java Co-authored-by: Trask Stalnaker <[email protected]> * remove unneeded bit of code Co-authored-by: Trask Stalnaker <[email protected]>
RashmiRam · May 23, 2022 · e9c33e0 · e9c33e0
1 parent d407ced
commit e9c33e0
Show file tree

Hide file tree

Showing 15 changed files with 515 additions and 6 deletions.
diff --git a/...lemetry/javaagent/instrumentation/netty/common/client/NettyClientInstrumenterFactory.java b/...lemetry/javaagent/instrumentation/netty/common/client/NettyClientInstrumenterFactory.java
@@ -21,11 +21,13 @@ public final class NettyClientInstrumenterFactory {
 
   private final String instrumentationName;
   private final boolean alwaysCreateConnectSpan;
+  private final boolean sslTelemetryEnabled;
 
   public NettyClientInstrumenterFactory(
-      String instrumentationName, boolean alwaysCreateConnectSpan) {
+      String instrumentationName, boolean alwaysCreateConnectSpan, boolean sslTelemetryEnabled) {
     this.instrumentationName = instrumentationName;
     this.alwaysCreateConnectSpan = alwaysCreateConnectSpan;
+    this.sslTelemetryEnabled = sslTelemetryEnabled;
   }
 
   public Instrumenter<HttpRequestAndChannel, HttpResponse> createHttpInstrumenter() {
@@ -66,4 +68,24 @@ public NettyConnectionInstrumenter createConnectionInstrumenter() {
         ? new NettyConnectionInstrumenterImpl(instrumenter)
         : new NettyErrorOnlyConnectionInstrumenter(instrumenter);
   }
+
+  public NettySslInstrumenter createSslInstrumenter() {
+    NettySslNetAttributesExtractor netAttributesExtractor = new NettySslNetAttributesExtractor();
+    Instrumenter<NettySslRequest, Void> instrumenter =
+        Instrumenter.<NettySslRequest, Void>builder(
+                GlobalOpenTelemetry.get(), instrumentationName, NettySslRequest::spanName)
+            .addAttributesExtractor(netAttributesExtractor)
+            .addAttributesExtractor(PeerServiceAttributesExtractor.create(netAttributesExtractor))
+            .setTimeExtractors(
+                request -> request.timer().startTime(),
+                (request, channel, error) -> request.timer().now())
+            .newInstrumenter(
+                sslTelemetryEnabled
+                    ? SpanKindExtractor.alwaysInternal()
+                    : SpanKindExtractor.alwaysClient());
+
+    return sslTelemetryEnabled
+        ? new NettySslInstrumenterImpl(instrumenter)
+        : new NettySslErrorOnlyInstrumenter(instrumenter);
+  }
 }
diff --git a/...elemetry/javaagent/instrumentation/netty/common/client/NettySslErrorOnlyInstrumenter.java b/...elemetry/javaagent/instrumentation/netty/common/client/NettySslErrorOnlyInstrumenter.java
@@ -0,0 +1,38 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.javaagent.instrumentation.netty.common.client;
+
+import io.opentelemetry.context.Context;
+import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter;
+import javax.annotation.Nullable;
+
+final class NettySslErrorOnlyInstrumenter implements NettySslInstrumenter {
+
+  private final Instrumenter<NettySslRequest, Void> instrumenter;
+
+  NettySslErrorOnlyInstrumenter(Instrumenter<NettySslRequest, Void> instrumenter) {
+    this.instrumenter = instrumenter;
+  }
+
+  @Override
+  public boolean shouldStart(Context parentContext, NettySslRequest request) {
+    // the "real" check is done on end() anyway
+    return true;
+  }
+
+  @Override
+  public Context start(Context parentContext, NettySslRequest request) {
+    return parentContext;
+  }
+
+  @Override
+  public void end(Context context, NettySslRequest request, @Nullable Throwable error) {
+    if (error != null && instrumenter.shouldStart(context, request)) {
+      Context connectContext = instrumenter.start(context, request);
+      instrumenter.end(connectContext, request, null, error);
+    }
+  }
+}
diff --git a/...lemetry/javaagent/instrumentation/netty/common/client/NettySslInstrumentationHandler.java b/...lemetry/javaagent/instrumentation/netty/common/client/NettySslInstrumentationHandler.java
@@ -0,0 +1,74 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.javaagent.instrumentation.netty.common.client;
+
+import io.netty.channel.ChannelDuplexHandler;
+import io.netty.channel.ChannelHandlerContext;
+import io.netty.channel.ChannelPromise;
+import io.netty.handler.ssl.SslHandshakeCompletionEvent;
+import io.opentelemetry.context.Context;
+import java.net.SocketAddress;
+
+// inspired by reactor-netty SslProvider.SslReadHandler
+public final class NettySslInstrumentationHandler extends ChannelDuplexHandler {
+
+  private final NettySslInstrumenter instrumenter;
+  private Context parentContext;
+  private NettySslRequest request;
+  private Context context;
+
+  public NettySslInstrumentationHandler(NettySslInstrumenter instrumenter) {
+    this.instrumenter = instrumenter;
+  }
+
+  @Override
+  public void channelRegistered(ChannelHandlerContext ctx) {
+    // remember the parent context from the time of channel registration;
+    // this happens inside Bootstrap#connect()
+    parentContext = Context.current();
+    ctx.fireChannelRegistered();
+  }
+
+  @Override
+  public void connect(
+      ChannelHandlerContext ctx,
+      SocketAddress remoteAddress,
+      SocketAddress localAddress,
+      ChannelPromise promise) {
+
+    // netty SslHandler starts the handshake after it receives the channelActive() signal; this
+    // happens just after the connection is established
+    // this makes connect() promise a good place to start the SSL handshake span
+    promise.addListener(
+        future -> {
+          // there won't be any SSL handshake if the channel fails to connect
+          if (!future.isSuccess()) {
+            return;
+          }
+          request = NettySslRequest.create(ctx.channel());
+          if (instrumenter.shouldStart(parentContext, request)) {
+            context = instrumenter.start(parentContext, request);
+          }
+        });
+    ctx.connect(remoteAddress, localAddress, promise);
+  }
+
+  @Override
+  public void userEventTriggered(ChannelHandlerContext ctx, Object evt) {
+    if (evt instanceof SslHandshakeCompletionEvent) {
+      if (ctx.pipeline().context(this) != null) {
+        ctx.pipeline().remove(this);
+      }
+
+      SslHandshakeCompletionEvent handshake = (SslHandshakeCompletionEvent) evt;
+      if (context != null) {
+        instrumenter.end(context, request, handshake.cause());
+      }
+    }
+
+    ctx.fireUserEventTriggered(evt);
+  }
+}
diff --git a/.../io/opentelemetry/javaagent/instrumentation/netty/common/client/NettySslInstrumenter.java b/.../io/opentelemetry/javaagent/instrumentation/netty/common/client/NettySslInstrumenter.java
@@ -0,0 +1,18 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.javaagent.instrumentation.netty.common.client;
+
+import io.opentelemetry.context.Context;
+import javax.annotation.Nullable;
+
+public interface NettySslInstrumenter {
+
+  boolean shouldStart(Context parentContext, NettySslRequest request);
+
+  Context start(Context parentContext, NettySslRequest request);
+
+  void end(Context context, NettySslRequest request, @Nullable Throwable error);
+}
diff --git a/...opentelemetry/javaagent/instrumentation/netty/common/client/NettySslInstrumenterImpl.java b/...opentelemetry/javaagent/instrumentation/netty/common/client/NettySslInstrumenterImpl.java
@@ -0,0 +1,34 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.javaagent.instrumentation.netty.common.client;
+
+import io.opentelemetry.context.Context;
+import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter;
+import javax.annotation.Nullable;
+
+final class NettySslInstrumenterImpl implements NettySslInstrumenter {
+
+  private final Instrumenter<NettySslRequest, Void> instrumenter;
+
+  NettySslInstrumenterImpl(Instrumenter<NettySslRequest, Void> instrumenter) {
+    this.instrumenter = instrumenter;
+  }
+
+  @Override
+  public boolean shouldStart(Context parentContext, NettySslRequest request) {
+    return instrumenter.shouldStart(parentContext, request);
+  }
+
+  @Override
+  public Context start(Context parentContext, NettySslRequest request) {
+    return instrumenter.start(parentContext, request);
+  }
+
+  @Override
+  public void end(Context context, NettySslRequest request, @Nullable Throwable error) {
+    instrumenter.end(context, request, null, error);
+  }
+}
diff --git a/...lemetry/javaagent/instrumentation/netty/common/client/NettySslNetAttributesExtractor.java b/...lemetry/javaagent/instrumentation/netty/common/client/NettySslNetAttributesExtractor.java
@@ -0,0 +1,32 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.javaagent.instrumentation.netty.common.client;
+
+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.socket.DatagramChannel;
+import io.opentelemetry.instrumentation.api.instrumenter.net.InetSocketAddressNetClientAttributesExtractor;
+import java.net.InetSocketAddress;
+import javax.annotation.Nullable;
+
+final class NettySslNetAttributesExtractor
+    extends InetSocketAddressNetClientAttributesExtractor<NettySslRequest, Void> {
+
+  @Nullable
+  @Override
+  public InetSocketAddress getAddress(NettySslRequest request, @Nullable Void unused) {
+    if (request.remoteAddress() instanceof InetSocketAddress) {
+      return (InetSocketAddress) request.remoteAddress();
+    }
+    return null;
+  }
+
+  @Override
+  public String transport(NettySslRequest request, @Nullable Void unused) {
+    return request.channel() instanceof DatagramChannel ? IP_UDP : IP_TCP;
+  }
+}
diff --git a/.../java/io/opentelemetry/javaagent/instrumentation/netty/common/client/NettySslRequest.java b/.../java/io/opentelemetry/javaagent/instrumentation/netty/common/client/NettySslRequest.java
@@ -0,0 +1,31 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.javaagent.instrumentation.netty.common.client;
+
+import com.google.auto.value.AutoValue;
+import io.netty.channel.Channel;
+import io.opentelemetry.javaagent.instrumentation.netty.common.Timer;
+import java.net.SocketAddress;
+import javax.annotation.Nullable;
+
+@AutoValue
+public abstract class NettySslRequest {
+
+  static NettySslRequest create(Channel channel) {
+    return new AutoValue_NettySslRequest(Timer.start(), channel, channel.remoteAddress());
+  }
+
+  String spanName() {
+    return "SSL handshake";
+  }
+
+  abstract Timer timer();
+
+  abstract Channel channel();
+
+  @Nullable
+  abstract SocketAddress remoteAddress();
+}
diff --git a/...a/io/opentelemetry/javaagent/instrumentation/netty/v4_0/client/NettyClientSingletons.java b/...a/io/opentelemetry/javaagent/instrumentation/netty/v4_0/client/NettyClientSingletons.java
@@ -22,7 +22,8 @@ public final class NettyClientSingletons {
 
   static {
     NettyClientInstrumenterFactory factory =
-        new NettyClientInstrumenterFactory("io.opentelemetry.netty-4.0", alwaysCreateConnectSpan);
+        new NettyClientInstrumenterFactory(
+            "io.opentelemetry.netty-4.0", alwaysCreateConnectSpan, false);
     INSTRUMENTER = factory.createHttpInstrumenter();
     CONNECTION_INSTRUMENTER = factory.createConnectionInstrumenter();
   }

diff --git a/instrumentation/netty/netty-4.1/javaagent/build.gradle.kts b/instrumentation/netty/netty-4.1/javaagent/build.gradle.kts
@@ -46,10 +46,12 @@ tasks {
   val testConnectionSpan by registering(Test::class) {
     filter {
       includeTestsMatching("Netty41ConnectionSpanTest")
+      includeTestsMatching("Netty41ClientSslTest")
       isFailOnNoMatchingTests = false
     }
-    include("**/Netty41ConnectionSpanTest.*")
+    include("**/Netty41ConnectionSpanTest.*", "**/Netty41ClientSslTest.*")
     jvmArgs("-Dotel.instrumentation.netty.always-create-connect-span=true")
+    jvmArgs("-Dotel.instrumentation.netty.ssl-telemetry.enabled=true")
   }
 
   test {
@@ -58,6 +60,7 @@ tasks {
     dependsOn(testConnectionSpan)
     filter {
       excludeTestsMatching("Netty41ConnectionSpanTest")
+      excludeTestsMatching("Netty41ClientSslTest")
       isFailOnNoMatchingTests = false
     }
   }

diff --git a/...entelemetry/javaagent/instrumentation/netty/v4_1/NettyChannelPipelineInstrumentation.java b/...entelemetry/javaagent/instrumentation/netty/v4_1/NettyChannelPipelineInstrumentation.java
@@ -5,6 +5,7 @@
 
 package io.opentelemetry.javaagent.instrumentation.netty.v4_1;
 
+import static io.opentelemetry.javaagent.instrumentation.netty.v4_1.client.NettyClientSingletons.sslInstrumenter;
 import static net.bytebuddy.matcher.ElementMatchers.isMethod;
 import static net.bytebuddy.matcher.ElementMatchers.nameStartsWith;
 import static net.bytebuddy.matcher.ElementMatchers.named;
@@ -23,6 +24,7 @@
 import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer;
 import io.opentelemetry.javaagent.instrumentation.api.CallDepth;
 import io.opentelemetry.javaagent.instrumentation.netty.common.AbstractNettyChannelPipelineInstrumentation;
+import io.opentelemetry.javaagent.instrumentation.netty.common.client.NettySslInstrumentationHandler;
 import io.opentelemetry.javaagent.instrumentation.netty.v4_1.client.HttpClientRequestTracingHandler;
 import io.opentelemetry.javaagent.instrumentation.netty.v4_1.client.HttpClientResponseTracingHandler;
 import io.opentelemetry.javaagent.instrumentation.netty.v4_1.client.HttpClientTracingHandler;
@@ -115,6 +117,10 @@ public static void addHandler(
         ourHandler = new HttpClientRequestTracingHandler();
       } else if (handler instanceof HttpResponseDecoder) {
         ourHandler = new HttpClientResponseTracingHandler();
+        // the SslHandler lives in the netty-handler module, using class name comparison to avoid
+        // adding a dependency
+      } else if (handler.getClass().getName().equals("io.netty.handler.ssl.SslHandler")) {
+        ourHandler = new NettySslInstrumentationHandler(sslInstrumenter());
       }
 
       if (ourHandler != null) {

diff --git a/...a/io/opentelemetry/javaagent/instrumentation/netty/v4_1/client/NettyClientSingletons.java b/...a/io/opentelemetry/javaagent/instrumentation/netty/v4_1/client/NettyClientSingletons.java
@@ -12,6 +12,7 @@
 import io.opentelemetry.javaagent.instrumentation.netty.common.HttpRequestAndChannel;
 import io.opentelemetry.javaagent.instrumentation.netty.common.client.NettyClientInstrumenterFactory;
 import io.opentelemetry.javaagent.instrumentation.netty.common.client.NettyConnectionInstrumenter;
+import io.opentelemetry.javaagent.instrumentation.netty.common.client.NettySslInstrumenter;
 
 public final class NettyClientSingletons {
 
@@ -22,15 +23,20 @@ public final class NettyClientSingletons {
 
   private static final boolean alwaysCreateConnectSpan =
       Config.get().getBoolean("otel.instrumentation.netty.always-create-connect-span", false);
+  private static final boolean sslTelemetryEnabled =
+      Config.get().getBoolean("otel.instrumentation.netty.ssl-telemetry.enabled", false);
 
   private static final Instrumenter<HttpRequestAndChannel, HttpResponse> INSTRUMENTER;
   private static final NettyConnectionInstrumenter CONNECTION_INSTRUMENTER;
+  private static final NettySslInstrumenter SSL_INSTRUMENTER;
 
   static {
     NettyClientInstrumenterFactory factory =
-        new NettyClientInstrumenterFactory("io.opentelemetry.netty-4.1", alwaysCreateConnectSpan);
+        new NettyClientInstrumenterFactory(
+            "io.opentelemetry.netty-4.1", alwaysCreateConnectSpan, sslTelemetryEnabled);
     INSTRUMENTER = factory.createHttpInstrumenter();
     CONNECTION_INSTRUMENTER = factory.createConnectionInstrumenter();
+    SSL_INSTRUMENTER = factory.createSslInstrumenter();
   }
 
   public static Instrumenter<HttpRequestAndChannel, HttpResponse> instrumenter() {
@@ -41,5 +47,9 @@ public static NettyConnectionInstrumenter connectionInstrumenter() {
     return CONNECTION_INSTRUMENTER;
   }
 
+  public static NettySslInstrumenter sslInstrumenter() {
+    return SSL_INSTRUMENTER;
+  }
+
   private NettyClientSingletons() {}
 }