Skip to content

otelhttp: handle request Pattern #7192

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.

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

Merged
merged 11 commits into from
May 15, 2025

Conversation

dmathieu
Copy link
Member

Closes #6193
Handles otelhttp for #6919 (@Ananyasinha13 I know you're assigned this, but it has been 2 weeks with no movement).

@dmathieu
Copy link
Member Author

This has the downside of running the span name formatter twice if there's a pattern.
I'm open to alternatives, but it would be good if they weren't modifying the public API.

@dmathieu
Copy link
Member Author

cc @seankhliao @axw

@dmathieu dmathieu marked this pull request as ready for review April 10, 2025 08:13
@dmathieu dmathieu requested a review from a team as a code owner April 10, 2025 08:13
Copy link

codecov bot commented Apr 10, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.2%. Comparing base (09c7ab5) to head (4ebb00d).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #7192     +/-   ##
=======================================
- Coverage   81.2%   81.2%   -0.1%     
=======================================
  Files        207     207             
  Lines      18271   18275      +4     
=======================================
+ Hits       14844   14845      +1     
- Misses      3006    3008      +2     
- Partials     421     422      +1     
Files with missing lines Coverage Δ
instrumentation/net/http/otelhttp/config.go 83.7% <ø> (ø)
instrumentation/net/http/otelhttp/handler.go 93.0% <100.0%> (+0.1%) ⬆️

... 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.

Copy link
Contributor

@axw axw left a comment

Choose a reason for hiding this comment

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

Neat.

One other downside of changing the name after handling the request is that it won't work well with samplers that care about the span name, like the per-operation Jaeger remote sampler, IIANM.

@dmathieu
Copy link
Member Author

I can add a comment to the WithSpanNameFormatter method for better awareness of that.

Copy link
Contributor

@seankhliao seankhliao left a comment

Choose a reason for hiding this comment

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

It's a cool trick,
it may be a bit fragile if someone uses some other middleware that clones the request (uses WithContext) between otelhttp and the mux, only the new copy will have the Pattern set.

Can we have a spanformatter func or option to use the pattern (a separate pr?)

@dmathieu
Copy link
Member Author

Can we have a spanformatter func or option to use the pattern (a separate pr?)

I'd make that a separate PR, yes. Do you want to open one to better articulate what you need?

@seankhliao
Copy link
Contributor

sure I can make one

seankhliao added a commit to seankhliao/opentelemetry-go-contrib that referenced this pull request Apr 10, 2025
Based on top of open-telemetry#7192

Makes SpanNameFormatter a type alias for documentation.
Exports the default formatters.
Adds a new formatter for use with http mux patterns.
@seankhliao
Copy link
Contributor

I was thinking something simple like a44c60f (currently based on top of this pr).

@MrAlias MrAlias added this to the v1.36.0 milestone Apr 11, 2025
@MrAlias
Copy link
Contributor

MrAlias commented May 13, 2025

@XSAM PTAL

@XSAM XSAM merged commit 1c65392 into open-telemetry:main May 15, 2025
28 checks passed
github-merge-queue bot pushed a commit to open-telemetry/opentelemetry-collector that referenced this pull request May 28, 2025
#### Description

Update the confighttp server span names to use the low-cardnality
request pattern, rather than the full client-specified path, as
described by the specification:
https://opentelemetry.io/docs/specs/semconv/http/http-spans/#name.

Requires
open-telemetry/opentelemetry-go-contrib#7192

#### Link to tracking issue

Fixes #12468

#### Testing

Added a unit test.

#### Documentation

None

---------

Co-authored-by: Alex Boten <[email protected]>
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.

Automatic route/pattern naming for http.ServeMux
7 participants