Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(asgi, starlette, fastapi): exclude background task durations from web requests #3799

Merged
9 changes: 8 additions & 1 deletion ddtrace/contrib/asgi/middleware.py
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,13 @@ async def wrapped_send(message):
span, self.integration_config, status_code=status_code, response_headers=response_headers
)

return await send(message)
try:
return await send(message)
finally:
# Per asgi spec, "more_data" is used if there is still data to send
wantsui marked this conversation as resolved.
Show resolved Hide resolved
# Close the span if "http.response.body" has no more data left to send in the response.
if message.get("type") == "http.response.body" and not message.get("more_body", False):
span.finish()

try:
return await self.app(scope, receive, wrapped_send)
Expand All @@ -183,4 +189,5 @@ async def wrapped_send(message):
del scope["datadog"]["request_span"]
except KeyError:
pass
brettlangdon marked this conversation as resolved.
Show resolved Hide resolved

ZStriker19 marked this conversation as resolved.
Show resolved Hide resolved
span.finish()
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
fixes:
- |
asgi, starlette, fastapi: Exclude background tasks duration from web request spans.
35 changes: 35 additions & 0 deletions tests/contrib/asgi/test_asgi.py
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,18 @@ def double_callable_app(scope):
return partial(basic_app, scope)


async def tasks_app(scope, receive, send):
"""
An app that does something in the background after the response is sent back.
"""
assert scope["type"] == "http"
message = await receive()
if message.get("type") == "http.request":
await send({"type": "http.response.start", "status": 200, "headers": [[b"Content-Type", b"text/plain"]]})
await send({"type": "http.response.body", "body": b"*"})
brettlangdon marked this conversation as resolved.
Show resolved Hide resolved
await asyncio.sleep(1)


def _check_span_tags(scope, span):
assert span.get_tag("http.method") == scope["method"]
server = scope.get("server")
Expand Down Expand Up @@ -445,3 +457,26 @@ async def test_app_no_middleware(scope, receive, send):
async with httpx.AsyncClient(app=test_app_no_middleware) as client:
response = await client.get("http://testserver/")
assert response.status_code == 200


@pytest.mark.asyncio
async def test_tasks_asgi(scope, tracer, test_spans):
"""
Tests that if someonething happens in the background, the asgi span only captures the
time it took for a user to get a response, not the time it took for other tasks
in the background.
"""
app = TraceMiddleware(tasks_app, tracer=tracer)
async with httpx.AsyncClient(app=app) as client:
response = await client.get("http://testserver/")
assert response.status_code == 200

spans = test_spans.pop_traces()
assert len(spans) == 1
assert len(spans[0]) == 1
request_span = spans[0][0]
assert request_span.name == "asgi.request"
assert request_span.span_type == "web"
# typical duration without background task should be in less than 10 ms
# duration with background task will take approximately 1.1s
assert request_span.duration < 1
10 changes: 10 additions & 0 deletions tests/contrib/fastapi/app.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
import asyncio
from tempfile import NamedTemporaryFile
import time
from typing import Optional

from fastapi import BackgroundTasks
from fastapi import FastAPI
from fastapi import HTTPException
from fastapi import Header
Expand Down Expand Up @@ -102,6 +104,14 @@ async def file():
fp.flush()
return FileResponse(fp.name)

async def custom_task():
await asyncio.sleep(1)

@app.get("/asynctask")
async def asynctask(bg_tasks: BackgroundTasks):
bg_tasks.add_task(custom_task)
return "task added"

subapp = FastAPI()

@subapp.get("/hello/{name}")
Expand Down
17 changes: 17 additions & 0 deletions tests/contrib/fastapi/test_fastapi.py
Original file line number Diff line number Diff line change
Expand Up @@ -552,3 +552,20 @@ def test_table_query_snapshot(snapshot_client):
"name": "Test Name",
"description": "This request adds a new entry to the test db",
}


def test_background_task(client, tracer, test_spans):
"""Tests if background tasks have been excluded from span duration"""
response = client.get("/asynctask")
assert response.status_code == 200
assert response.json() == "task added"
spans = test_spans.pop_traces()
assert len(spans) == 1
assert len(spans[0]) == 2
request_span, serialize_span = spans[0]

assert request_span.name == "fastapi.request"
assert request_span.resource == "GET /asynctask"
# typical duration without background task should be in less than 10 ms
# duration with background task will take approximately 1.1s
assert request_span.duration < 1
12 changes: 12 additions & 0 deletions tests/contrib/starlette/app.py
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
import asyncio
from tempfile import NamedTemporaryFile
import time

import databases
import sqlalchemy
from starlette.applications import Starlette
from starlette.background import BackgroundTask
from starlette.responses import FileResponse
from starlette.responses import JSONResponse
from starlette.responses import PlainTextResponse
from starlette.responses import Response
from starlette.responses import StreamingResponse
Expand Down Expand Up @@ -106,6 +109,14 @@ async def add_note(request):
response = "Success"
return PlainTextResponse(response)

async def custom_task():
await asyncio.sleep(1)

async def background_task(request):
"""An example endpoint that just adds to background tasks"""
jsonmsg = {"result": "Background task added"}
return JSONResponse(jsonmsg, background=BackgroundTask(custom_task))

routes = [
Route("/", endpoint=homepage, name="homepage", methods=["GET"]),
Route("/200", endpoint=success, name="200", methods=["GET"]),
Expand All @@ -119,6 +130,7 @@ async def add_note(request):
Route("/notes", endpoint=list_notes, methods=["GET"]),
Route("/notes", endpoint=add_note, methods=["POST"]),
Mount("/sub-app", Starlette(routes=[Route("/hello/{name}", endpoint=success, name="200", methods=["GET"])])),
Route("/backgroundtask", endpoint=background_task, name="200", methods=["GET"]),
]

app = Starlette(routes=routes, on_startup=[database.connect], on_shutdown=[database.disconnect])
Expand Down
14 changes: 14 additions & 0 deletions tests/contrib/starlette/test_starlette.py
Original file line number Diff line number Diff line change
Expand Up @@ -450,3 +450,17 @@ def test_incorrect_patching(run_python_code_in_subprocess):
assert status == 0, err
assert out == b"", err
assert err == b"datadog context not present in ASGI request scope, trace middleware may be missing\n"


def test_background_task(client, tracer, test_spans):
"""Tests if background tasks have been excluded from span duration"""
r = client.get("/backgroundtask")
assert r.status_code == 200
assert r.text == '{"result":"Background task added"}'

request_span = next(test_spans.filter_spans(name="starlette.request"))
assert request_span.name == "starlette.request"
assert request_span.resource == "GET /backgroundtask"
# typical duration without background task should be in less than 10ms
# duration with background task will take approximately 1.1s
assert request_span.duration < 1