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

otelgin: Using c.FullPath() to set http.route attribute #5732

Closed
wants to merge 3 commits into from

Conversation

NeoCN
Copy link
Contributor

@NeoCN NeoCN commented Jun 5, 2024

As spanName can be custumized with SpanNameFormatter, so the spanName may not be the same with http.route, e.g. the spanName can be GET /users/:id, but the http.route is /users/:id, so, using c.FullPath() to set http.route attribute in otelgin

@NeoCN NeoCN requested a review from hanyuancheung as a code owner June 5, 2024 08:10
@NeoCN NeoCN requested a review from a team June 5, 2024 08:10
Copy link

linux-foundation-easycla bot commented Jun 5, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@dmathieu
Copy link
Member

dmathieu commented Jun 5, 2024

Could you add a test, as well as a changelog entry?

Copy link

codecov bot commented Jun 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.8%. Comparing base (1ddc6ac) to head (14d182a).

Current head 14d182a differs from pull request most recent head 9ed7145

Please upload reports for the commit 9ed7145 to get more accurate results.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #5732   +/-   ##
=====================================
  Coverage   63.8%   63.8%           
=====================================
  Files        194     194           
  Lines      12238   12236    -2     
=====================================
- Hits        7818    7817    -1     
  Misses      4197    4197           
+ Partials     223     222    -1     
Files Coverage Δ
...ation/github.com/gin-gonic/gin/otelgin/gintrace.go 81.1% <100.0%> (+0.8%) ⬆️

@NeoCN
Copy link
Contributor Author

NeoCN commented Jun 5, 2024

@dmathieu Done.

// SpanNameFormatter is used to set span name by http.request.
type SpanNameFormatter func(r *http.Request) string
// SpanNameFormatter is used to set span name by http.Request.
type SpanNameFormatter func(routeName string, r *http.Request) string
Copy link
Member

Choose a reason for hiding this comment

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

This changes the public API, and isn't what the PR describes. It should be its own PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I will create two PR for those changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#5734 created for http.route change. @dmathieu

@NeoCN NeoCN closed this Jun 5, 2024
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.

2 participants