Use canonical name for aiohttp request span name

[krnr]

Description

Current implementation doesn't use canonical attribute of a Resource, which leads to the situation when generated spans all have unique names. It creates high cardinality of spans. Other frameworks that use path params create spans with "canonical" paths in names (e.g. fastapi [1] )

Type of change

• New feature (non-breaking change which adds functionality)

How Has This Been Tested?

add a new unit test:

• test_url_params_instrumentation

Does This PR Require a Core Repo Change?

• Yes. - Link to PR:
• No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

• Followed the style guidelines of this project
• Changelogs have been updated
• Unit tests have been added
• Documentation has been updated

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: krnr / name: yuri (44ae541, 1d5dfa3, 2b33c49, 3de4702, 9d4b5a1, b304292)

[bisgaard-itis]

Looks good. Thanks a lot for the effort! 👍🏻

[bisgaard-itis]

👍🏻

[bisgaard-itis]

Is match_info.route guaranteed to be not None when match_info is not None?

[krnr]

thank you for the good point. i think this can be the proof: https://github.com/aio-libs/aiohttp/blob/v3.13.1/aiohttp/web_urldispatcher.py#L271

(in fact, even match_info can't be None: https://github.com/aio-libs/aiohttp/blob/v3.13.1/aiohttp/web_request.py#L890 but it may raise issues with type hints)

[tammy-baylis-swi]

I'm not super familiar with aiohttp, but it could we be extra careful and wrap this with a try-except AttributeError? So it's like _get_view_func below.

[krnr]

i am pretty sure that the attributes always exist: https://github.com/aio-libs/aiohttp/blob/v3.5.0/aiohttp/web_request.py#L704 https://github.com/aio-libs/aiohttp/blob/v3.5.0/aiohttp/web_urldispatcher.py#L161 (at least since v3.5.0)

but we definitely can add this "extra-safe" layer. i am just not sure yet what should the name be in case of AttributeError - the default one from request.path or some sentinel because "we-found-smth-unexpected"... having thought about it for a while i decided that the first option is better. let me know if you have any other ideas.

[krnr]

in fact, there's another possibility of "high cardinality abuse": https://github.com/open-telemetry/opentelemetry-python-contrib/pull/3896/files#diff-682c3e961f134b947f243e707c7a75856ea5640e0dfb8f32dad7ffd1b3fa8c9eR145

every non existent name is a span name by itself. so, you can generate an endless amount of 404 and each of them will be a different span. this logic should be (should it?) fixed in a separate PR, because i can't decide right on the spot what shall we do with 404 span names. should they all be "GET <Not Found>" if there's no match_info (i think so, and only get the path from the span attributes), or some users rely on the existing behavior and collect an "inventory" of such calls using span names? i don't know.

[tammy-baylis-swi]

Good questions, hmm. This part of the semconv says

HTTP span names SHOULD be {method} {target} if there is a (low-cardinality) target available. If there is no (low-cardinality) {target} available, HTTP span names SHOULD be {method}.

I'm guessing cardinality of request.path.strip() can depend on the app but does have the potential to be high. What do you think of something like below, where we prefer target (path) as canonical > None? If we stop relying request.path.strip() entirely, maybe this would address the endless-404s concern, though it might be a small breaking change to some users:

    try:
        resource = request.match_info.route.resource
        if resource:
            path = resource.canonical
    except AttributeError:
        path = ""

    if path:
        return f"{request.method} {path}"
    else:
        return f"{request.method}"

[krnr]

yes, i think it will be better. i just modified it a bit to silent a type checker.