opentelemetry-go-contrib icon indicating copy to clipboard operation
opentelemetry-go-contrib copied to clipboard

otelgin: add `routeName` argument to `SpanNameFormatter` function

Open NeoCN opened this issue 1 year ago • 5 comments

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.

NeoCN avatar Jun 06 '24 04:06 NeoCN

@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 ?

NeoCN avatar Jun 06 '24 08:06 NeoCN

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

Impacted file tree graph

@@          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%> (ø)

... and 1 file with indirect coverage changes

codecov[bot] avatar Jun 06 '24 16:06 codecov[bot]

Ping @hanyuancheung

dmathieu avatar Jun 10 '24 07:06 dmathieu

@dmathieu @hanyuancheung Any suggestions for this PR?

NeoCN avatar Jun 17 '24 06:06 NeoCN

@hanyuancheung Any suggestions? Or any principles to follow?

NeoCN avatar Jul 10 '24 07:07 NeoCN

@dmathieu @hanyuancheung Do you have any suggestions for this PR? Or should I change to add new function like WithSpanNameGinFormatter ?

NeoCN avatar Oct 21 '24 11:10 NeoCN

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.

dmathieu avatar Oct 22 '24 07:10 dmathieu

I feel that this PR can be reconsidered for evaluation. Currently, it seems that otelgin is becoming active.

flc1125 avatar Nov 26 '24 05:11 flc1125