From d3735fb3a2aaead6cf676859759cc7adbe384f4d Mon Sep 17 00:00:00 2001 From: Nick Brandt Date: Mon, 11 Mar 2024 17:06:10 -0400 Subject: [PATCH 1/3] Don't call c.Error inside echotrace middlewares --- contrib/labstack/echo.v4/echotrace.go | 9 +-------- contrib/labstack/echo/echotrace.go | 3 --- 2 files changed, 1 insertion(+), 11 deletions(-) diff --git a/contrib/labstack/echo.v4/echotrace.go b/contrib/labstack/echo.v4/echotrace.go index ddbf53da08..d3a8867bd4 100644 --- a/contrib/labstack/echo.v4/echotrace.go +++ b/contrib/labstack/echo.v4/echotrace.go @@ -52,11 +52,7 @@ func Middleware(opts ...Option) echo.MiddlewareFunc { return func(c echo.Context) error { // If we have an ignoreRequestFunc, use it to see if we proceed with tracing if cfg.ignoreRequestFunc != nil && cfg.ignoreRequestFunc(c) { - if err := next(c); err != nil { - c.Error(err) - return err - } - return nil + return next(c) } request := c.Request() @@ -90,9 +86,6 @@ func Middleware(opts ...Option) echo.MiddlewareFunc { // serve the request to the next middleware err := next(c) if err != nil && !shouldIgnoreError(cfg, err) { - // invokes the registered HTTP error handler - c.Error(err) - // It is impossible to determine what the final status code of a request is in echo. // This is the best we can do. if echoErr, ok := cfg.translateError(err); ok { diff --git a/contrib/labstack/echo/echotrace.go b/contrib/labstack/echo/echotrace.go index bfd8c7918a..f1f7f063de 100644 --- a/contrib/labstack/echo/echotrace.go +++ b/contrib/labstack/echo/echotrace.go @@ -76,9 +76,6 @@ func Middleware(opts ...Option) echo.MiddlewareFunc { // serve the request to the next middleware err := next(c) if err != nil { - // invokes the registered HTTP error handler - c.Error(err) - // It is impossible to determine what the final status code of a request is in echo. // This is the best we can do. var echoErr *echo.HTTPError From 230fae8440c693bc78bffcd6f180dd4fa4a7ed2a Mon Sep 17 00:00:00 2001 From: Nick Brandt Date: Mon, 1 Apr 2024 10:20:17 -0400 Subject: [PATCH 2/3] Add test to verify broken behavior, test should fail --- contrib/labstack/echo.v4/echotrace_test.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/contrib/labstack/echo.v4/echotrace_test.go b/contrib/labstack/echo.v4/echotrace_test.go index 50af8802d1..c6a90715f9 100644 --- a/contrib/labstack/echo.v4/echotrace_test.go +++ b/contrib/labstack/echo.v4/echotrace_test.go @@ -151,9 +151,13 @@ func TestError(t *testing.T) { mt := mocktracer.Start() defer mt.Stop() var called, traced bool + handlerCalled := 0 // setup router := echo.New() + router.HTTPErrorHandler = func(err error, c echo.Context) { + handlerCalled++ + } router.Use(Middleware(WithServiceName("foobar"))) wantErr := errors.New("oh no") @@ -173,6 +177,7 @@ func TestError(t *testing.T) { // verify the errors and status are correct assert.True(called) assert.True(traced) + assert.Equal(1, handlerCalled) spans := mt.FinishedSpans() require.Len(t, spans, 1) From 7fc87fe680e25f9dabdf89b1fcffae930e15834f Mon Sep 17 00:00:00 2001 From: Nick Brandt Date: Mon, 1 Apr 2024 10:23:00 -0400 Subject: [PATCH 3/3] Remove c.Error invocation from test --- contrib/labstack/echo.v4/echotrace_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/contrib/labstack/echo.v4/echotrace_test.go b/contrib/labstack/echo.v4/echotrace_test.go index c6a90715f9..6483733f9d 100644 --- a/contrib/labstack/echo.v4/echotrace_test.go +++ b/contrib/labstack/echo.v4/echotrace_test.go @@ -167,7 +167,6 @@ func TestError(t *testing.T) { called = true err := wantErr - c.Error(err) return err }) r := httptest.NewRequest("GET", "/err", nil)