-
Notifications
You must be signed in to change notification settings - Fork 910
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.
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
Collect timings related with TLS handshake #5647
Conversation
fdcf3ae
to
a751fa5
Compare
a751fa5
to
1c8aa22
Compare
core/src/main/java/com/linecorp/armeria/client/HttpClientPipelineConfigurator.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/client/HttpClientPipelineConfigurator.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall! Left some minor comments! 👍
core/src/main/java/com/linecorp/armeria/client/HttpClientPipelineConfigurator.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/client/HttpChannelPool.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/client/HttpChannelPool.java
Outdated
Show resolved
Hide resolved
core/src/test/java/com/linecorp/armeria/client/TlsHandshakeTimingTest.java
Outdated
Show resolved
Hide resolved
Additionally, shouldn't the check condition at the bottom be
|
Oops, I think so. 😓 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks almost done! Left some minor comments 👍
core/src/main/java/com/linecorp/armeria/client/HttpClientPipelineConfigurator.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/client/HttpClientPipelineConfigurator.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/common/logging/ClientConnectionTimingsBuilder.java
Outdated
Show resolved
Hide resolved
If find a bug while working on a different �issue like this, should I make and work on a separate issue ticket or fix it together in this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgot to mention this, but it is probably more useful if users can view this timing by default when they add a MetricCollectingClient
.
e.g.
armeria/core/src/main/java/com/linecorp/armeria/internal/common/metric/RequestMetricSupport.java
Lines 118 to 134 in 0d5b520
if (timings != null) { | |
metrics.connectionAcquisitionDuration().record(timings.connectionAcquisitionDurationNanos(), | |
TimeUnit.NANOSECONDS); | |
final long dnsResolutionDurationNanos = timings.dnsResolutionDurationNanos(); | |
if (dnsResolutionDurationNanos >= 0) { | |
metrics.dnsResolutionDuration().record(dnsResolutionDurationNanos, TimeUnit.NANOSECONDS); | |
} | |
final long socketConnectDurationNanos = timings.socketConnectDurationNanos(); | |
if (socketConnectDurationNanos >= 0) { | |
metrics.socketConnectDuration().record(socketConnectDurationNanos, TimeUnit.NANOSECONDS); | |
} | |
final long pendingAcquisitionDurationNanos = timings.pendingAcquisitionDurationNanos(); | |
if (pendingAcquisitionDurationNanos >= 0) { | |
metrics.pendingAcquisitionDuration().record(pendingAcquisitionDurationNanos, | |
TimeUnit.NANOSECONDS); | |
} | |
} |
It's your choice, but I prefer that it be handled separately |
Okay, I will make new issue and new PR!! |
3a3652c
to
e2027e2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really great!
Thanks, @Leewongi0731! 👍 👍 👍
core/src/main/java/com/linecorp/armeria/common/logging/ClientConnectionTimings.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nicely done. Thanks @Leewongi0731 🙇 👍 🙇
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job, @Leewongi0731! Let's merge once @minwoox's comment about deduplication is addressed.
d128348
to
43eca15
Compare
Great job, @Leewongi0731! 😊 |
Motivation: Collect the timings related with TLS handshake. If a request was the first in a connection, armeria could also provide it in a RequestLog to tell a user that the request timing has been affected by TLS handshake. Modifications: - Add TLS handshake related field in `ClientConnectionTimings` - Add TLS handshake duration metric field at MetricCollectingClient - Start collecting the TLS handshake timer in the case below. - ~~If the client is enabled as `TCP fast open` in the first request, start the timer before the TCP connection.~~ - start the timer when netty calls `SslHandler.channelActive()` Result: - Closes line#3647
Motivation:
Collect the timings related with TLS handshake. If a request was the first in a connection, armeria could also provide it in a RequestLog to tell a user that the request timing has been affected by TLS handshake.
Modifications:
ClientConnectionTimings
If the client is enabled asTCP fast open
in the first request, start the timer before the TCP connection.SslHandler.channelActive()
Result: