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 transaction names for starlette Mount routes #1037

Merged
merged 9 commits into from
Feb 19, 2021
1 change: 1 addition & 0 deletions CHANGELOG.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ endif::[]
* Fix for potential `capture_body: error` hang in Starlette/FastAPI {pull}1038[#1038]
* Fix a rare error around processing stack frames {pull}1012[#1012]
* Fix for Starlette/FastAPI to correctly capture request bodies as strings {pull}1042[#1041]
* Fix transaction names for Starlette Mount routes {pull}1037[#1037]


[[release-notes-6.x]]
Expand Down
35 changes: 21 additions & 14 deletions elasticapm/contrib/starlette/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
from starlette.middleware.base import BaseHTTPMiddleware, RequestResponseEndpoint
from starlette.requests import Request
from starlette.responses import Response
from starlette.routing import Match
from starlette.routing import Match, Mount
from starlette.types import ASGIApp

import elasticapm
Expand Down Expand Up @@ -207,18 +207,11 @@ async def _request_finished(self, response: Response):
elasticapm.set_transaction_result(result, override=False)

def get_route_name(self, request: Request) -> str:
route_name = None
app = request.app
scope = request.scope
routes = app.routes
route_name = self._get_route_name(scope, routes)

for route in routes:
match, _ = route.matches(scope)
if match == Match.FULL:
route_name = route.path
break
elif match == Match.PARTIAL and route_name is None:
route_name = route.path
# Starlette magically redirects requests if the path matches a route name with a trailing slash
# appended or removed. To not spam the transaction names list, we do the same here and put these
# redirects all in the same "redirect trailing slashes" transaction name
Expand All @@ -230,9 +223,23 @@ def get_route_name(self, request: Request) -> str:
else:
redirect_scope["path"] = scope["path"] + "/"
trim = False
for route in routes:
match, _ = route.matches(redirect_scope)
if match != Match.NONE:
route_name = route.path + "/" if trim else route.path[:-1]
break

route_name = self._get_route_name(redirect_scope, routes)
route_name = route_name + "/" if trim else route_name[:-1]
return route_name

def _get_route_name(self, scope, routes, route_name=None):
for route in routes:
match, child_scope = route.matches(scope)
if match == Match.FULL:
route_name = route.path
child_scope = {**scope, **child_scope}
if isinstance(route, Mount):
child_route_name = self._get_route_name(child_scope, route.routes, route_name)
if child_route_name is None:
route_name = None
else:
route_name += child_route_name
return route_name
elif match == Match.PARTIAL and route_name is None:
route_name = route.path
36 changes: 36 additions & 0 deletions tests/contrib/asyncio/starlette_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,10 @@
@pytest.fixture
def app(elasticapm_client):
app = Starlette()
sub = Starlette()
subsub = Starlette()
app.mount("/sub", sub)
sub.mount("/subsub", subsub)

@app.route("/", methods=["GET", "POST"])
async def hi(request):
Expand Down Expand Up @@ -86,6 +90,14 @@ async def with_slash(request):
async def without_slash(request):
return PlainTextResponse("Hi {}".format(request.path_params["name"]))

@sub.route("/hi")
async def hi_from_sub(request):
return PlainTextResponse("sub")

@subsub.route("/hihi/{name}")
async def hi_from_sub(request):
return PlainTextResponse(request.path_params["name"])

app.add_middleware(ElasticAPM, client=elasticapm_client)

yield app
Expand Down Expand Up @@ -270,6 +282,7 @@ def test_transaction_name_is_route(app, elasticapm_client):
(
("/hi/shay/with/slash", "GET /hi/{name}/with/slash"),
("/hi/shay/without/slash/", "GET /hi/{name}/without/slash/"),
("/sub/subsub/hihi/shay/", "GET /sub/subsub/hihi/{name}/"),
),
)
def test_trailing_slash_redirect_detection(app, elasticapm_client, url, expected):
Expand All @@ -294,6 +307,29 @@ def test_enabled_instrumentation(app, elasticapm_client):
assert not isinstance(urllib3.connectionpool.HTTPConnectionPool.urlopen, wrapt.BoundFunctionWrapper)


def test_transaction_name_is_route_for_mounts(app, elasticapm_client):
"""
Tests if recursive URL matching works when apps are mounted in other apps
"""
client = TestClient(app)
response = client.get("/sub/hi")
assert response.status_code == 200

assert len(elasticapm_client.events[constants.TRANSACTION]) == 1
transaction = elasticapm_client.events[constants.TRANSACTION][0]
assert transaction["name"] == "GET /sub/hi"
assert transaction["context"]["request"]["url"]["pathname"] == "/sub/hi"

response = client.get("/sub/subsub/hihi/shay")
assert response.status_code == 200

assert len(elasticapm_client.events[constants.TRANSACTION]) == 2
transaction = elasticapm_client.events[constants.TRANSACTION][1]
assert transaction["name"] == "GET /sub/subsub/hihi/{name}"
assert transaction["context"]["request"]["url"]["pathname"] == "/sub/subsub/hihi/shay"



@pytest.mark.parametrize(
"elasticapm_client",
[
Expand Down