opentelemetry-go-contrib
opentelemetry-go-contrib copied to clipboard
otelgin: add `routeName` argument to `SpanNameFormatter` function
For Gin framework, the route name can only be get by using FullPath() function in gin.Context, the original SpanNameFormatter only has the http.Request argument that can not get the right route name if route parameters used.
This PR add routeName argument to SpanNameFormatter function, align with SpanNameFormatter in otelmux.
@dmathieu Currently no other info needed from the gin.Context. Honestly, using gin.Context will be more flexible as lots of things can be got from the context. Do we need the flexibility ?
Codecov Report
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 66.6%. Comparing base (
0a25107) to head (1fd7392).
Additional details and impacted files
@@ Coverage Diff @@
## main #5741 +/- ##
=====================================
Coverage 66.6% 66.6%
=====================================
Files 192 192
Lines 15525 15525
=====================================
+ Hits 10351 10354 +3
+ Misses 4883 4881 -2
+ Partials 291 290 -1
| Files with missing lines | Coverage Δ | |
|---|---|---|
| ...ation/github.com/gin-gonic/gin/otelgin/gintrace.go | 84.0% <100.0%> (ø) |
|
| ...ntation/github.com/gin-gonic/gin/otelgin/option.go | 100.0% <100.0%> (ø) |
Ping @hanyuancheung
@dmathieu @hanyuancheung Any suggestions for this PR?
@hanyuancheung Any suggestions? Or any principles to follow?
@dmathieu @hanyuancheung Do you have any suggestions for this PR? Or should I change to add new function like WithSpanNameGinFormatter ?
otelgin is now deprecated due to a lack of owner: https://github.com/open-telemetry/opentelemetry-go-contrib/issues/6190 While we would be happy to get a new owner for this package, I'm not sure introducing breaking changes at this stage makes sense. If you're interested in stepping up to own this package, I'd recommend starting with smaller, more trivial changes.
I feel that this PR can be reconsidered for evaluation. Currently, it seems that otelgin is becoming active.