Add "ConcurrencyAcquireDuration" metric for apache-client #2912
Conversation
The time taken to acquire a new channel from a channel pool can be both non-trivial and highly variable, depending upon whether a new connection needs to be established, and depending upon the overhead of new connection establishment (including TLS handshakes). The same metric was recently added for netty-nio-client: aws#2903
| Instant startGet = null; | ||
| if ("get".equals(method.getName())) { | ||
| startGet = Instant.now(); | ||
| } | ||
| try { | ||
| return method.invoke(orig, args); | ||
| } finally { | ||
| if (startGet != null) { | ||
| recordGetTime(startGet); | ||
| } | ||
| } |
There was a problem hiding this comment.
Why do we need to use a dynamic proxy instead of just implementing the ConnectionRequest interface?
There was a problem hiding this comment.
Good point. I don't see any reason we must use a proxy here since this is an interface and not a final class. I was just taking advantage of the existing infrastructure that was seemingly awaiting this metric to be plugged in. :) But let me modify it to just use a simple delegation scheme.
There was a problem hiding this comment.
Updated to use delegation instead. I think the primary reason to use a proxy is that it would automatically support any new interface methods that are added. I don't know how likely that is to happen with these interfaces.
Bennett-Lynch
changed the title
|
SonarCloud Quality Gate failed. |
…a4bdfc7f6 Pull request: release <- staging/2e29ee54-41b8-4e6f-abe1-e97a4bdfc7f6
The time taken to acquire a new channel from a channel pool can be both non-trivial and highly variable, depending upon whether a new connection needs to be established, and depending upon the overhead of new connection establishment (including TLS handshakes).
The same metric was recently added for netty-nio-client: #2903
Ideally we would avoid the use of
ThreadLocal, but the Apache interfaces make it very difficult to expose a request-levelMetricsCollectorto the connection acquire call. While we can easily wrap aHttpClientConnectionManagerto include metrics, theHttpClientConnectionManager, is declared at the Apache-client-level, meaning we would need to create (not just wrap) a new Apache client instance for every request. Apache offers theHttpContextclass to help propagate arbitrary attributes, but the context object is not exposed to HttpClientConnectionManager#requestConnection or ConnectionRequest#get. To lease & establish new connection, Apache callsrequestConnection/connect. To lease an already-established connection, Apache calls onlyrequestConnection. And onlyconnectis given the context object.License