From 14ba828b1c2eb51ea87e7da8cb8ee197ea6fcab4 Mon Sep 17 00:00:00 2001 From: Flc Date: Sun, 24 Nov 2024 22:19:33 +0800 Subject: [PATCH 01/12] feat(otelecho): add SpanNameFormatter config and tests --- .../labstack/echo/otelecho/config.go | 25 ++++++++-- .../github.com/labstack/echo/otelecho/echo.go | 9 +++- .../labstack/echo/otelecho/test/echo_test.go | 50 ++++++++++++++++++- 3 files changed, 78 insertions(+), 6 deletions(-) diff --git a/instrumentation/github.com/labstack/echo/otelecho/config.go b/instrumentation/github.com/labstack/echo/otelecho/config.go index f8041e2566e..0c9078fa213 100644 --- a/instrumentation/github.com/labstack/echo/otelecho/config.go +++ b/instrumentation/github.com/labstack/echo/otelecho/config.go @@ -4,17 +4,27 @@ package otelecho // import "go.opentelemetry.io/contrib/instrumentation/github.com/labstack/echo/otelecho" import ( + "github.com/labstack/echo/v4" "github.com/labstack/echo/v4/middleware" "go.opentelemetry.io/otel/propagation" oteltrace "go.opentelemetry.io/otel/trace" ) +// defaultSpanNameFormatter is the default function used for formatting span names. +var defaultSpanNameFormatter = func(c echo.Context) string { + return c.Path() +} + +// SpanNameFormatter is a function that takes an echo.Context and returns a span name. +type SpanNameFormatter func(c echo.Context) string + // config is used to configure the mux middleware. type config struct { - TracerProvider oteltrace.TracerProvider - Propagators propagation.TextMapPropagator - Skipper middleware.Skipper + TracerProvider oteltrace.TracerProvider + Propagators propagation.TextMapPropagator + Skipper middleware.Skipper + spanNameFormatter SpanNameFormatter } // Option specifies instrumentation configuration options. @@ -55,3 +65,12 @@ func WithSkipper(skipper middleware.Skipper) Option { cfg.Skipper = skipper }) } + +// WithSpanNameFormatter specifies a function to use for formatting span names. +func WithSpanNameFormatter(formatter SpanNameFormatter) Option { + return optionFunc(func(cfg *config) { + if formatter != nil { + cfg.spanNameFormatter = formatter + } + }) +} diff --git a/instrumentation/github.com/labstack/echo/otelecho/echo.go b/instrumentation/github.com/labstack/echo/otelecho/echo.go index 2771ebc5d9e..60f237dcf49 100644 --- a/instrumentation/github.com/labstack/echo/otelecho/echo.go +++ b/instrumentation/github.com/labstack/echo/otelecho/echo.go @@ -11,11 +11,12 @@ import ( "go.opentelemetry.io/otel" - "go.opentelemetry.io/contrib/instrumentation/github.com/labstack/echo/otelecho/internal/semconvutil" "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/propagation" semconv "go.opentelemetry.io/otel/semconv/v1.20.0" oteltrace "go.opentelemetry.io/otel/trace" + + "go.opentelemetry.io/contrib/instrumentation/github.com/labstack/echo/otelecho/internal/semconvutil" ) const ( @@ -45,6 +46,10 @@ func Middleware(service string, opts ...Option) echo.MiddlewareFunc { cfg.Skipper = middleware.DefaultSkipper } + if cfg.spanNameFormatter == nil { + cfg.spanNameFormatter = defaultSpanNameFormatter + } + return func(next echo.HandlerFunc) echo.HandlerFunc { return func(c echo.Context) error { if cfg.Skipper(c) { @@ -67,7 +72,7 @@ func Middleware(service string, opts ...Option) echo.MiddlewareFunc { rAttr := semconv.HTTPRoute(path) opts = append(opts, oteltrace.WithAttributes(rAttr)) } - spanName := c.Path() + spanName := cfg.spanNameFormatter(c) if spanName == "" { spanName = fmt.Sprintf("HTTP %s route not found", request.Method) } diff --git a/instrumentation/github.com/labstack/echo/otelecho/test/echo_test.go b/instrumentation/github.com/labstack/echo/otelecho/test/echo_test.go index 905bb63dbd7..37ad8e74c37 100644 --- a/instrumentation/github.com/labstack/echo/otelecho/test/echo_test.go +++ b/instrumentation/github.com/labstack/echo/otelecho/test/echo_test.go @@ -16,12 +16,13 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "go.opentelemetry.io/contrib/instrumentation/github.com/labstack/echo/otelecho" "go.opentelemetry.io/otel" "go.opentelemetry.io/otel/codes" "go.opentelemetry.io/otel/sdk/trace" "go.opentelemetry.io/otel/sdk/trace/tracetest" + "go.opentelemetry.io/contrib/instrumentation/github.com/labstack/echo/otelecho" + "go.opentelemetry.io/otel/attribute" oteltrace "go.opentelemetry.io/otel/trace" ) @@ -202,3 +203,50 @@ func TestErrorNotSwallowedByMiddleware(t *testing.T) { err := h(c) assert.Equal(t, assert.AnError, err) } + +func TestSpanNameFormatter(t *testing.T) { + imsb := tracetest.NewInMemoryExporter() + otel.SetTracerProvider(trace.NewTracerProvider( + trace.WithSyncer(imsb), + )) + + tests := []struct { + name string + formatter otelecho.SpanNameFormatter + expected string + }{ + { + name: "default", + formatter: nil, + expected: "/user/:id", + }, + { + name: "custom", + formatter: func(c echo.Context) string { + return "custom " + c.Path() + }, + expected: "custom /user/:id", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + defer imsb.Reset() + + router := echo.New() + router.Use(otelecho.Middleware("foobar", otelecho.WithSpanNameFormatter(test.formatter))) + router.GET("/user/:id", func(c echo.Context) error { + return c.NoContent(http.StatusOK) + }) + + r := httptest.NewRequest("GET", "/user/123", nil) + w := httptest.NewRecorder() + + router.ServeHTTP(w, r) + + spans := imsb.GetSpans() + assert.Len(t, spans, 1) + assert.Equal(t, test.expected, spans[0].Name) + }) + } +} From 95c0f4344c1896af7af5c3b4a87c05abe03306bb Mon Sep 17 00:00:00 2001 From: Flc Date: Sun, 24 Nov 2024 22:32:15 +0800 Subject: [PATCH 02/12] feat(otelecho): add SpanNameFormatter config and tests --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index f022916f572..f8f6d207837 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - Added support for providing `endpoint`, `pollingIntervalMs` and `initialSamplingRate` using environment variable `OTEL_TRACES_SAMPLER_ARG` in `go.opentelemetry.io/contrib/samples/jaegerremote`. (#6310) - Added support exporting logs via OTLP over gRPC in `go.opentelemetry.io/contrib/config`. (#6340) +- Added support custom span name in `go.opentelemetry.io/contrib/instrumentation/github.com/labstack/echo/otelecho`. (#6365) ### Fixed From 7deb0fe1ed739ca237ff761b58dcd55d43eab86c Mon Sep 17 00:00:00 2001 From: Flc Date: Mon, 25 Nov 2024 22:16:04 +0800 Subject: [PATCH 03/12] refactor(otelecho): refine span name formatter and tests --- .../github.com/labstack/echo/otelecho/config.go | 11 ++++++----- .../github.com/labstack/echo/otelecho/echo.go | 6 ++---- .../labstack/echo/otelecho/test/echo_test.go | 11 +++++++++-- 3 files changed, 17 insertions(+), 11 deletions(-) diff --git a/instrumentation/github.com/labstack/echo/otelecho/config.go b/instrumentation/github.com/labstack/echo/otelecho/config.go index 0c9078fa213..1d36673a5f9 100644 --- a/instrumentation/github.com/labstack/echo/otelecho/config.go +++ b/instrumentation/github.com/labstack/echo/otelecho/config.go @@ -4,7 +4,8 @@ package otelecho // import "go.opentelemetry.io/contrib/instrumentation/github.com/labstack/echo/otelecho" import ( - "github.com/labstack/echo/v4" + "net/http" + "github.com/labstack/echo/v4/middleware" "go.opentelemetry.io/otel/propagation" @@ -12,12 +13,12 @@ import ( ) // defaultSpanNameFormatter is the default function used for formatting span names. -var defaultSpanNameFormatter = func(c echo.Context) string { - return c.Path() +var defaultSpanNameFormatter = func(path string, _ *http.Request) string { + return path } -// SpanNameFormatter is a function that takes an echo.Context and returns a span name. -type SpanNameFormatter func(c echo.Context) string +// SpanNameFormatter is a function that takes a path and an HTTP request and returns a span name. +type SpanNameFormatter func(string, *http.Request) string // config is used to configure the mux middleware. type config struct { diff --git a/instrumentation/github.com/labstack/echo/otelecho/echo.go b/instrumentation/github.com/labstack/echo/otelecho/echo.go index 60f237dcf49..3091d1ca7f2 100644 --- a/instrumentation/github.com/labstack/echo/otelecho/echo.go +++ b/instrumentation/github.com/labstack/echo/otelecho/echo.go @@ -9,14 +9,12 @@ import ( "github.com/labstack/echo/v4" "github.com/labstack/echo/v4/middleware" + "go.opentelemetry.io/contrib/instrumentation/github.com/labstack/echo/otelecho/internal/semconvutil" "go.opentelemetry.io/otel" - "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/propagation" semconv "go.opentelemetry.io/otel/semconv/v1.20.0" oteltrace "go.opentelemetry.io/otel/trace" - - "go.opentelemetry.io/contrib/instrumentation/github.com/labstack/echo/otelecho/internal/semconvutil" ) const ( @@ -72,7 +70,7 @@ func Middleware(service string, opts ...Option) echo.MiddlewareFunc { rAttr := semconv.HTTPRoute(path) opts = append(opts, oteltrace.WithAttributes(rAttr)) } - spanName := cfg.spanNameFormatter(c) + spanName := cfg.spanNameFormatter(c.Path(), c.Request()) if spanName == "" { spanName = fmt.Sprintf("HTTP %s route not found", request.Method) } diff --git a/instrumentation/github.com/labstack/echo/otelecho/test/echo_test.go b/instrumentation/github.com/labstack/echo/otelecho/test/echo_test.go index 37ad8e74c37..b3db4a2682f 100644 --- a/instrumentation/github.com/labstack/echo/otelecho/test/echo_test.go +++ b/instrumentation/github.com/labstack/echo/otelecho/test/echo_test.go @@ -222,11 +222,18 @@ func TestSpanNameFormatter(t *testing.T) { }, { name: "custom", - formatter: func(c echo.Context) string { - return "custom " + c.Path() + formatter: func(path string, req *http.Request) string { + return "custom " + path }, expected: "custom /user/:id", }, + { + name: "has_request", + formatter: func(path string, req *http.Request) string { + return req.Method + " " + path + }, + expected: "GET /user/:id", + }, } for _, test := range tests { From 977f80672e1fb4971e58906be6e04b1d33137c95 Mon Sep 17 00:00:00 2001 From: Flc Date: Mon, 25 Nov 2024 22:17:29 +0800 Subject: [PATCH 04/12] refactor(otelecho): refine span name formatter and tests --- instrumentation/github.com/labstack/echo/otelecho/echo_test.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/instrumentation/github.com/labstack/echo/otelecho/echo_test.go b/instrumentation/github.com/labstack/echo/otelecho/echo_test.go index 9e424a617c3..4669521d53d 100644 --- a/instrumentation/github.com/labstack/echo/otelecho/echo_test.go +++ b/instrumentation/github.com/labstack/echo/otelecho/echo_test.go @@ -14,12 +14,11 @@ import ( "github.com/labstack/echo/v4" "github.com/stretchr/testify/assert" + b3prop "go.opentelemetry.io/contrib/propagators/b3" "go.opentelemetry.io/otel" "go.opentelemetry.io/otel/propagation" "go.opentelemetry.io/otel/trace" "go.opentelemetry.io/otel/trace/noop" - - b3prop "go.opentelemetry.io/contrib/propagators/b3" ) func TestGetSpanNotInstrumented(t *testing.T) { From 8ae3b69f1bd977f85e830b58cde49d006a0dd0b3 Mon Sep 17 00:00:00 2001 From: Flc Date: Mon, 25 Nov 2024 22:17:57 +0800 Subject: [PATCH 05/12] refactor(otelecho): refine span name formatter and tests --- .../github.com/labstack/echo/otelecho/test/echo_test.go | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/instrumentation/github.com/labstack/echo/otelecho/test/echo_test.go b/instrumentation/github.com/labstack/echo/otelecho/test/echo_test.go index b3db4a2682f..5d1c8e9d361 100644 --- a/instrumentation/github.com/labstack/echo/otelecho/test/echo_test.go +++ b/instrumentation/github.com/labstack/echo/otelecho/test/echo_test.go @@ -12,18 +12,15 @@ import ( "testing" "github.com/labstack/echo/v4" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "go.opentelemetry.io/contrib/instrumentation/github.com/labstack/echo/otelecho" "go.opentelemetry.io/otel" + "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/codes" "go.opentelemetry.io/otel/sdk/trace" "go.opentelemetry.io/otel/sdk/trace/tracetest" - - "go.opentelemetry.io/contrib/instrumentation/github.com/labstack/echo/otelecho" - - "go.opentelemetry.io/otel/attribute" oteltrace "go.opentelemetry.io/otel/trace" ) From 8aa4055df156e406a3b1e5c7f87eea2173993cbb Mon Sep 17 00:00:00 2001 From: Flc Date: Tue, 26 Nov 2024 08:47:21 +0800 Subject: [PATCH 06/12] feat(otelecho): Adjust the default span name --- .../github.com/labstack/echo/otelecho/config.go | 4 ++-- .../labstack/echo/otelecho/test/echo_test.go | 17 +++++++++-------- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/instrumentation/github.com/labstack/echo/otelecho/config.go b/instrumentation/github.com/labstack/echo/otelecho/config.go index 1d36673a5f9..9873c069728 100644 --- a/instrumentation/github.com/labstack/echo/otelecho/config.go +++ b/instrumentation/github.com/labstack/echo/otelecho/config.go @@ -13,8 +13,8 @@ import ( ) // defaultSpanNameFormatter is the default function used for formatting span names. -var defaultSpanNameFormatter = func(path string, _ *http.Request) string { - return path +var defaultSpanNameFormatter = func(path string, req *http.Request) string { + return req.Method + " " + path } // SpanNameFormatter is a function that takes a path and an HTTP request and returns a span name. diff --git a/instrumentation/github.com/labstack/echo/otelecho/test/echo_test.go b/instrumentation/github.com/labstack/echo/otelecho/test/echo_test.go index 5d1c8e9d361..80bf466c63b 100644 --- a/instrumentation/github.com/labstack/echo/otelecho/test/echo_test.go +++ b/instrumentation/github.com/labstack/echo/otelecho/test/echo_test.go @@ -203,9 +203,7 @@ func TestErrorNotSwallowedByMiddleware(t *testing.T) { func TestSpanNameFormatter(t *testing.T) { imsb := tracetest.NewInMemoryExporter() - otel.SetTracerProvider(trace.NewTracerProvider( - trace.WithSyncer(imsb), - )) + provider := trace.NewTracerProvider(trace.WithSyncer(imsb)) tests := []struct { name string @@ -215,7 +213,7 @@ func TestSpanNameFormatter(t *testing.T) { { name: "default", formatter: nil, - expected: "/user/:id", + expected: "GET /user/:id", }, { name: "custom", @@ -225,11 +223,11 @@ func TestSpanNameFormatter(t *testing.T) { expected: "custom /user/:id", }, { - name: "has_request", + name: "use request", formatter: func(path string, req *http.Request) string { - return req.Method + " " + path + return req.Method + " " + req.URL.String() }, - expected: "GET /user/:id", + expected: "GET /user/123", }, } @@ -238,7 +236,10 @@ func TestSpanNameFormatter(t *testing.T) { defer imsb.Reset() router := echo.New() - router.Use(otelecho.Middleware("foobar", otelecho.WithSpanNameFormatter(test.formatter))) + router.Use(otelecho.Middleware("foobar", + otelecho.WithSpanNameFormatter(test.formatter), + otelecho.WithTracerProvider(provider)), + ) router.GET("/user/:id", func(c echo.Context) error { return c.NoContent(http.StatusOK) }) From 21235e994938fc7c2cad7e0116ba5a304b26b960 Mon Sep 17 00:00:00 2001 From: Flc Date: Tue, 26 Nov 2024 08:52:01 +0800 Subject: [PATCH 07/12] feat(otelecho): Adjust the default span name --- CHANGELOG.md | 2 +- .../github.com/labstack/echo/otelecho/test/echo_test.go | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5cbe3acb9f7..9f6c8903d05 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,7 +12,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - Added support for providing `endpoint`, `pollingIntervalMs` and `initialSamplingRate` using environment variable `OTEL_TRACES_SAMPLER_ARG` in `go.opentelemetry.io/contrib/samples/jaegerremote`. (#6310) - Added support exporting logs via OTLP over gRPC in `go.opentelemetry.io/contrib/config`. (#6340) -- Added support custom span name in `go.opentelemetry.io/contrib/instrumentation/github.com/labstack/echo/otelecho`. (#6365) +- Added support custom span name and refactored the default span name in `go.opentelemetry.io/contrib/instrumentation/github.com/labstack/echo/otelecho`. (#6365) ### Fixed diff --git a/instrumentation/github.com/labstack/echo/otelecho/test/echo_test.go b/instrumentation/github.com/labstack/echo/otelecho/test/echo_test.go index 80bf466c63b..116f556708a 100644 --- a/instrumentation/github.com/labstack/echo/otelecho/test/echo_test.go +++ b/instrumentation/github.com/labstack/echo/otelecho/test/echo_test.go @@ -84,7 +84,7 @@ func TestTrace200(t *testing.T) { spans := sr.Ended() require.Len(t, spans, 1) span := spans[0] - assert.Equal(t, "/user/:id", span.Name()) + assert.Equal(t, "GET /user/:id", span.Name()) assert.Equal(t, oteltrace.SpanKindServer, span.SpanKind()) attrs := span.Attributes() assert.Contains(t, attrs, attribute.String("net.host.name", "foobar")) @@ -116,7 +116,7 @@ func TestError(t *testing.T) { spans := sr.Ended() require.Len(t, spans, 1) span := spans[0] - assert.Equal(t, "/server_err", span.Name()) + assert.Equal(t, "GET /server_err", span.Name()) attrs := span.Attributes() assert.Contains(t, attrs, attribute.String("net.host.name", "foobar")) assert.Contains(t, attrs, attribute.Int("http.status_code", http.StatusInternalServerError)) @@ -175,7 +175,7 @@ func TestStatusError(t *testing.T) { spans := sr.Ended() require.Len(t, spans, 1) span := spans[0] - assert.Equal(t, "/err", span.Name()) + assert.Equal(t, "GET /err", span.Name()) assert.Equal(t, tc.spanCode, span.Status().Code) attrs := span.Attributes() From 3ee4cc812bfa9d1bec9ba4c3d6ef2cc7904391e2 Mon Sep 17 00:00:00 2001 From: Flc Date: Tue, 26 Nov 2024 09:19:37 +0800 Subject: [PATCH 08/12] =?UTF-8?q?=E2=9C=A8=20feat(echo):=20Enhance=20span?= =?UTF-8?q?=20name=20formatter=20and=20add=20tests=20for=20lowercase=20met?= =?UTF-8?q?hods?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../labstack/echo/otelecho/config.go | 3 ++- .../labstack/echo/otelecho/test/echo_test.go | 27 ++++++++++++++++--- 2 files changed, 26 insertions(+), 4 deletions(-) diff --git a/instrumentation/github.com/labstack/echo/otelecho/config.go b/instrumentation/github.com/labstack/echo/otelecho/config.go index 9873c069728..7af5f39ad28 100644 --- a/instrumentation/github.com/labstack/echo/otelecho/config.go +++ b/instrumentation/github.com/labstack/echo/otelecho/config.go @@ -5,6 +5,7 @@ package otelecho // import "go.opentelemetry.io/contrib/instrumentation/github.c import ( "net/http" + "strings" "github.com/labstack/echo/v4/middleware" @@ -14,7 +15,7 @@ import ( // defaultSpanNameFormatter is the default function used for formatting span names. var defaultSpanNameFormatter = func(path string, req *http.Request) string { - return req.Method + " " + path + return strings.ToUpper(req.Method) + " " + path } // SpanNameFormatter is a function that takes a path and an HTTP request and returns a span name. diff --git a/instrumentation/github.com/labstack/echo/otelecho/test/echo_test.go b/instrumentation/github.com/labstack/echo/otelecho/test/echo_test.go index 116f556708a..4b767e13f94 100644 --- a/instrumentation/github.com/labstack/echo/otelecho/test/echo_test.go +++ b/instrumentation/github.com/labstack/echo/otelecho/test/echo_test.go @@ -217,7 +217,7 @@ func TestSpanNameFormatter(t *testing.T) { }, { name: "custom", - formatter: func(path string, req *http.Request) string { + formatter: func(path string, _ *http.Request) string { return "custom " + path }, expected: "custom /user/:id", @@ -244,9 +244,8 @@ func TestSpanNameFormatter(t *testing.T) { return c.NoContent(http.StatusOK) }) - r := httptest.NewRequest("GET", "/user/123", nil) + r := httptest.NewRequest(http.MethodGet, "/user/123", nil) w := httptest.NewRecorder() - router.ServeHTTP(w, r) spans := imsb.GetSpans() @@ -254,4 +253,26 @@ func TestSpanNameFormatter(t *testing.T) { assert.Equal(t, test.expected, spans[0].Name) }) } + + t.Run("test default span name formatter with lowercase method", func(t *testing.T) { + router := echo.New() + router.Use(otelecho.Middleware("foobar", + otelecho.WithTracerProvider(provider)), + ) + router.GET("/user/:id", func(c echo.Context) error { + return c.NoContent(http.StatusOK) + }) + + for _, method := range []string{"Get", "GET", "get"} { + r := httptest.NewRequest(method, "/user/123", nil) + w := httptest.NewRecorder() + router.ServeHTTP(w, r) + + spans := imsb.GetSpans() + assert.Len(t, spans, 1) + assert.Equal(t, "GET /user/:id", spans[0].Name) + + imsb.Reset() + } + }) } From 23c9bc0f0f151f7d7150e52c91e18ab1291f88e5 Mon Sep 17 00:00:00 2001 From: Flc Date: Tue, 26 Nov 2024 09:23:02 +0800 Subject: [PATCH 09/12] fix(otelecho): Upgrade assertions to requirements in echo_test.go for stricter validation --- .../github.com/labstack/echo/otelecho/test/echo_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/instrumentation/github.com/labstack/echo/otelecho/test/echo_test.go b/instrumentation/github.com/labstack/echo/otelecho/test/echo_test.go index 4b767e13f94..bc399f62834 100644 --- a/instrumentation/github.com/labstack/echo/otelecho/test/echo_test.go +++ b/instrumentation/github.com/labstack/echo/otelecho/test/echo_test.go @@ -249,7 +249,7 @@ func TestSpanNameFormatter(t *testing.T) { router.ServeHTTP(w, r) spans := imsb.GetSpans() - assert.Len(t, spans, 1) + require.Len(t, spans, 1) assert.Equal(t, test.expected, spans[0].Name) }) } @@ -269,7 +269,7 @@ func TestSpanNameFormatter(t *testing.T) { router.ServeHTTP(w, r) spans := imsb.GetSpans() - assert.Len(t, spans, 1) + require.Len(t, spans, 1) assert.Equal(t, "GET /user/:id", spans[0].Name) imsb.Reset() From b120bdd6b4499695310016a52879011b0780583c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Flc=E3=82=9B?= Date: Tue, 26 Nov 2024 10:25:37 +0800 Subject: [PATCH 10/12] refactor(otelecho): comply with OTEL span naming specs --- CHANGELOG.md | 5 +- .../labstack/echo/otelecho/config.go | 27 +------ .../github.com/labstack/echo/otelecho/echo.go | 32 +++++--- .../labstack/echo/otelecho/test/echo_test.go | 80 +++++++------------ 4 files changed, 60 insertions(+), 84 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9f6c8903d05..c61fe075a85 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,7 +12,10 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - Added support for providing `endpoint`, `pollingIntervalMs` and `initialSamplingRate` using environment variable `OTEL_TRACES_SAMPLER_ARG` in `go.opentelemetry.io/contrib/samples/jaegerremote`. (#6310) - Added support exporting logs via OTLP over gRPC in `go.opentelemetry.io/contrib/config`. (#6340) -- Added support custom span name and refactored the default span name in `go.opentelemetry.io/contrib/instrumentation/github.com/labstack/echo/otelecho`. (#6365) + +### Changed + +- Change the logic of span to make it comply with OTEL specifications in `go.opentelemetry.io/contrib/instrumentation/github.com/labstack/echo/otelecho`. (#6365) ### Fixed diff --git a/instrumentation/github.com/labstack/echo/otelecho/config.go b/instrumentation/github.com/labstack/echo/otelecho/config.go index 7af5f39ad28..f8041e2566e 100644 --- a/instrumentation/github.com/labstack/echo/otelecho/config.go +++ b/instrumentation/github.com/labstack/echo/otelecho/config.go @@ -4,29 +4,17 @@ package otelecho // import "go.opentelemetry.io/contrib/instrumentation/github.com/labstack/echo/otelecho" import ( - "net/http" - "strings" - "github.com/labstack/echo/v4/middleware" "go.opentelemetry.io/otel/propagation" oteltrace "go.opentelemetry.io/otel/trace" ) -// defaultSpanNameFormatter is the default function used for formatting span names. -var defaultSpanNameFormatter = func(path string, req *http.Request) string { - return strings.ToUpper(req.Method) + " " + path -} - -// SpanNameFormatter is a function that takes a path and an HTTP request and returns a span name. -type SpanNameFormatter func(string, *http.Request) string - // config is used to configure the mux middleware. type config struct { - TracerProvider oteltrace.TracerProvider - Propagators propagation.TextMapPropagator - Skipper middleware.Skipper - spanNameFormatter SpanNameFormatter + TracerProvider oteltrace.TracerProvider + Propagators propagation.TextMapPropagator + Skipper middleware.Skipper } // Option specifies instrumentation configuration options. @@ -67,12 +55,3 @@ func WithSkipper(skipper middleware.Skipper) Option { cfg.Skipper = skipper }) } - -// WithSpanNameFormatter specifies a function to use for formatting span names. -func WithSpanNameFormatter(formatter SpanNameFormatter) Option { - return optionFunc(func(cfg *config) { - if formatter != nil { - cfg.spanNameFormatter = formatter - } - }) -} diff --git a/instrumentation/github.com/labstack/echo/otelecho/echo.go b/instrumentation/github.com/labstack/echo/otelecho/echo.go index 3091d1ca7f2..bdbbef24d80 100644 --- a/instrumentation/github.com/labstack/echo/otelecho/echo.go +++ b/instrumentation/github.com/labstack/echo/otelecho/echo.go @@ -4,7 +4,9 @@ package otelecho // import "go.opentelemetry.io/contrib/instrumentation/github.com/labstack/echo/otelecho" import ( - "fmt" + "net/http" + "slices" + "strings" "github.com/labstack/echo/v4" "github.com/labstack/echo/v4/middleware" @@ -44,10 +46,6 @@ func Middleware(service string, opts ...Option) echo.MiddlewareFunc { cfg.Skipper = middleware.DefaultSkipper } - if cfg.spanNameFormatter == nil { - cfg.spanNameFormatter = defaultSpanNameFormatter - } - return func(next echo.HandlerFunc) echo.HandlerFunc { return func(c echo.Context) error { if cfg.Skipper(c) { @@ -70,10 +68,7 @@ func Middleware(service string, opts ...Option) echo.MiddlewareFunc { rAttr := semconv.HTTPRoute(path) opts = append(opts, oteltrace.WithAttributes(rAttr)) } - spanName := cfg.spanNameFormatter(c.Path(), c.Request()) - if spanName == "" { - spanName = fmt.Sprintf("HTTP %s route not found", request.Method) - } + spanName := spanNameFormatter(c) ctx, span := tracer.Start(ctx, spanName, opts...) defer span.End() @@ -99,3 +94,22 @@ func Middleware(service string, opts ...Option) echo.MiddlewareFunc { } } } + +func spanNameFormatter(c echo.Context) string { + method, path := strings.ToUpper(c.Request().Method), c.Path() + if !slices.Contains([]string{ + http.MethodGet, http.MethodHead, + http.MethodPost, http.MethodPut, + http.MethodPatch, http.MethodDelete, + http.MethodConnect, http.MethodOptions, + http.MethodTrace, + }, method) { + method = "HTTP" + } + + if path != "" { + return method + " " + path + } + + return method +} diff --git a/instrumentation/github.com/labstack/echo/otelecho/test/echo_test.go b/instrumentation/github.com/labstack/echo/otelecho/test/echo_test.go index bc399f62834..e3b3f06245d 100644 --- a/instrumentation/github.com/labstack/echo/otelecho/test/echo_test.go +++ b/instrumentation/github.com/labstack/echo/otelecho/test/echo_test.go @@ -206,29 +206,34 @@ func TestSpanNameFormatter(t *testing.T) { provider := trace.NewTracerProvider(trace.WithSyncer(imsb)) tests := []struct { - name string - formatter otelecho.SpanNameFormatter - expected string + name string + method string + path string + url string + expected string }{ - { - name: "default", - formatter: nil, - expected: "GET /user/:id", - }, - { - name: "custom", - formatter: func(path string, _ *http.Request) string { - return "custom " + path - }, - expected: "custom /user/:id", - }, - { - name: "use request", - formatter: func(path string, req *http.Request) string { - return req.Method + " " + req.URL.String() - }, - expected: "GET /user/123", - }, + // Test for standard methods + {"", http.MethodGet, "/user/:id", "/user/123", "GET /user/:id"}, + {"", http.MethodHead, "/user/:id", "/user/123", "HEAD /user/:id"}, + {"", http.MethodPost, "/user/:id", "/user/123", "POST /user/:id"}, + {"", http.MethodPut, "/user/:id", "/user/123", "PUT /user/:id"}, + {"", http.MethodPatch, "/user/:id", "/user/123", "PATCH /user/:id"}, + {"", http.MethodDelete, "/user/:id", "/user/123", "DELETE /user/:id"}, + {"", http.MethodConnect, "/user/:id", "/user/123", "CONNECT /user/:id"}, + {"", http.MethodOptions, "/user/:id", "/user/123", "OPTIONS /user/:id"}, + {"", http.MethodTrace, "/user/:id", "/user/123", "TRACE /user/:id"}, + {"", http.MethodGet, "/", "/", "GET /"}, + + // Test for no route + {"", http.MethodGet, "/", "/user/id", "GET"}, + + // Test for case-insensitive method + {"", "get", "/user/123", "/user/123", "GET /user/123"}, + {"", "Get", "/user/123", "/user/123", "GET /user/123"}, + {"", "GET", "/user/:id", "/user/123", "GET /user/:id"}, + + // Test for invalid method + {"", "INVALID", "/user/123", "/user/123", "HTTP /user/123"}, } for _, test := range tests { @@ -236,15 +241,12 @@ func TestSpanNameFormatter(t *testing.T) { defer imsb.Reset() router := echo.New() - router.Use(otelecho.Middleware("foobar", - otelecho.WithSpanNameFormatter(test.formatter), - otelecho.WithTracerProvider(provider)), - ) - router.GET("/user/:id", func(c echo.Context) error { + router.Use(otelecho.Middleware("foobar", otelecho.WithTracerProvider(provider))) + router.Add(test.method, test.path, func(c echo.Context) error { return c.NoContent(http.StatusOK) }) - r := httptest.NewRequest(http.MethodGet, "/user/123", nil) + r := httptest.NewRequest(test.method, test.url, nil) w := httptest.NewRecorder() router.ServeHTTP(w, r) @@ -253,26 +255,4 @@ func TestSpanNameFormatter(t *testing.T) { assert.Equal(t, test.expected, spans[0].Name) }) } - - t.Run("test default span name formatter with lowercase method", func(t *testing.T) { - router := echo.New() - router.Use(otelecho.Middleware("foobar", - otelecho.WithTracerProvider(provider)), - ) - router.GET("/user/:id", func(c echo.Context) error { - return c.NoContent(http.StatusOK) - }) - - for _, method := range []string{"Get", "GET", "get"} { - r := httptest.NewRequest(method, "/user/123", nil) - w := httptest.NewRecorder() - router.ServeHTTP(w, r) - - spans := imsb.GetSpans() - require.Len(t, spans, 1) - assert.Equal(t, "GET /user/:id", spans[0].Name) - - imsb.Reset() - } - }) } From 9d9968ee0a19cce50015dd495bc4b86a22232cf9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Flc=E3=82=9B?= Date: Wed, 27 Nov 2024 17:12:32 +0800 Subject: [PATCH 11/12] Update CHANGELOG.md Co-authored-by: Damien Mathieu <42@dmathieu.com> --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c61fe075a85..e85f3af70b6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,7 +15,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ### Changed -- Change the logic of span to make it comply with OTEL specifications in `go.opentelemetry.io/contrib/instrumentation/github.com/labstack/echo/otelecho`. (#6365) +- Change the span name to be `GET /path` so it complies with the OTel HTTP semantic conventions in `go.opentelemetry.io/contrib/instrumentation/github.com/labstack/echo/otelecho`. (#6365) ### Fixed From 6b0d7a740a9892aeeef54e367b6762e48dff3293 Mon Sep 17 00:00:00 2001 From: Flc Date: Wed, 27 Nov 2024 18:30:17 +0800 Subject: [PATCH 12/12] test(otelecho): enhance test descriptions for clarity and coverage --- .../labstack/echo/otelecho/test/echo_test.go | 30 +++++++++---------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/instrumentation/github.com/labstack/echo/otelecho/test/echo_test.go b/instrumentation/github.com/labstack/echo/otelecho/test/echo_test.go index e3b3f06245d..33487974c56 100644 --- a/instrumentation/github.com/labstack/echo/otelecho/test/echo_test.go +++ b/instrumentation/github.com/labstack/echo/otelecho/test/echo_test.go @@ -213,27 +213,27 @@ func TestSpanNameFormatter(t *testing.T) { expected string }{ // Test for standard methods - {"", http.MethodGet, "/user/:id", "/user/123", "GET /user/:id"}, - {"", http.MethodHead, "/user/:id", "/user/123", "HEAD /user/:id"}, - {"", http.MethodPost, "/user/:id", "/user/123", "POST /user/:id"}, - {"", http.MethodPut, "/user/:id", "/user/123", "PUT /user/:id"}, - {"", http.MethodPatch, "/user/:id", "/user/123", "PATCH /user/:id"}, - {"", http.MethodDelete, "/user/:id", "/user/123", "DELETE /user/:id"}, - {"", http.MethodConnect, "/user/:id", "/user/123", "CONNECT /user/:id"}, - {"", http.MethodOptions, "/user/:id", "/user/123", "OPTIONS /user/:id"}, - {"", http.MethodTrace, "/user/:id", "/user/123", "TRACE /user/:id"}, - {"", http.MethodGet, "/", "/", "GET /"}, + {"standard method of GET", http.MethodGet, "/user/:id", "/user/123", "GET /user/:id"}, + {"standard method of HEAD", http.MethodHead, "/user/:id", "/user/123", "HEAD /user/:id"}, + {"standard method of POST", http.MethodPost, "/user/:id", "/user/123", "POST /user/:id"}, + {"standard method of PUT", http.MethodPut, "/user/:id", "/user/123", "PUT /user/:id"}, + {"standard method of PATCH", http.MethodPatch, "/user/:id", "/user/123", "PATCH /user/:id"}, + {"standard method of DELETE", http.MethodDelete, "/user/:id", "/user/123", "DELETE /user/:id"}, + {"standard method of CONNECT", http.MethodConnect, "/user/:id", "/user/123", "CONNECT /user/:id"}, + {"standard method of OPTIONS", http.MethodOptions, "/user/:id", "/user/123", "OPTIONS /user/:id"}, + {"standard method of TRACE", http.MethodTrace, "/user/:id", "/user/123", "TRACE /user/:id"}, + {"standard method of GET, but it's another route.", http.MethodGet, "/", "/", "GET /"}, // Test for no route - {"", http.MethodGet, "/", "/user/id", "GET"}, + {"no route", http.MethodGet, "/", "/user/id", "GET"}, // Test for case-insensitive method - {"", "get", "/user/123", "/user/123", "GET /user/123"}, - {"", "Get", "/user/123", "/user/123", "GET /user/123"}, - {"", "GET", "/user/:id", "/user/123", "GET /user/:id"}, + {"all lowercase", "get", "/user/123", "/user/123", "GET /user/123"}, + {"partial capitalization", "Get", "/user/123", "/user/123", "GET /user/123"}, + {"full capitalization", "GET", "/user/:id", "/user/123", "GET /user/:id"}, // Test for invalid method - {"", "INVALID", "/user/123", "/user/123", "HTTP /user/123"}, + {"invalid method", "INVALID", "/user/123", "/user/123", "HTTP /user/123"}, } for _, test := range tests {