Repository navigation
fix(aiohttp-client): create the duration histograms once, not per ClientSession - #5029
bharadwaj-pendyala wants to merge 8 commits into
Conversation
…entSession instrumented_init called create_trace_config for every aiohttp.ClientSession, and that function called get_meter and create_histogram each time. A meter is retained by its provider for the life of the process, so a program that opens sessions in a loop accumulated one meter and one histogram per session and never released them. Move the instrument creation into _instrument so it runs once, and split the per-session TraceConfig assembly into a private _create_trace_config that takes the histograms as arguments. The public create_trace_config keeps its signature, its docstring and its behaviour for callers that build a TraceConfig themselves. This matches how the requests, httpx and aiohttp-server instrumentations already build their instruments at instrument time. Assisted-by: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01TN2giNS6XRUzN2XojZH4zi
Assisted-by: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01TN2giNS6XRUzN2XojZH4zi
Pull request dashboard statusWaiting on reviewers · refreshed 2026-11-11 05:01 UTC Review the latest changes. Status above doesn't look right?
|
| ) -> aiohttp.TraceConfig: | ||
| schema_url = _get_schema_url(sem_conv_opt_in_mode) | ||
|
|
||
| tracer = get_tracer( |
There was a problem hiding this comment.
Should we fix this potential leak as well?
There was a problem hiding this comment.
I checked, and that one doesn't retain. _ProxyMeterProvider.get_meter appends every meter to self._meters and nothing drops them, which is what made the meter grow. ProxyTracerProvider.get_tracer returns a fresh ProxyTracer and keeps no reference, and an SDK TracerProvider dedupes on InstrumentationScope. Measured on this branch with 300 sessions: 300 live ProxyTracer objects, 0 once the sessions close, len(tp._tracers) == 1 under the SDK.
Hoisting it to match requests/httpx does move provider resolution into instrument(). With OTEL_PYTHON_TRACER_PROVIDER=sdk_tracer_provider and set_tracer_provider() called after instrument(), I get 1 exported span today and 0 with the tracer hoisted, plus "Overriding of current TracerProvider is not allowed". requests already behaves that way, so it's defensible on its own, but it's a behaviour change rather than a leak fix.
There was a problem hiding this comment.
I think it's OK if we align the HTTP clients so that they exhibit the same behavior. Feel free to add a note in the changelog.
There was a problem hiding this comment.
Pushed in 5070e10. _instrument resolves the tracer once now and passes it in, and create_trace_config still resolves per call since callers invoke that one themselves. The changelog fragment records that a provider installed after instrument() no longer gets picked up.
CI hasn't run on this PR yet, both pushes are stuck at action_required waiting on workflow approval.
_create_trace_config called get_tracer for every ClientSession, so the provider was resolved per session rather than once. Hoist it next to the histograms in _instrument and pass the tracer in, which is how requests and httpx already do it. This makes instrument() resolve the tracer provider when it is called, so a provider installed after instrument() is no longer picked up. That is the existing requests and httpx behaviour, and the changelog fragment records it. Assisted-by: Claude Opus 5
|
This PR has been automatically marked as stale because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 days of this comment. |
…ter-per-session-sync
…and env-var cases
…e the per-session tests
|
@herin049 the Sep 28 CI runs expired at action_required without running, so I merged main to get fresh ones. Those need a workflow approval too. I also narrowed the changelog note: with the default proxy providers, a provider set after |
|
This PR has been automatically marked as stale because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 days of this comment. |
Description
Fixes #4991.
instrumented_initcalledcreate_trace_config()for everyaiohttp.ClientSession;create_trace_config()callsget_meter()andcreate_histogram(), retaining one meter and histogram per session for the life of the process.Leak reproduction on
a3881e6with the default (proxy) meter provider, opening and closing sessions with no requests:_PROXY_METER_PROVIDER._meters_instrument()now builds histograms once and passes them to private_create_trace_config(). Publiccreate_trace_config()keeps its signature, docstring and behaviour, so direct callers are unaffected. This matchesrequests,httpxandaiohttp-server.Repeated
instrument()/uninstrument()cycles still retain meters: 300 cycles retain 300 meters here and inopentelemetry-instrumentation-requests.get_meter()now runs perinstrument()call rather than per session; this is existing repository behaviour.Type of change
How Has This Been Tested?
test_instrument_creates_one_meter_for_all_sessions: fails onmainwith1 != 5, passes with the fix.pytest tests/test_aiohttp_client_integration.py: 40 passed, up from 39; 36 pre-existing local-environment failures before and after (released core 1.39.0/0.60b0 against this tree's 0.66b0.dev pins).http.client.duration count=1with an SDKMeterProviderandInMemoryMetricReaderthroughAioHttpClientInstrumentorand directcreate_trace_config().ruff checkandruff format --checkclean on the package.Does This PR Require a Core Repo Change?
Checklist:
AI assistance was used for this change (Claude).