Trace SSL handshakes in netty 4.1 #4604
Conversation
mateuszrzeszutek
requested review from
anuraaga,
iNikem,
jkwatson,
laurit,
pavolloffay,
trask and
tylerbenson
as code owners
.../src/main/java/io/opentelemetry/instrumentation/testing/junit/http/HttpClientTestServer.java
Outdated
Show resolved
Hide resolved
instrumentation/netty/netty-4.1/javaagent/src/test/groovy/Netty41ClientSslTest.groovy
Show resolved
Hide resolved
| } | ||
| span(3) { | ||
| name "SSL handshake" | ||
| kind INTERNAL |
There was a problem hiding this comment.
should RESOLVE, CONNECT and SSL handshake be CLIENT spans? (users would need to enable outgoing-span-suppression-by-type=true, or we could consider making suppression-by-type the default)
sort of related, what do you think of converting the "error-only" spans into span events on the current span? this way those would still get captured even when using the "no nested CLIENT spans" setting
I can create an issue to discuss both of these further since they have broader impact than just this PR
There was a problem hiding this comment.
I can create an issue to discuss both of these further since they have broader impact than just this PR
Yes, please! There's a lot of things going on here that we could discuss before establishing a "common pattern" for tracing these things.
should
RESOLVE,CONNECTandSSL handshakebeCLIENTspans?
I think it makes sense for them to be CLIENT spans. I was following the current implementation, which used INTERNAL everywhere though.
users would need to enable
outgoing-span-suppression-by-type=true, or we could consider making suppression-by-type the default)
In plain netty the connection phase and actually sending the HTTP request are completely separate; so all those low-level spans are not the children of the CLIENT span. In case of "normal"/higher-level HTTP clients I think it'd make perfect sense for them to be nested under the HTTP CLIENT span.
sort of related, what do you think of converting the "error-only" spans into span events on the current span? this way those would still get captured even when using the "no nested CLIENT spans" setting
Hmm, do we want that in all cases? Or only when nested spans are disabled?
Using spans for these operations (as opposing to events) enables gathering timing and exception information; e.g. you know that SSL handshake took 10 seconds and threw an exception, and it failed before the HTTP request was sent.
There was a problem hiding this comment.
👍 created issue to continue discussion #4617
...ntelemetry/javaagent/instrumentation/netty/common/client/NettySslInstrumentationHandler.java
Outdated
Show resolved
Hide resolved
…testing/junit/http/HttpClientTestServer.java Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
* 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 <trask.stalnaker@gmail.com> * remove unneeded bit of code Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
This PR adds telemetry to netty's SSL handshakes. It only includes netty 4.1 changes -- I'll implement the same thing in netty 4.0 and reactor-netty once this PR is merged.
CC @lmolkova