Skip to content

fix(instrumentation-asgi): remove high cardinal path from span name #2650

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.

Sign up for GitHub

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

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
+153 −48
Open

fix(instrumentation-asgi): remove high cardinal path from span name #2650

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
076144c
fix(instrumentation-asgi): remove high cardinal path from span name
srikanthccv Jul 2, 2024
251b3c1
resolve conflicts
srikanthccv Jul 26, 2024
5de29db
fix asgi instrumentation
srikanthccv Jul 26, 2024
2da1dac
Merge branch 'main' into fix-asgi-name
srikanthccv Aug 31, 2024
3ed3922
Fix merge conflicts
srikanthccv Mar 8, 2025
1c1b748
Fix name and update tests
srikanthccv Mar 8, 2025
3f5758e
Merge branch 'fix-asgi-name' of github.com:srikanthccv/opentelemetry-…
srikanthccv Mar 8, 2025
99923c2
Format and CHANGELOG
srikanthccv Mar 9, 2025
498527f
Merge branch 'main' into fix-asgi-name
srikanthccv Nov 8, 2025
95b0c48
chore: fix test
srikanthccv Nov 8, 2025
f98022e
chore: update CHANGELOG.md
srikanthccv Nov 8, 2025
e2ff03e
chore: fix format
srikanthccv Nov 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
([#3322](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/3322))
- `opentelemetry-instrumentation-requests` always record span status code in duration metric
([#3323](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/3323))
- `opentelemetry-instrumentation-asgi` remove high cardinal path from span name
([#2650](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2650))

## Version 1.30.0/0.51b0 (2025-02-03)

Expand Down
19 changes: 8 additions & 11 deletions ...ion/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py
View file
Open in desktop
Copy link
Member

@aabmass aabmass Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to see you @srikanthccv 👋

Is the span name being modified here supposed to follow the OTel HTTP server semconv or is it a more internal span? This change would be a breakage for anyone relying on this particular name, but if it's not the main {method} {target} span it may be OK.

Copy link
Member Author

@srikanthccv srikanthccv Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the span name being modified here supposed to follow the OTel HTTP server semconv or is it a more internal span?

Correct, specifically this part of the spec: If there is no (low-cardinality) {target} available, HTTP span names SHOULD be {method}. The path value of the ASGI scope for both HTTP and WebSocket contains the non-templatized path. The change in this PR removes the path as the target and just sets the method as recommended.

Some examples image

This change would be a breakage for anyone relying on this particular name

Agreed, but at the same time, it doesn't follow the semantic convention and spec guidelines for span names. I would say it's a necessary breaking change. Even from the end-user perspective, I believe this is a useful change; otherwise, IDs (from messaging-adjacent apps, in my experience) become part of the name, leading to no meaningful aggregation for server spans. In many deployments, the spanmetrics connector is used to derive metrics from these spans, which creates a problem.

Copy link
Member

@aabmass aabmass Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, but at the same time, it doesn't follow the semantic convention and spec guidelines for span names. I would say it's a necessary breaking change. Even from the end-user perspective, I believe this is a useful change

Ya I agree, maybe we can just gate this behind the new semconv stability opt in like we do here?

opentelemetry-python-contrib/instrumentation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py

Lines 741 to 743 in bd3c1f2

attributes = collect_request_attributes(
scope, self._sem_conv_opt_in_mode
)

How hard would that be to do?

Copy link
Member Author

@srikanthccv srikanthccv Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me take a look at it.

Copy link
Member Author

@srikanthccv srikanthccv Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went through the code. It should be possible to keep the old and new name following the opt in mode. Let me know if we should make it opt-in.

Copy link
Contributor

@tammy-baylis-swi tammy-baylis-swi Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi! 👋

There is a similar PR open for the aiohttp server instrumentation that will also introduce a breaking change to span name in order to follow semconv and reduce cardinality: #3896 (comment) . @aabmass should that PR also include an opt-in? cc @krnr

Copy link
Member

@aabmass aabmass Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO it would be better since the purpose of the opt-in was to prevent breaking people

Any objections to that though? Does it work for you @srikanthccv ?

Copy link
Member Author

@srikanthccv srikanthccv Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me 👍

Original file line number Diff line number Diff line change
Expand Up @@ -471,15 +471,12 @@ def get_default_span_details(scope: dict) -> Tuple[str, dict]:
Returns:
a tuple of the span name, and any attributes to attach to the span.
"""
path = scope.get("path", "").strip()
method = sanitize_method(scope.get("method", "").strip())
if method == "_OTHER":
method = "HTTP"
if method and path: # http
return f"{method} {path}", {}
if path: # websocket
return path, {}
return method, {} # http with no path
if scope.get("type") == "http":
method = sanitize_method(scope.get("method", "").strip())
if method == "_OTHER":
method = "HTTP"
return method, {}
return scope.get("type", ""), {}


def _collect_target_attribute(
Expand Down Expand Up @@ -811,7 +808,7 @@ def _get_otel_receive(self, server_span_name, scope, receive):
@wraps(receive)
async def otel_receive():
with self.tracer.start_as_current_span(
" ".join((server_span_name, scope["type"], "receive"))
" ".join((server_span_name, "receive"))
) as receive_span:
message = await receive()
if callable(self.client_request_hook):
Expand Down Expand Up @@ -842,7 +839,7 @@ def _set_send_span(
):
"""Set send span attributes and status code."""
with self.tracer.start_as_current_span(
" ".join((server_span_name, scope["type"], "send"))
" ".join((server_span_name, "send"))
) as send_span:
if callable(self.client_response_hook):
self.client_response_hook(send_span, scope, message)
Expand Down
Loading