Trace DNS resolution in Netty 4.1 #4587
Conversation
mateuszrzeszutek
requested review from
anuraaga,
iNikem,
jkwatson,
laurit,
pavolloffay,
trask and
tylerbenson
as code owners
| @@ -24,21 +24,21 @@ | |||
| Config.get().getBoolean("otel.instrumentation.netty.always-create-connect-span", false); | |||
There was a problem hiding this comment.
Can we still change property names? According to VERSIONING.md
Changes to configuration properties (...) will be considered breaking changes (unless they only affect telemetry produced by instrumentation)
This only affects generated telemetry, so maybe it'd be okay to rename it? I've been thinking about renaming this to sth like otel.instrumentation.<module>.connection-telemetry.enabled
There was a problem hiding this comment.
Yes we can rename this since it only affects telemetry shape
There was a problem hiding this comment.
Nice. I'll do that in a separate PR, together with docs for the new property.
| public static void onExit(@Advice.This Bootstrap bootstrap) { | ||
| // this is already the default value, but we're calling the resolver() method to invoke its | ||
| // instrumentation | ||
| bootstrap.resolver(DefaultAddressResolverGroup.INSTANCE); | ||
| } |
There was a problem hiding this comment.
below alternative feels less strange, though I'm not sure what protection we get from muzzle for structure dependency like FieldValue? e.g. if the field had a different name for netty 4.1.20, would the CI muzzle check catch that? (or we'd only catch it if we ran tests against that version)
| public static void onExit(@Advice.This Bootstrap bootstrap) { | |
| // this is already the default value, but we're calling the resolver() method to invoke its | |
| // instrumentation | |
| bootstrap.resolver(DefaultAddressResolverGroup.INSTANCE); | |
| } | |
| public static void onExit( | |
| @Advice.FieldValue(value = "resolver", readOnly = false) AddressResolverGroup<?> resolver) { | |
| resolver = InstrumentedAddressResolverGroup.wrap(connectionInstrumenter(), resolver); | |
| } |
There was a problem hiding this comment.
No protection at all, I'm afraid; ByteBuddy would just fail when applying this particular advice class and apply everything else.
...ntelemetry/javaagent/instrumentation/netty/v4_1/client/InstrumentedAddressResolverGroup.java
Show resolved
Hide resolved
| Instrumenter<NettyConnectionRequest, Channel> instrumenter = | ||
| Instrumenter.<NettyConnectionRequest, Channel>builder( | ||
| GlobalOpenTelemetry.get(), instrumentationName, NettyConnectionRequest::spanName) |
There was a problem hiding this comment.
I sort of see these as two different instrumenters, one for resolve and one for connect, but ok with this too
There was a problem hiding this comment.
That was my first idea too, but then I noticed that they're basically the same; so I ended up with just reusing it and changing just the span name.
…ntelemetry/javaagent/instrumentation/netty/v4_1/client/InstrumentedAddressResolverGroup.java Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
* Trace DNS resolution in Netty 4.1 * Fix reactor-netty 0.9 tests * Update instrumentation/netty/netty-4.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v4_1/client/InstrumentedAddressResolverGroup.java Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com> Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
Previously, the netty 4.1 instrumentation created a single
CONNECTspan for both DNS resolution and the actual connection. This PR splits it into two spans:RESOLVEthat captures DNS resolution andCONNECTcapturing the actual connection.I'm planning to implement the same change in reactor netty next - it will share a lot of classes with netty 4.1 (e.g. the instrumented
AddressResolverGroupimplementattion). Netty 3.8 and 4.0 don't seem to support any resolution abstraction (other than passing aInetSocketAddress) so I won't implement this change there.