Skip to content

internal/shared/semconv: set http.route from Request.Pattern#6905

Merged
dmathieu merged 8 commits intoopen-telemetry:mainfrom
axw:otelhttp-httproute-pattern
Mar 12, 2025
Merged

internal/shared/semconv: set http.route from Request.Pattern#6905
dmathieu merged 8 commits intoopen-telemetry:mainfrom
axw:otelhttp-httproute-pattern

Conversation

@axw
Copy link
Copy Markdown
Contributor

@axw axw commented Mar 10, 2025

If Request.Pattern is set (e.g. by net/http.ServeMux), then use it for setting the http.route attribute on server request spans.

Related to #6193

(But does not close that, since according to https://opentelemetry.io/docs/specs/semconv/http/http-spans/#name we SHOULD use http.route in the span name. I'm leaving that for another day.)

@axw axw requested review from a team and dmathieu as code owners March 10, 2025 02:00
@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 10, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.6%. Comparing base (b6f6e0e) to head (4ff9fcc).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #6905     +/-   ##
=======================================
+ Coverage   75.5%   75.6%   +0.1%     
=======================================
  Files        207     207             
  Lines      19319   19354     +35     
=======================================
+ Hits       14598   14646     +48     
+ Misses      4285    4273     -12     
+ Partials     436     435      -1     
Files with missing lines Coverage Δ
...o-restful/otelrestful/internal/semconv/httpconv.go 86.7% <100.0%> (+0.7%) ⬆️
...gin-gonic/gin/otelgin/internal/semconv/httpconv.go 86.7% <100.0%> (+0.7%) ⬆️
...m/gorilla/mux/otelmux/internal/semconv/httpconv.go 86.7% <100.0%> (+0.7%) ⬆️
...tptrace/otelhttptrace/internal/semconv/httpconv.go 86.7% <100.0%> (+0.7%) ⬆️
...ion/net/http/otelhttp/internal/semconv/httpconv.go 86.7% <100.0%> (+0.7%) ⬆️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@axw axw marked this pull request as draft March 10, 2025 02:19
If Request.Pattern is set (e.g. by net/http.ServeMux),
then use it for setting the http.route attribute on
server request spans.
@axw axw force-pushed the otelhttp-httproute-pattern branch from c8a8312 to 49886b7 Compare March 10, 2025 02:30
@axw axw changed the title otelhttp: set http.route from Request.Pattern internal/shared/semconvutil: set http.route from Request.Pattern Mar 10, 2025
@axw axw marked this pull request as ready for review March 10, 2025 03:24
@dmathieu
Copy link
Copy Markdown
Member

semconvutil is for the 1.20.0 semantic conventions. It will disappear on the next release.
Don't you want to add that to internal/shared/semconv instead?

@axw
Copy link
Copy Markdown
Contributor Author

axw commented Mar 10, 2025

@dmathieu sounds like it - I wasn't aware of that :) I'll have a look tomorrow.

@axw
Copy link
Copy Markdown
Contributor Author

axw commented Mar 11, 2025

@dmathieu updated, I hope I'm doing the right thing now.

Comment thread CHANGELOG.md Outdated
@axw axw changed the title internal/shared/semconvutil: set http.route from Request.Pattern internal/shared/semconv: set http.route from Request.Pattern Mar 11, 2025
Copy link
Copy Markdown
Contributor

@akats7 akats7 left a comment

Choose a reason for hiding this comment

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

Looks like most of the middleware's are setting this value with their internal equivalent, in which case the order that the attributes are passed in the middleware matters, looks fine but just something to be aware of.

@axw
Copy link
Copy Markdown
Contributor Author

axw commented Mar 12, 2025

Thanks for pointing that out @akats7. I've checked all the in-tree instrumentation and the instrumentation-specific route attribute will take precedence.

@dmathieu
Copy link
Copy Markdown
Member

Not for this PR, but we should analyze the frameworks that have those instrumentation-specific route, and see if they could be replace to setting the request's Pattern.

@dmathieu dmathieu merged commit 163b23c into open-telemetry:main Mar 12, 2025
25 checks passed
@MrAlias MrAlias added this to the v1.36.0 milestone Mar 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants