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
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm

- The superfluous `response.WriteHeader` call in `go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp` when the response writer is flushed. (#5634)
- Custom attributes targeting metrics recorded by the `go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp` are not ignored anymore. (#5129)

- Using `c.FullPath()` to set `http.route` attribute in [`otelgin`](go.opentelemetry.io/contrib/instrumentation/github.com/gin-gonic/gin/otelgin)
- Support pass `routeName` to `SpanNameFormatter` in [`otelgin`](go.opentelemetry.io/contrib/instrumentation/github.com/gin-gonic/gin/otelgin)
### Deprecated

- The `go.opentelemetry.io/contrib/instrumentation/go.mongodb.org/mongo-driver/mongo/otelmongo` package is deprecated.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,19 +60,17 @@ func Middleware(service string, opts ...Option) gin.HandlerFunc {
ctx := cfg.Propagators.Extract(savedCtx, propagation.HeaderCarrier(c.Request.Header))
opts := []oteltrace.SpanStartOption{
oteltrace.WithAttributes(semconvutil.HTTPServerRequest(service, c.Request)...),
oteltrace.WithAttributes(semconv.HTTPRoute(c.FullPath())),
oteltrace.WithSpanKind(oteltrace.SpanKindServer),
}
var spanName string
if cfg.SpanNameFormatter == nil {
spanName = c.FullPath()
} else {
spanName = cfg.SpanNameFormatter(c.Request)
spanName = cfg.SpanNameFormatter(c.FullPath(), c.Request)
}
if spanName == "" {
spanName = fmt.Sprintf("HTTP %s route not found", c.Request.Method)
} else {
rAttr := semconv.HTTPRoute(spanName)
opts = append(opts, oteltrace.WithAttributes(rAttr))
}
ctx, span := tracer.Start(ctx, spanName, opts...)
defer span.End()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,8 @@ type config struct {
// be traced. A Filter must return true if the request should be traced.
type Filter func(*http.Request) bool

// 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


// Option specifies instrumentation configuration options.
type Option interface {
Expand Down Expand Up @@ -70,9 +70,11 @@ func WithFilter(f ...Filter) Option {
})
}

// WithSpanNameFormatter takes a function that will be called on every
// request and the returned string will become the Span Name.
func WithSpanNameFormatter(f func(r *http.Request) string) Option {
// WithSpanNameFormatter specifies a function to use for generating a custom span
// name. By default, the route name (path template or regexp) is used. The route
// name is provided so you can use it in the span name without needing to
// duplicate the logic for extracting it from the request.
func WithSpanNameFormatter(f func(routeName string, r *http.Request) string) Option {
return optionFunc(func(c *config) {
c.SpanNameFormatter = f
})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ package test

import (
"errors"
"fmt"
"html/template"
"net/http"
"net/http/httptest"
Expand Down Expand Up @@ -160,7 +161,7 @@ func TestSpanName(t *testing.T) {
wantSpanName string
}{
{"/user/1", nil, "/user/:id"},
{"/user/1", func(r *http.Request) string { return r.URL.Path }, "/user/1"},
{"/user/1", func(route string, r *http.Request) string { return r.URL.Path }, "/user/1"},
}
for _, tc := range testCases {
t.Run(tc.requestPath, func(t *testing.T) {
Expand All @@ -179,6 +180,39 @@ func TestSpanName(t *testing.T) {
}
}

func TestHTTPRouteWithSpanNameFormatter(t *testing.T) {
sr := tracetest.NewSpanRecorder()
provider := sdktrace.NewTracerProvider(sdktrace.WithSpanProcessor(sr))

router := gin.New()
router.Use(otelgin.Middleware("foobar",
otelgin.WithTracerProvider(provider),
otelgin.WithSpanNameFormatter(func(routeName string, r *http.Request) string {
return fmt.Sprintf("%s %s", r.Method, routeName)
}),
))
router.GET("/user/:id", func(c *gin.Context) {
id := c.Param("id")
_, _ = c.Writer.Write([]byte(id))
})

r := httptest.NewRequest("GET", "/user/123", nil)
w := httptest.NewRecorder()

// do and verify the request
router.ServeHTTP(w, r)
response := w.Result()
require.Equal(t, http.StatusOK, response.StatusCode)

// verify traces look good
spans := sr.Ended()
span := spans[0]
assert.Equal(t, "GET /user/:id", span.Name())
attr := span.Attributes()
assert.Contains(t, attr, attribute.String("http.method", "GET"))
assert.Contains(t, attr, attribute.String("http.route", "/user/:id"))
}

func TestHTML(t *testing.T) {
sr := tracetest.NewSpanRecorder()
provider := sdktrace.NewTracerProvider(sdktrace.WithSpanProcessor(sr))
Expand Down