- 
                Notifications
    You must be signed in to change notification settings 
- Fork 803
Use canonical name for aiohttp request span name #3896
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | 
|---|---|---|
|  | @@ -123,6 +123,10 @@ def get_default_span_details(request: web.Request) -> Tuple[str, dict]: | |
| a tuple of the span name, and any attributes to attach to the span. | ||
| """ | ||
| span_name = request.path.strip() or f"HTTP {request.method}" | ||
| if request.match_info and request.match_info.route.resource: | ||
| resource = request.match_info.route.resource | ||
| if resource.canonical: | ||
| span_name = resource.canonical | ||
| There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for working on this! It will definitely be better to have lower cardinality span names. Could this be adjusted to make sure the new functionality adheres to the semconv for http server span names? This currently sets span_name as  | ||
| return span_name, {} | ||
|  | ||
|  | ||
|  | ||
| Original file line number | Diff line number | Diff line change | 
|---|---|---|
|  | @@ -27,6 +27,7 @@ | |
| from opentelemetry.semconv._incubating.attributes.http_attributes import ( | ||
| HTTP_METHOD, | ||
| HTTP_STATUS_CODE, | ||
| HTTP_TARGET, | ||
| HTTP_URL, | ||
| ) | ||
| from opentelemetry.test.globals_test import reset_trace_globals | ||
|  | @@ -81,7 +82,15 @@ async def fixture_server_fixture(tracer, aiohttp_server, suppress): | |
| AioHttpServerInstrumentor().instrument() | ||
|  | ||
| app = aiohttp.web.Application() | ||
| app.add_routes([aiohttp.web.get("/test-path", default_handler)]) | ||
| app.add_routes( | ||
| [ | ||
| aiohttp.web.get("/test-path", default_handler), | ||
| aiohttp.web.get("/test-path/{url_param}", default_handler), | ||
| aiohttp.web.get( | ||
| "/object/{object_id}/action/{another_param}", default_handler | ||
| ), | ||
| ] | ||
| ) | ||
| if suppress: | ||
| with suppress_http_instrumentation(): | ||
| server = await aiohttp_server(app) | ||
|  | @@ -139,6 +148,53 @@ async def test_status_code_instrumentation( | |
| ) | ||
|  | ||
|  | ||
| @pytest.mark.asyncio | ||
| @pytest.mark.parametrize( | ||
| "url, example_paths", | ||
| [ | ||
| ( | ||
| "/test-path/{url_param}", | ||
| ( | ||
| "/test-path/foo", | ||
| "/test-path/bar", | ||
| ), | ||
| ), | ||
| ( | ||
| "/object/{object_id}/action/{another_param}", | ||
| ( | ||
| "/object/1/action/bar", | ||
| "/object/234/action/baz", | ||
| ), | ||
| ), | ||
| ], | ||
| ) | ||
| async def test_url_params_instrumentation( | ||
| tracer, | ||
| server_fixture, | ||
| aiohttp_client, | ||
| url, | ||
| example_paths, | ||
| ): | ||
| _, memory_exporter = tracer | ||
| server, _ = server_fixture | ||
|  | ||
| assert len(memory_exporter.get_finished_spans()) == 0 | ||
|  | ||
| client = await aiohttp_client(server) | ||
| for path in example_paths: | ||
| await client.get(path) | ||
|  | ||
| assert len(memory_exporter.get_finished_spans()) == 2 | ||
|  | ||
| for request_path, span in zip( | ||
| example_paths, memory_exporter.get_finished_spans() | ||
| ): | ||
| assert url == span.name | ||
| There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍🏻 | ||
| assert request_path == span.attributes[HTTP_TARGET] | ||
| full_url = f"http://{server.host}:{server.port}{request_path}" | ||
| assert full_url == span.attributes[HTTP_URL] | ||
|  | ||
|  | ||
| @pytest.mark.asyncio | ||
| @pytest.mark.parametrize("suppress", [True]) | ||
| async def test_suppress_instrumentation( | ||
|  | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is
match_info.routeguaranteed to be notNonewhenmatch_infois notNone?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you for the good point. i think this can be the proof: https://github.com/aio-libs/aiohttp/blob/master/aiohttp/web_urldispatcher.py#L231
(in fact, even
match_infocan't beNone: https://github.com/aio-libs/aiohttp/blob/v3.13.1/aiohttp/web_request.py#L890 but it may raise issues with type hints)