Apache HttpClient per route connection metrics #2031
Conversation
…depend on connection manager
|
@mihanjk First of all, thanks for all the work that went into this. Seems like maybe your first major Github PR to an OSS project? Looks like you have a bright future. Give me a bit to examine this closer, please. |
|
@jkschneider Thanks. I opened pull request to open source project for the first time. |
| public void bindTo(@NonNull MeterRegistry registry) { | ||
| super.bindTo(registry); | ||
| updateRoutesMetrics(registry); | ||
| } | ||
|
|
||
| /** | ||
| * Adds metrics for newly created {@link HttpRoute} to the given meter registry. | ||
| */ | ||
| public void updateRoutesMetrics(MeterRegistry registry) { | ||
| for (HttpRoute route : getRoutes(connectionManager)) { |
There was a problem hiding this comment.
👋 I come across this PR trying to resolve #1616 as I have similar requirements as well. Great initiative!
However, I wonder how this piece will work as the gauge per route is registered at "init" time and won't be updated going forward. If I understand the problem correctly, we need to add a gauge on a route created event but it's buried deep into the apache httpclient implementation...
There was a problem hiding this comment.
Hi, Alan! Your comprehension of the problem is absolutely correct. Thanks for having a look 👍
I didn't find out another way of exposing the routes by using contract provided byPoolingHttpClientConnectionManager class. So I've decided to move the responsibility of scheduling exposed method updateRoutesMetrics to the client side. That's not obvious, so I need to expand docs at least.
Potentially the problem can be solved by registering another gauge for that case. I'm not sure if it's a good idea because each call of getRoutes method cause acquiring the lock and creation of another HashSet object.
|
A year has passed. Is there any update for this feature? Thanks。 |
|
@mYakimchenko Please sign the Contributor License Agreement! Click here to manually synchronize the status of this Pull Request. See the FAQ for frequently asked questions. |
This comment was marked as resolved.
This comment was marked as resolved.
| private void registerRouteMetrics(String host, PoolStats poolStats, MeterRegistry registry) { | ||
| Gauge.builder("httpcomponents.httpclient.pool.route.max", | ||
| poolStats, | ||
| PoolStats::getMax) |
There was a problem hiding this comment.
I know this is year old, but i've come across this trying to implement it in my app.
The information below applies at least to PoolingHttpClientConnectionManager.
You can't use/reuse the PoolStats object here, because the object is created when connectionManager.getStats(route); is called and the primitive values are filled into it. It is an immutable object, so your statistics will permanently display the state they were in the moment registerRouteMetrics was first called for the host.
I've worked around this by passing the HttpRoute object into the registerRouteMetrics and calling getStats in the Gauge lambda. This seems to work correctly even if the connection pool completely empties and the Route is no longer returned in getStats. However it will need further testing to see if it works correctly in all cases.
|
Sorry this PR has sat for so long without review from the team. Is there interest in updating it - that is, rebasing on the latest changes, which include an upgrade to Apache HttpComponents 5? I wonder if any of the review comments may be addressed by new features available in the latest Apache HC 5. |
This PR adds per route connection metrics according to #1616
I'm not sure if it's good decision to introduce abstraction over
ConnPoolControlas I found only one usage across Apache HttpComponents ofHttpRouteas generic type. It will break compatibility as well 🤔