Support metrics for Apache http components async client pools #1716
Conversation
|
Tests also failing in master |
|
Sorry about the test flake. It should be fixed on master now. Could you rebase these changes on the latest master now and push it so the tests get re-run? |
There was a problem hiding this comment.
Overall it looks good as it is basically the same as the existing non-async binder. That made me wonder though if we couldn't combine the two. I notice that both of the HttpClientConnectionManager implementations implement ConnPoolControl, which defines all of the methods used for the metrics currently. Do you think it'd be too complex to handle both these in the existing MeterBinder?
.../core/instrument/binder/httpcomponents/PoolingNHttpClientConnectionManagerMetricsBinder.java
Outdated
Show resolved
Hide resolved
|
|
||
| @BeforeEach | ||
| void setup() { | ||
| connectionManager = mock(PoolingNHttpClientConnectionManager.class); |
There was a problem hiding this comment.
Would there be any issues instantiating a connection pool for the test? It seems like that would be better than mocking. Though I suppose we would need a wiremock server to make HTTP requests with the HTTP client, and that would add some complexity.
There was a problem hiding this comment.
I would love to add testcontainers and get rid of some of these mocks :/
That's a good point. This stops being http specific though. I like the idea of having a "PoolingHttpClientConnectionManagerMetricsBinder" because it helps to direct users to what they are looking for. For that reason I agree with your suggestion of merging both sync and async into one, but I would not try to make this more generic than that, ie, I would keep this name, even if in practical terms this same implementation could be used for other cases. Or maybe we can have dulplicate implementation for all apache pools, but still keep this one as "syntatic sugar" or a convenience class. |
shakuzen
added
the
release notes
shakuzen
force-pushed
the
httpasyncclient_conn_manager
branch
from
ed6ece2
to
c72bcad
Compare
…mbine both apache httpcomponents binders)
shakuzen
force-pushed
the
httpasyncclient_conn_manager
branch
from
c72bcad
to
0f0d4fb
Compare
shakuzen
changed the title
Same as the regular PoolingHttpClientConnectionManagerMetricsBinder but for the async client.