Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Trace SSL handshakes in netty 4.0 #4635

Merged
merged 1 commit into from
Nov 16, 2021
Merged

Trace SSL handshakes in netty 4.0 #4635

merged 1 commit into from
Nov 16, 2021

Conversation

mateuszrzeszutek
Copy link
Member

Continuation of #4604

iNikem
iNikem reviewed Nov 13, 2021
View reviewed changes
Copy link
Contributor

@iNikem iNikem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have we already discussed having this as event as opposed to a span? Is there any separation between start/end for that span?

...ain/java/io/opentelemetry/javaagent/instrumentation/netty/v4_0/BootstrapInstrumentation.java
named("doConnect")
.and(takesArgument(0, SocketAddress.class))
.and(returns(named("io.netty.channel.ChannelFuture"))),
named("doConnect0")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just out of curiosity - why this change?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The doConnect actually traces more than just connection - it starts with tracing channel registration (which happens before) and ends after handler channelActive() has returned (which is way after SSL handshake is finished). The doConnect0 method seems to be doing just the connection, which is more accurate.

@mateuszrzeszutek
Copy link
Member Author

Have we already discussed having this as event as opposed to a span? Is there any separation between start/end for that span?

We've briefly touched upon this idea in the last PR, and we have #4617 where we can discuss it further. To sum up the previously mentioned points: SSL handshake takes time and has non-zero duration, and can fail - we can record the exception and set the span status.

@mateuszrzeszutek mateuszrzeszutek merged commit 7caa402 into open-telemetry:main Nov 16, 2021
@mateuszrzeszutek mateuszrzeszutek deleted the netty-4.0-ssl branch November 16, 2021 07:36
RashmiRam pushed a commit to RashmiRam/opentelemetry-auto-instr-java that referenced this pull request May 23, 2022
@RashmiRam
Trace SSL handshakes in netty 4.0 (open-telemetry#4635)
2a284e0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iNikem iNikem iNikem approved these changes

@anuraaga anuraaga Awaiting requested review from anuraaga

@jkwatson jkwatson Awaiting requested review from jkwatson

@laurit laurit Awaiting requested review from laurit

@pavolloffay pavolloffay Awaiting requested review from pavolloffay

@trask trask Awaiting requested review from trask

@tylerbenson tylerbenson Awaiting requested review from tylerbenson

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mateuszrzeszutek @iNikem