Skip to content

Commit 4e34678

Browse files
ZStriker19gnufede
authored andcommitted
fix(tracing): add flag for aiohttp that disables potential for memory leak (#12081)
This PR adds the environment variable ``DD_AIOHTTP_CLIENT_DISABLE_STREAM_TIMING_FOR_MEM_LEAK`` to address a potential memory leak in the aiohttp integration. When set to true, this flag may cause streamed response span timing to be inaccurate. The flag defaults to false. When this was merged #7551 we essentially added support for getting the correct timing of streamed responses, however it also caused us to sometimes never close spans, which lead to a memory leak. When set to true, this flag essentially reverts that behavior. ## Checklist - [x] PR author has checked that all the criteria below are met - The PR description includes an overview of the change - The PR description articulates the motivation for the change - The change includes tests OR the PR description describes a testing strategy - The PR description notes risks associated with the change, if any - Newly-added code is easy to change - The change follows the [library release note guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) - The change includes or references documentation updates if necessary - Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) ## Reviewer Checklist - [x] Reviewer has checked that all the criteria below are met - Title is accurate - All changes are related to the pull request's stated goal - Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes - Testing strategy adequately addresses listed risks - Newly-added code is easy to change - Release note makes sense to a user of the library - If necessary, author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment - Backport labels are set in a manner that is consistent with the [release branch maintenance policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)
1 parent 11fc09d commit 4e34678

File tree

5 files changed

+56
-6
lines changed

5 files changed

+56
-6
lines changed

ddtrace/contrib/aiohttp.py

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,13 @@
3636
3737
Default: ``False``
3838
39+
.. py:data:: ddtrace.config.aiohttp['disable_stream_timing_for_mem_leak']
40+
41+
Whether or not to to address a potential memory leak in the aiohttp integration.
42+
When set to ``True``, this flag may cause streamed response span timing to be inaccurate.
43+
44+
Default: ``False``
45+
3946
4047
Server
4148
******

ddtrace/contrib/internal/aiohttp/middlewares.py

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -59,8 +59,9 @@ async def attach_context(request):
5959
request[REQUEST_CONFIG_KEY] = app[CONFIG_KEY]
6060
try:
6161
response = await handler(request)
62-
if isinstance(response, web.StreamResponse):
63-
request.task.add_done_callback(lambda _: finish_request_span(request, response))
62+
if not config.aiohttp["disable_stream_timing_for_mem_leak"]:
63+
if isinstance(response, web.StreamResponse):
64+
request.task.add_done_callback(lambda _: finish_request_span(request, response))
6465
return response
6566
except Exception:
6667
req_span.set_traceback()
@@ -134,9 +135,13 @@ async def on_prepare(request, response):
134135
the trace middleware execution.
135136
"""
136137
# NB isinstance is not appropriate here because StreamResponse is a parent of the other
137-
# aiohttp response types
138-
if type(response) is web.StreamResponse and not response.task.done():
139-
return
138+
# aiohttp response types. However in some cases this can also lead to missing the closing of
139+
# spans, leading to a memory leak, which is why we have this flag.
140+
# todo: this is a temporary fix for a memory leak in aiohttp. We should find a way to
141+
# consistently close spans with the correct timing.
142+
if not config.aiohttp["disable_stream_timing_for_mem_leak"]:
143+
if type(response) is web.StreamResponse and not response.task.done():
144+
return
140145
finish_request_span(request, response)
141146

142147

ddtrace/contrib/internal/aiohttp/patch.py

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
from ddtrace.internal.utils import get_argument_value
2323
from ddtrace.internal.utils.formats import asbool
2424
from ddtrace.propagation.http import HTTPPropagator
25+
from ddtrace.settings._core import get_config as _get_config
2526
from ddtrace.trace import Pin
2627

2728

@@ -31,7 +32,12 @@
3132
# Server config
3233
config._add(
3334
"aiohttp",
34-
dict(distributed_tracing=True),
35+
dict(
36+
distributed_tracing=True,
37+
disable_stream_timing_for_mem_leak=asbool(
38+
_get_config("DD_AIOHTTP_CLIENT_DISABLE_STREAM_TIMING_FOR_MEM_LEAK", default=False)
39+
),
40+
),
3541
)
3642

3743
config._add(
Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
3+
fixes:
4+
- |
5+
aiohttp: Adds the environment variable ``DD_AIOHTTP_CLIENT_DISABLE_STREAM_TIMING_FOR_MEM_LEAK`` to address a potential memory leak in the aiohttp integration. When set to true, this flag may cause streamed response span timing to be inaccurate. The flag defaults to false.

tests/contrib/aiohttp/test_request.py

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,33 @@ async def test_full_request(patched_app_tracer, aiohttp_client, loop):
3131
assert "GET /" == request_span.resource
3232

3333

34+
async def test_full_request_w_mem_leak_prevention_flag(patched_app_tracer, aiohttp_client, loop):
35+
config.aiohttp.disable_stream_timing_for_mem_leak = True
36+
try:
37+
app, tracer = patched_app_tracer
38+
client = await aiohttp_client(app)
39+
# it should create a root span when there is a handler hit
40+
# with the proper tags
41+
request = await client.request("GET", "/")
42+
assert 200 == request.status
43+
await request.text()
44+
# the trace is created
45+
traces = tracer.pop_traces()
46+
assert 1 == len(traces)
47+
assert 1 == len(traces[0])
48+
request_span = traces[0][0]
49+
assert_is_measured(request_span)
50+
51+
# request
52+
assert "aiohttp-web" == request_span.service
53+
assert "aiohttp.request" == request_span.name
54+
assert "GET /" == request_span.resource
55+
except Exception:
56+
raise
57+
finally:
58+
config.aiohttp.disable_stream_timing_for_mem_leak = False
59+
60+
3461
async def test_stream_request(patched_app_tracer, aiohttp_client, loop):
3562
app, tracer = patched_app_tracer
3663
async with await aiohttp_client(app) as client:

0 commit comments

Comments
 (0)