From d0ab4eaf878dc0b52d0db73b869706ac857a8d62 Mon Sep 17 00:00:00 2001 From: Riccardo Magliocchetti Date: Fri, 7 Jun 2024 16:17:18 +0200 Subject: [PATCH 1/2] contrib/starlette: fix outcome for 500 responses without exception Set the outcome based on the status code of the http.response.start ASGI message instead of relying only on the raise of an exception. --- elasticapm/contrib/starlette/__init__.py | 1 + tests/contrib/asyncio/starlette_tests.py | 25 ++++++++++++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/elasticapm/contrib/starlette/__init__.py b/elasticapm/contrib/starlette/__init__.py index ad26d7a0a..3dfb225c9 100644 --- a/elasticapm/contrib/starlette/__init__.py +++ b/elasticapm/contrib/starlette/__init__.py @@ -147,6 +147,7 @@ async def wrapped_send(message) -> None: ) result = "HTTP {}xx".format(message["status"] // 100) elasticapm.set_transaction_result(result, override=False) + elasticapm.set_transaction_outcome(http_status_code=message["status"], override=False) await send(message) _mocked_receive = None diff --git a/tests/contrib/asyncio/starlette_tests.py b/tests/contrib/asyncio/starlette_tests.py index fcd7d0dee..e3c4f4a16 100644 --- a/tests/contrib/asyncio/starlette_tests.py +++ b/tests/contrib/asyncio/starlette_tests.py @@ -110,6 +110,10 @@ async def with_slash(request): async def without_slash(request): return PlainTextResponse("Hi {}".format(request.path_params["name"])) + @app.route("/500/", methods=["GET"]) + async def with_500_status_code(request): + return PlainTextResponse("Oops", status_code=500) + @sub.route("/hi") async def hi_from_sub(request): return PlainTextResponse("sub") @@ -236,6 +240,27 @@ def test_exception(app, elasticapm_client): assert error["context"]["request"] == transaction["context"]["request"] +def test_failure_outcome_with_500_status_code(app, elasticapm_client): + client = TestClient(app) + + client.get("/500/") + + assert len(elasticapm_client.events[constants.TRANSACTION]) == 1 + transaction = elasticapm_client.events[constants.TRANSACTION][0] + spans = elasticapm_client.spans_for_transaction(transaction) + assert len(spans) == 0 + + assert transaction["name"] == "GET /500/" + assert transaction["result"] == "HTTP 5xx" + assert transaction["outcome"] == "failure" + assert transaction["type"] == "request" + request = transaction["context"]["request"] + assert request["method"] == "GET" + assert transaction["context"]["response"]["status_code"] == 500 + + assert len(elasticapm_client.events[constants.ERROR]) == 0 + + @pytest.mark.parametrize("header_name", [constants.TRACEPARENT_HEADER_NAME, constants.TRACEPARENT_LEGACY_HEADER_NAME]) def test_traceparent_handling(app, elasticapm_client, header_name): client = TestClient(app) From 3e33d8f1a580e7f3faab6c4094e41e2534a0685f Mon Sep 17 00:00:00 2001 From: Riccardo Magliocchetti Date: Fri, 7 Jun 2024 16:37:01 +0200 Subject: [PATCH 2/2] contrib/asgi: fix outcome for 500 responses without exception Set the outcome based on the status code of the http.response.start ASGI message instead of relying only on the raise of an exception. --- elasticapm/contrib/asgi.py | 1 + tests/contrib/asgi/app.py | 5 +++++ tests/contrib/asgi/asgi_tests.py | 17 +++++++++++++++++ 3 files changed, 23 insertions(+) diff --git a/elasticapm/contrib/asgi.py b/elasticapm/contrib/asgi.py index 096fed36a..92ee6c193 100644 --- a/elasticapm/contrib/asgi.py +++ b/elasticapm/contrib/asgi.py @@ -50,6 +50,7 @@ async def wrapped_send(message) -> None: await set_context(lambda: middleware.get_data_from_response(message, constants.TRANSACTION), "response") result = "HTTP {}xx".format(message["status"] // 100) elasticapm.set_transaction_result(result, override=False) + elasticapm.set_transaction_outcome(http_status_code=message["status"], override=False) await send(message) return wrapped_send diff --git a/tests/contrib/asgi/app.py b/tests/contrib/asgi/app.py index a919b2cef..352720135 100644 --- a/tests/contrib/asgi/app.py +++ b/tests/contrib/asgi/app.py @@ -59,3 +59,8 @@ async def boom() -> None: @app.route("/body") async def json(): return jsonify({"hello": "world"}) + + +@app.route("/500", methods=["GET"]) +async def error(): + return "KO", 500 diff --git a/tests/contrib/asgi/asgi_tests.py b/tests/contrib/asgi/asgi_tests.py index 824a23b68..f2a096dcc 100644 --- a/tests/contrib/asgi/asgi_tests.py +++ b/tests/contrib/asgi/asgi_tests.py @@ -66,6 +66,23 @@ async def test_transaction_span(instrumented_app, elasticapm_client): assert span["sync"] == False +@pytest.mark.asyncio +async def test_transaction_span(instrumented_app, elasticapm_client): + async with async_asgi_testclient.TestClient(instrumented_app) as client: + resp = await client.get("/500") + assert resp.status_code == 500 + assert resp.text == "KO" + + assert len(elasticapm_client.events[constants.TRANSACTION]) == 1 + assert len(elasticapm_client.events[constants.SPAN]) == 0 + transaction = elasticapm_client.events[constants.TRANSACTION][0] + assert transaction["name"] == "GET unknown route" + assert transaction["result"] == "HTTP 5xx" + assert transaction["outcome"] == "failure" + assert transaction["context"]["request"]["url"]["full"] == "/500" + assert transaction["context"]["response"]["status_code"] == 500 + + @pytest.mark.asyncio async def test_transaction_ignore_url(instrumented_app, elasticapm_client): elasticapm_client.config.update("1", transaction_ignore_urls="/foo*")