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 (bb73657)

[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]

Hi again @krnr , sorry for a bit of scope creep but I think it's important since this will be a breaking change (though a needed one!)

I cc'd you on a very similar PR for the ASGI instrumentor. The change for span name cardinality+semconv will need to be behind an opt-in flag for users. Would it be reasonable to implement something similar here, or in a separate PR? cc @aabmass

[krnr]

(now it's my turn for being sorry to be late) well, as they say "better be safe than sorry". i agree with a flag idea, however, it's beyond my mind to find a good name for that. OTEL_PYTHON_AIOHTTP_SERVER_CANONICAL_PATH_SPAN_NAME ? too verbose?

[tammy-baylis-swi]

There is a shared util at opentelemetry.instrumentation._semconv, which reads OTEL_SEMCONV_STABILITY_OPT_IN for us. This might require a middleware update to accept something like sem_conv_opt_in_mode, like what the AsgiInstrumentor does.

[krnr]

i apologize again for a late response @tammy-baylis-swi . today i checked the semconv module and it seems i misunderstood the concept a bit. the semconv is about the names of attributes (of smth - span, metric, whatever). not about the values of the attributes. and here we're talking about the value of span.name, "should it be 'fooooooooooo' or 'foo...'" and if that must be decided by a feature-flag, that is out of the scope of semconv feature flags in my opinion.

moreover, introducing semconv leads to a lot of changes unrelated to the topic of this PR. i'm happy to continue working on opt-in, but in a separate branch and a separate PR. i've already made a draft commit ( cc882b4 ) in a separate branch to illustrate how much is being changed and - more importantly - how many new discussion points emerge. which in turn will draw the attention from the span name even further.

[tammy-baylis-swi]

Hi @krnr no problem at all! I had some chat with others last week and this PR you've already done will be fine as it is. ✅ We've approved and we'll merge it for an upcoming release.

I'd say don't worry about the opt-in for span name, i.e. don't do what I asked here. It is indeed confusing. The semconv does have Must/Should for span name. You're also right in that the Python implementation so far is focused on the names of attributes.

If you do want to put in a separate PR at some point for names of attributes, indeed it would be a lot of change. You're welcome to work on it if you're interested! Otherwise I'd say this much-needed fix is done.

[tammy-baylis-swi]

Thank you @krnr for this. Lgtm though I want some other Approvers to have a look at this change.

If you wanted to bring this up more synchronously, please feel welcome to join the Python SIG meeting on Zoom on Thursdays -- Calendar with links here. Else this PR should still be visible on the PR digest board.