Skip to content

feat(net/http/otelhttp): Add http.route attribute#8632

Open
gaiaz-iusipov wants to merge 14 commits intoopen-telemetry:mainfrom
gaiaz-iusipov:metric-route-attr
Open

feat(net/http/otelhttp): Add http.route attribute#8632
gaiaz-iusipov wants to merge 14 commits intoopen-telemetry:mainfrom
gaiaz-iusipov:metric-route-attr

Conversation

@gaiaz-iusipov
Copy link
Copy Markdown
Contributor

@gaiaz-iusipov gaiaz-iusipov commented Mar 4, 2026

In this pull request, I add recording of the http.route metric attribute for net/http/otelhttp.

The routeFromRequest function code is duplicated, and it might be better to move it to semconv.HTTPServer.

@gaiaz-iusipov gaiaz-iusipov requested review from a team and dmathieu as code owners March 4, 2026 06:26
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 4, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.2%. Comparing base (0a5b338) to head (1f60395).

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #8632   +/-   ##
=====================================
  Coverage   82.1%   82.2%           
=====================================
  Files        183     183           
  Lines      13798   13810   +12     
=====================================
+ Hits       11340   11352   +12     
  Misses      2054    2054           
  Partials     404     404           
Files with missing lines Coverage Δ
.../go-restful/otelrestful/internal/semconv/server.go 83.2% <100.0%> (+0.1%) ⬆️
...m/gin-gonic/gin/otelgin/internal/semconv/server.go 83.2% <100.0%> (+0.1%) ⬆️
...com/gorilla/mux/otelmux/internal/semconv/server.go 83.2% <100.0%> (+0.1%) ⬆️
.../labstack/echo/otelecho/internal/semconv/server.go 83.2% <100.0%> (+0.1%) ⬆️
...httptrace/otelhttptrace/internal/semconv/server.go 83.2% <100.0%> (+0.1%) ⬆️
...ation/net/http/otelhttp/internal/semconv/server.go 83.2% <100.0%> (+0.1%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

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

Can you please create an issue first?

From https://github.com/open-telemetry/opentelemetry-go-contrib/blob/main/CONTRIBUTING.md#contributing-code:

It's recommended that you signal your intention to contribute in the issue tracker, either by filing a new issue or by claiming an existing one.

@dmathieu
Copy link
Copy Markdown
Member

dmathieu commented Mar 4, 2026

Could this be done with a Labeler?

@gaiaz-iusipov
Copy link
Copy Markdown
Contributor Author

I created an issue where I tried to explain my motivation: #8633

@gaiaz-iusipov
Copy link
Copy Markdown
Contributor Author

I changed the logic so that the attribute is recorded by default, with an option to disable it. There is also still the question of moving the routeFromRequest function to the semconv package. Let’s discuss the final approach in the issue.

@gaiaz-iusipov gaiaz-iusipov marked this pull request as draft March 6, 2026 12:04
@gaiaz-iusipov gaiaz-iusipov changed the title feat(net/http/otelhttp): Add optional MetricRouteAttribute feat(net/http/otelhttp): Add http.route attribute Mar 9, 2026
@gaiaz-iusipov
Copy link
Copy Markdown
Contributor Author

Could this be done with a Labeler?

Sure it could, but @pellared pointed out in the issue that this attribute should always (when it is available) be recorded by default.

@gaiaz-iusipov gaiaz-iusipov marked this pull request as ready for review March 9, 2026 13:46
@dmathieu
Copy link
Copy Markdown
Member

dmathieu commented Mar 9, 2026

Sure, though this is not what that PR was doing when I left my comment.
Now: the attribute can be removed with a view. Why do we need the option?

@gaiaz-iusipov
Copy link
Copy Markdown
Contributor Author

Sure, though this is not what that PR was doing when I left my comment. Now: the attribute can be removed with a view. Why do we need the option?

So it turns out the option is not needed at all. What about moving the routeFromRequest function to semconv.HTTPServer and making it public since its logic is duplicated?

@github-actions github-actions bot requested a review from akats7 March 9, 2026 16:21
@dmathieu dmathieu requested a review from pellared March 10, 2026 15:17
Copy link
Copy Markdown
Member

@flc1125 flc1125 left a comment

Choose a reason for hiding this comment

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

Some small adjustments.

@gaiaz-iusipov
Copy link
Copy Markdown
Contributor Author

I think this PR is ready to merge.

@dmathieu
Copy link
Copy Markdown
Member

I'm waiting for @pellared's review, since he requested changes.

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