Add http.route attribute and update span name from Plug router#621
Add http.route attribute and update span name from Plug router#621lunks wants to merge 5 commits intoopen-telemetry:mainfrom
Conversation
Bandit created HTTP server spans with atom names (e.g. :GET) from semconv
values. The OTel spec requires span names to be strings. This changes
the initial span name to use conn.method (a string like "GET").
Additionally, at request completion, the span name is updated to include
the route template ("{method} {route}") when conn.private[:plug_route]
is available. This is set by Phoenix and other Plug-based routers during
dispatch, enabling proper span names without requiring opentelemetry_phoenix.
Follows up on open-telemetry#401 and the discussion about OTel HTTP semantic conventions
requiring span names to be "{method} {http.route}" when the route is known.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 15a197cb52
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Avoid unbounded cardinality by mapping non-standard HTTP methods to "HTTP" in maybe_update_span_name, matching the initial span name logic. The exception handler cannot update the span name with the route because Bandit's exception telemetry uses the original conn from before the Plug handler ran, so conn.private[:plug_route] is not available.
|
Addressed the review feedback in c0df875: Re: exception handler span naming — Bandit's exception telemetry uses the original conn from before the Plug handler ran ( Re: unbounded cardinality from unknown methods — Added |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c0df875ea9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- Set HTTPAttributes.http_route() attribute when route is available - Add test for POST method with route update - Add test for 5xx response with route update
|
Why a string over an atom? |
|
I can revert the atom-to-string change since it's not necessary for adding the route from From my research, only the cowboy, bandit, and req libraries use atoms as span names. Ruby Otel libraries, although not enforced by the SDK, also use strings. |
|
@lunks I do not disagree with the inconsistency, but I would rather keep the PR focused on one thing at a time. We need to clean up, and we need to first document them so we are all aligned and know what to expect. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
lunks
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 98285aea28
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| |> Tracer.set_attributes() | ||
| end | ||
|
|
||
| maybe_update_span_name(conn) |
There was a problem hiding this comment.
This couldn't be moved to the start event?
There was a problem hiding this comment.
No. We only have the URL path, not the route name. call_plug! sets it, but the span has already been started:
https://github.com/mtrudel/bandit/blob/1.10.3/lib/bandit/pipeline.ex#L38-L42
There was a problem hiding this comment.
@tsloughter I am gonna defer to you, I am not sure if changing midway the span name is a major concern or not
There was a problem hiding this comment.
If you folks think there's a better approach to accomplish a similar result, let me know.
4 participants
Summary
conn.private[:plug_route]to build"GET /items/:id"span names and set thehttp.routeattribute"HTTP"to prevent unbounded span name cardinality:plug_routeFollows up on #401.