[rursprung]

(full disclosure: AI-generated, reviewed by me)

HttpClientObservationSupport.enable(...) with MetricSet.CONN_POOL only bound the pool gauges. The <prefix>.pool.lease timer and the <prefix>.pool.leases counter were recorded only if the caller wrapped the connection manager explicitly with meteredConnectionManager() or meteredAsyncConnectionManager(). This is not possible when a library creates the connection manager internally and only exposes the builder, for example ApacheHttpClient5TransportBuilder in opensearch-java.

All enable(...) variants (classic, async and caching) now replace the connection manager of the builder with the metered wrapper and bind the pool gauges to it:

• A manager that is already a MeteredConnectionManager or a MeteredAsyncConnectionManager is not wrapped again, so explicit wrapping and repeated enable(...) calls stay safe.
• The connection manager shared flag is not changed.
• Pool gauges are bound only if the original manager is a ConnPoolControl, as before.
• If no connection manager is set on the builder, no pool metrics are installed, as before. The default manager that build() creates is not instrumented.
• If CONN_POOL is disabled, nothing changes.

[arturobernalg]

@rursprung
I am not convinced we should automatically replace the connection manager when CONN_POOL metrics are enabled.

We already provide explicit wrappers for lease instrumentation. Making enable() modify the connection manager introduces additional complexity and depends on configuration order and internal builder APIs. It also does not cover the default connection manager.

I understand the OpenSearch use case, but I would rather keep connection manager instrumentation explicit than introduce this implicit behavior.

There is also a reproducible issue with repeated enable() calls: a non-pooling manager ends up exposing pool gauges because the metered wrapper implements ConnPoolControl.

I would prefer a more explicit approach that addresses the original use case without changing the existing enable() semantics.

[rursprung]

I would prefer a more explicit approach that addresses the original use case without changing the existing enable() semantics.

thanks for your review! do you have something specific in mind for this so that i don't go off in the wrong direction?