Skip to content
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

confighttp: route-based span naming for ServeMux #12593

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

axw
Copy link
Contributor

@axw axw commented Mar 10, 2025

Description

Update confighttp.ServerConfig.ToServer to handle ServeMux specially: if the handler supplied to ToServer is an http.ServeMux, we use its Handler method to determine the matching pattern and use that to name the span as described at https://opentelemetry.io/docs/specs/semconv/http/http-spans/#name. If there is no matching pattern then we fall back to "unknown route".

This approach means that everything will magically just work for components that supply a ServeMux, but there are a couple of downsides:

  • this is a little bit magical, and won't help for components that don't happen to use a ServeMux
  • route pattern matching needs to be done twice: once for the instrumentation, and once for routing to the correct handler

The alternative that I started working on before I came to this solution was to introduce a new method, ServerConfig.ToServeMux, which would not accept an http.Handler, and would instead return a type that wraps *http.ServeMux, and would instrument handlers as they are registered. With that approach the instrumentation would come after the routing, and we would install a default / NotFoundHandler that is also instrumented. This approach is a bit less magical, and a bit more efficient, but would require all the components to be updated.

Link to tracking issue

Fixes #12468 (for any receiver that uses a ServeMux, like otlpreceiver)

Testing

  • Added a unit test to confighttp for the instrumentation added by ServerConfig.ToServer.
  • Tested with a otlpreceiver:
    • ran make run with trace telemetry enabled, exporting to console
    • used telemetrygen logs --otlp-http, confirmed that a span named "POST /v1/logs" is produced
    • used curl http://localhost:4318, confirmed that a span named "GET unknown route" is produced

Documentation

Added doc comment to confighttp.ServerConfig.ToServer explaining the new behaviour.

Copy link

codecov bot commented Mar 10, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.17%. Comparing base (7d3e03e) to head (52f39b3).
Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #12593      +/-   ##
==========================================
- Coverage   92.18%   92.17%   -0.02%     
==========================================
  Files         469      471       +2     
  Lines       25394    25583     +189     
==========================================
+ Hits        23409    23580     +171     
- Misses       1574     1591      +17     
- Partials      411      412       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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

@axw axw force-pushed the confighttp-server-spanname branch 3 times, most recently from 746c104 to b8d31c4 Compare March 10, 2025 04:52
If the handler supplied to ToServer is an *http.ServeMux,
use its Handler method to determine the matching pattern
and use that to name the span as described at
https://opentelemetry.io/docs/specs/semconv/http/http-spans/#name
If there is no matching pattern, fall back to "unknown route".

Fixes
open-telemetry#12468
@axw axw force-pushed the confighttp-server-spanname branch from b8d31c4 to ebfa547 Compare March 10, 2025 05:19
@axw axw marked this pull request as ready for review March 10, 2025 05:46
@axw axw requested a review from a team as a code owner March 10, 2025 05:46
@axw axw requested a review from jpkrohling March 10, 2025 05:46
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.

[confighttp] otelhttp-generated span names may have high cardinality
1 participant