Add option to create span on new netty connection #3707
Conversation
laurit
requested review from
anuraaga,
iNikem,
jkwatson,
mateuszrzeszutek,
pavolloffay,
trask and
tylerbenson
as code owners
There was a problem hiding this comment.
Do I understand correctly:
- If
always-create-connect-span=truethen we create span both for successful and failed connections and their durations are "true", not 0ms? - if
always-create-connect-span=falsewe still create span for failed connection with 0 duration?
...entelemetry/javaagent/instrumentation/netty/common/client/AbstractNettyHttpClientTracer.java
Show resolved
Hide resolved
.../opentelemetry/javaagent/instrumentation/netty/v4_0/NettyChannelPipelineInstrumentation.java
Show resolved
Hide resolved
|
@iNikem yes that is correct. |
laurit
force-pushed
the
netty-connect-span
branch
from
68546af
to
4639fe7
Compare
| @@ -52,7 +53,7 @@ public Context startSpan(Context parentContext, ChannelHandlerContext ctx, HttpR | |||
| } | |||
|
|
|||
| public void connectionFailure(Context parentContext, Channel channel, Throwable throwable) { | |||
| SpanBuilder spanBuilder = spanBuilder(parentContext, "CONNECT", CLIENT); | |||
| SpanBuilder spanBuilder = spanBuilder(parentContext, "CONNECT", INTERNAL); | |||
There was a problem hiding this comment.
When we always create connection span it could be nested under a client span so using INTERNAL. When creating connection span only for failure it could be kept as CLIENT though as the connection failed it shouldn't have a child SERVER span so changed this also to INTERNAL.
There was a problem hiding this comment.
As we currently suppress nested CLIENT spans, we cannot have these new spans as CLIENT. Because they will be suppressed almost always. So we should have them INTERNAL for now.
There was a problem hiding this comment.
I'd prefer to keep this one here as CLIENT, and the other one can be INTERNAL for now until we support nested CLIENT spans in #3691.
CLIENT - Indicates that the span describes a request to some remote service
INTERNAL - Indicates that the span represents an internal operation within an application
| SocketAddress remoteAddress, | ||
| Channel channel, | ||
| Throwable throwable) { | ||
| if (context != null) { |
There was a problem hiding this comment.
Nit: using alwaysCreateConnectSpan will probably allow JIT to skip the else part easily when that property is true
There was a problem hiding this comment.
Added alwaysCreateConnectSpan check, though I suspect that it won't change much for jit as I believe it speculatively removes branches that have not executed anyway.
| @@ -25,6 +32,9 @@ | |||
| public void transform(TypeTransformer transformer) { | |||
| transformer.applyAdviceToMethod( | |||
| isConstructor(), BootstrapInstrumentation.class.getName() + "$InitAdvice"); | |||
| transformer.applyAdviceToMethod( | |||
| named("doConnect").and(takesArgument(0, named("java.net.SocketAddress"))), | |||
There was a problem hiding this comment.
Nit:
| named("doConnect").and(takesArgument(0, named("java.net.SocketAddress"))), | |
| named("doConnect").and(takesArgument(0, SocketAddress.class)), |
| } | ||
|
|
||
| def cleanupSpec() { | ||
| eventLoopGroup?.shutdownGracefully() |
There was a problem hiding this comment.
Is it possible for eventLoopGroup to be null? Is that ? needed?
There was a problem hiding this comment.
Removed it, I guess I have developed a habit of adding too many of these to cleanupSpec as npe in cleanupSpec is fairly common when something fails during initialization
| import java.net.InetSocketAddress; | ||
| import java.net.SocketAddress; | ||
|
|
||
| public class ReactorNettyTracer extends BaseTracer { |
There was a problem hiding this comment.
can you change this to Instrumenter, or add it to #2713?
Resolves #3319
Currently we create a zero length span when netty fails to connect and not inside a client span. This pr adds an option to capture a span around netty establishing connection.