From b21f9138e53a1b813d191385d1b8544b2e9ef30f Mon Sep 17 00:00:00 2001 From: Kyle Nusbaum Date: Wed, 8 Sep 2021 13:01:43 -0500 Subject: [PATCH 1/7] contrib/labstack/{echo, echo.v4}: improve error detection This adds improved error detection to the echo integrations. Previously, any returned error was recorded as an error in the echo span. This patch causes the integration to inspect the returned error to determine if it is a echo.HTTPError which it can use to discriminate between real errors (5xx and up) or non-errors (4xx and below) Fixes #987 --- contrib/labstack/echo.v4/echotrace.go | 18 +++++++++++++++++- contrib/labstack/echo/echotrace.go | 18 +++++++++++++++++- 2 files changed, 34 insertions(+), 2 deletions(-) diff --git a/contrib/labstack/echo.v4/echotrace.go b/contrib/labstack/echo.v4/echotrace.go index 6afa2562bc..6bad4ce055 100644 --- a/contrib/labstack/echo.v4/echotrace.go +++ b/contrib/labstack/echo.v4/echotrace.go @@ -8,6 +8,7 @@ package echo import ( "math" + "strconv" "gopkg.in/DataDog/dd-trace-go.v1/contrib/internal/httptrace" "gopkg.in/DataDog/dd-trace-go.v1/ddtrace" @@ -75,8 +76,23 @@ func Middleware(opts ...Option) echo.MiddlewareFunc { finishOpts = append(finishOpts, tracer.WithError(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. + switch err := err.(type) { + case *echo.HTTPError: + if err.Code >= 500 { + span.SetTag(ext.Error, err) + } + span.SetTag(ext.HTTPCode, strconv.Itoa(err.Code)) + default: + // Any non-HTTPError errors appear as 5xx errors. + span.SetTag(ext.Error, err) + span.SetTag(ext.HTTPCode, "500") + } + } else { + span.SetTag(ext.HTTPCode, "200") + } return err } } diff --git a/contrib/labstack/echo/echotrace.go b/contrib/labstack/echo/echotrace.go index 5a827a437a..9945724c04 100644 --- a/contrib/labstack/echo/echotrace.go +++ b/contrib/labstack/echo/echotrace.go @@ -8,6 +8,7 @@ package echo import ( "math" + "strconv" "gopkg.in/DataDog/dd-trace-go.v1/contrib/internal/httptrace" "gopkg.in/DataDog/dd-trace-go.v1/ddtrace" @@ -60,8 +61,23 @@ func Middleware(opts ...Option) echo.MiddlewareFunc { finishOpts = append(finishOpts, tracer.WithError(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. + switch err := err.(type) { + case *echo.HTTPError: + if err.Code >= 500 { + span.SetTag(ext.Error, err) + } + span.SetTag(ext.HTTPCode, strconv.Itoa(err.Code)) + default: + // Any non-HTTPError errors appear as 5xx errors. + span.SetTag(ext.Error, err) + span.SetTag(ext.HTTPCode, "500") + } + } else { + span.SetTag(ext.HTTPCode, "200") + } return err } } From b879761bff218b7045a7cea848ccd4a45bc053f0 Mon Sep 17 00:00:00 2001 From: Kyle Nusbaum Date: Wed, 8 Sep 2021 13:46:51 -0500 Subject: [PATCH 2/7] add/fix unit tests --- contrib/labstack/echo.v4/echotrace.go | 5 +- contrib/labstack/echo.v4/echotrace_test.go | 60 ++++++++++++++++++++++ contrib/labstack/echo/echotrace.go | 5 +- contrib/labstack/echo/echotrace_test.go | 60 ++++++++++++++++++++++ 4 files changed, 124 insertions(+), 6 deletions(-) diff --git a/contrib/labstack/echo.v4/echotrace.go b/contrib/labstack/echo.v4/echotrace.go index 6bad4ce055..cddea7112a 100644 --- a/contrib/labstack/echo.v4/echotrace.go +++ b/contrib/labstack/echo.v4/echotrace.go @@ -73,7 +73,6 @@ func Middleware(opts ...Option) echo.MiddlewareFunc { } err := next(c) if err != nil { - finishOpts = append(finishOpts, tracer.WithError(err)) // invokes the registered HTTP error handler c.Error(err) @@ -82,12 +81,12 @@ func Middleware(opts ...Option) echo.MiddlewareFunc { switch err := err.(type) { case *echo.HTTPError: if err.Code >= 500 { - span.SetTag(ext.Error, err) + finishOpts = append(finishOpts, tracer.WithError(err)) } span.SetTag(ext.HTTPCode, strconv.Itoa(err.Code)) default: // Any non-HTTPError errors appear as 5xx errors. - span.SetTag(ext.Error, err) + finishOpts = append(finishOpts, tracer.WithError(err)) span.SetTag(ext.HTTPCode, "500") } } else { diff --git a/contrib/labstack/echo.v4/echotrace_test.go b/contrib/labstack/echo.v4/echotrace_test.go index 52b2345d4f..5cfe68fd71 100644 --- a/contrib/labstack/echo.v4/echotrace_test.go +++ b/contrib/labstack/echo.v4/echotrace_test.go @@ -224,6 +224,66 @@ func TestErrorHandling(t *testing.T) { assert.Equal(ext.SpanKindServer, span.Tag(ext.SpanKind)) } +func TestStatusError(t *testing.T) { + for _, tt := range []struct { + err error + code string + handler func(c echo.Context) error + }{ + { + err: errors.New("oh no"), + code: "500", + handler: func(c echo.Context) error { + return errors.New("oh no") + }, + }, + { + err: echo.NewHTTPError(http.StatusInternalServerError, "my error message"), + code: "500", + handler: func(c echo.Context) error { + return echo.NewHTTPError(http.StatusInternalServerError, "my error message") + }, + }, + { + err: nil, + code: "400", + handler: func(c echo.Context) error { + return echo.NewHTTPError(http.StatusBadRequest, "my error message") + }, + }, + } { + t.Run("", func(t *testing.T) { + assert := assert.New(t) + mt := mocktracer.Start() + defer mt.Stop() + + router := echo.New() + router.Use(Middleware(WithServiceName("foobar"))) + router.GET("/err", tt.handler) + r := httptest.NewRequest("GET", "/err", nil) + w := httptest.NewRecorder() + router.ServeHTTP(w, r) + + spans := mt.FinishedSpans() + assert.Len(spans, 1) + span := spans[0] + assert.Equal("http.request", span.OperationName()) + assert.Equal(ext.SpanTypeWeb, span.Tag(ext.SpanType)) + assert.Equal("foobar", span.Tag(ext.ServiceName)) + assert.Contains(span.Tag(ext.ResourceName), "/err") + assert.Equal(tt.code, span.Tag(ext.HTTPCode)) + assert.Equal("GET", span.Tag(ext.HTTPMethod)) + err := span.Tag(ext.Error) + if tt.err != nil { + assert.NotNil(err) + assert.Equal(tt.err.Error(), err.(error).Error()) + } else { + assert.Nil(err) + } + }) + } +} + func TestGetSpanNotInstrumented(t *testing.T) { assert := assert.New(t) router := echo.New() diff --git a/contrib/labstack/echo/echotrace.go b/contrib/labstack/echo/echotrace.go index 9945724c04..a7584f4250 100644 --- a/contrib/labstack/echo/echotrace.go +++ b/contrib/labstack/echo/echotrace.go @@ -58,7 +58,6 @@ func Middleware(opts ...Option) echo.MiddlewareFunc { // serve the request to the next middleware err := next(c) if err != nil { - finishOpts = append(finishOpts, tracer.WithError(err)) // invokes the registered HTTP error handler c.Error(err) @@ -67,12 +66,12 @@ func Middleware(opts ...Option) echo.MiddlewareFunc { switch err := err.(type) { case *echo.HTTPError: if err.Code >= 500 { - span.SetTag(ext.Error, err) + finishOpts = append(finishOpts, tracer.WithError(err)) } span.SetTag(ext.HTTPCode, strconv.Itoa(err.Code)) default: // Any non-HTTPError errors appear as 5xx errors. - span.SetTag(ext.Error, err) + finishOpts = append(finishOpts, tracer.WithError(err)) span.SetTag(ext.HTTPCode, "500") } } else { diff --git a/contrib/labstack/echo/echotrace_test.go b/contrib/labstack/echo/echotrace_test.go index 6f9b098798..f67fe3c1da 100644 --- a/contrib/labstack/echo/echotrace_test.go +++ b/contrib/labstack/echo/echotrace_test.go @@ -219,6 +219,66 @@ func TestErrorHandling(t *testing.T) { assert.Equal(ext.SpanKindServer, span.Tag(ext.SpanKind)) } +func TestStatusError(t *testing.T) { + for _, tt := range []struct { + err error + code string + handler func(c echo.Context) error + }{ + { + err: errors.New("oh no"), + code: "500", + handler: func(c echo.Context) error { + return errors.New("oh no") + }, + }, + { + err: echo.NewHTTPError(http.StatusInternalServerError, "my error message"), + code: "500", + handler: func(c echo.Context) error { + return echo.NewHTTPError(http.StatusInternalServerError, "my error message") + }, + }, + { + err: nil, + code: "400", + handler: func(c echo.Context) error { + return echo.NewHTTPError(http.StatusBadRequest, "my error message") + }, + }, + } { + t.Run("", func(t *testing.T) { + assert := assert.New(t) + mt := mocktracer.Start() + defer mt.Stop() + + router := echo.New() + router.Use(Middleware(WithServiceName("foobar"))) + router.GET("/err", tt.handler) + r := httptest.NewRequest("GET", "/err", nil) + w := httptest.NewRecorder() + router.ServeHTTP(w, r) + + spans := mt.FinishedSpans() + assert.Len(spans, 1) + span := spans[0] + assert.Equal("http.request", span.OperationName()) + assert.Equal(ext.SpanTypeWeb, span.Tag(ext.SpanType)) + assert.Equal("foobar", span.Tag(ext.ServiceName)) + assert.Contains(span.Tag(ext.ResourceName), "/err") + assert.Equal(tt.code, span.Tag(ext.HTTPCode)) + assert.Equal("GET", span.Tag(ext.HTTPMethod)) + err := span.Tag(ext.Error) + if tt.err != nil { + assert.NotNil(err) + assert.Equal(tt.err.Error(), err.(error).Error()) + } else { + assert.Nil(err) + } + }) + } +} + func TestGetSpanNotInstrumented(t *testing.T) { assert := assert.New(t) router := echo.New() From 83001e7ae2310d7e467f10580db6a331c1ab3507 Mon Sep 17 00:00:00 2001 From: Kyle Nusbaum Date: Tue, 21 Sep 2021 10:44:52 -0500 Subject: [PATCH 3/7] add WithStatusCheck option --- contrib/labstack/echo.v4/echotrace.go | 18 ++++++- contrib/labstack/echo.v4/echotrace_test.go | 61 ++++++++++++++++++++-- contrib/labstack/echo.v4/option.go | 14 +++++ contrib/labstack/echo/echotrace.go | 18 ++++++- contrib/labstack/echo/echotrace_test.go | 57 ++++++++++++++++++-- contrib/labstack/echo/option.go | 16 ++++++ 6 files changed, 171 insertions(+), 13 deletions(-) diff --git a/contrib/labstack/echo.v4/echotrace.go b/contrib/labstack/echo.v4/echotrace.go index cddea7112a..ef24309158 100644 --- a/contrib/labstack/echo.v4/echotrace.go +++ b/contrib/labstack/echo.v4/echotrace.go @@ -7,7 +7,9 @@ package echo import ( + "fmt" "math" + "net/http" "strconv" "gopkg.in/DataDog/dd-trace-go.v1/contrib/internal/httptrace" @@ -80,16 +82,28 @@ func Middleware(opts ...Option) echo.MiddlewareFunc { // This is the best we can do. switch err := err.(type) { case *echo.HTTPError: - if err.Code >= 500 { + if cfg.isStatusError(err.Code) { finishOpts = append(finishOpts, tracer.WithError(err)) } span.SetTag(ext.HTTPCode, strconv.Itoa(err.Code)) default: // Any non-HTTPError errors appear as 5xx errors. - finishOpts = append(finishOpts, tracer.WithError(err)) + if cfg.isStatusError(500) { + finishOpts = append(finishOpts, tracer.WithError(err)) + } span.SetTag(ext.HTTPCode, "500") } + } else if status := c.Response().Status; status > 0 { + if cfg.isStatusError(status) { + // mark 5xx server error + finishOpts = append(finishOpts, tracer.WithError(fmt.Errorf("%d: %s", status, http.StatusText(status)))) + } + span.SetTag(ext.HTTPCode, strconv.Itoa(status)) } else { + if cfg.isStatusError(200) { + // mark 5xx server error + finishOpts = append(finishOpts, tracer.WithError(fmt.Errorf("%d: %s", 200, http.StatusText(200)))) + } span.SetTag(ext.HTTPCode, "200") } return err diff --git a/contrib/labstack/echo.v4/echotrace_test.go b/contrib/labstack/echo.v4/echotrace_test.go index 5cfe68fd71..1d1c7bf580 100644 --- a/contrib/labstack/echo.v4/echotrace_test.go +++ b/contrib/labstack/echo.v4/echotrace_test.go @@ -7,6 +7,7 @@ package echo import ( "errors" + "fmt" "io" "net/http" "net/http/httptest" @@ -226,9 +227,10 @@ func TestErrorHandling(t *testing.T) { func TestStatusError(t *testing.T) { for _, tt := range []struct { - err error - code string - handler func(c echo.Context) error + isStatusError func(statusCode int) bool + err error + code string + handler func(c echo.Context) error }{ { err: errors.New("oh no"), @@ -251,6 +253,49 @@ func TestStatusError(t *testing.T) { return echo.NewHTTPError(http.StatusBadRequest, "my error message") }, }, + { + isStatusError: func(statusCode int) bool { return statusCode >= 400 && statusCode < 500 }, + err: nil, + code: "500", + handler: func(c echo.Context) error { + return errors.New("oh no") + }, + }, + { + isStatusError: func(statusCode int) bool { return statusCode >= 400 && statusCode < 500 }, + err: nil, + code: "500", + handler: func(c echo.Context) error { + return echo.NewHTTPError(http.StatusInternalServerError, "my error message") + }, + }, + { + isStatusError: func(statusCode int) bool { return statusCode >= 400 }, + err: echo.NewHTTPError(http.StatusBadRequest, "my error message"), + code: "400", + handler: func(c echo.Context) error { + return echo.NewHTTPError(http.StatusBadRequest, "my error message") + }, + }, + { + isStatusError: func(statusCode int) bool { return statusCode >= 200 }, + err: fmt.Errorf("201: Created"), + code: "201", + handler: func(c echo.Context) error { + c.JSON(201, map[string]string{"status": "ok", "type": "test"}) + return nil + }, + }, + { + isStatusError: func(statusCode int) bool { return statusCode >= 200 }, + err: fmt.Errorf("200: OK"), + code: "200", + handler: func(c echo.Context) error { + // It's not clear if unset (0) status is possible naturally, but we can simulate that situation. + c.Response().Status = 0 + return nil + }, + }, } { t.Run("", func(t *testing.T) { assert := assert.New(t) @@ -258,7 +303,11 @@ func TestStatusError(t *testing.T) { defer mt.Stop() router := echo.New() - router.Use(Middleware(WithServiceName("foobar"))) + opts := []Option{WithServiceName("foobar")} + if tt.isStatusError != nil { + opts = append(opts, WithStatusCheck(tt.isStatusError)) + } + router.Use(Middleware(opts...)) router.GET("/err", tt.handler) r := httptest.NewRequest("GET", "/err", nil) w := httptest.NewRecorder() @@ -275,7 +324,9 @@ func TestStatusError(t *testing.T) { assert.Equal("GET", span.Tag(ext.HTTPMethod)) err := span.Tag(ext.Error) if tt.err != nil { - assert.NotNil(err) + if !assert.NotNil(err) { + return + } assert.Equal(tt.err.Error(), err.(error).Error()) } else { assert.Nil(err) diff --git a/contrib/labstack/echo.v4/option.go b/contrib/labstack/echo.v4/option.go index ea6bd4f97e..1d1f4ca66c 100644 --- a/contrib/labstack/echo.v4/option.go +++ b/contrib/labstack/echo.v4/option.go @@ -18,6 +18,7 @@ type config struct { analyticsRate float64 noDebugStack bool ignoreRequestFunc IgnoreRequestFunc + isStatusError func(statusCode int) bool } // Option represents an option that can be passed to Middleware. @@ -32,6 +33,7 @@ func defaults(cfg *config) { cfg.serviceName = svc } cfg.analyticsRate = math.NaN() + cfg.isStatusError = isServerError } // WithServiceName sets the given service name for the system. @@ -64,6 +66,7 @@ func WithAnalyticsRate(rate float64) Option { } } + // NoDebugStack prevents stack traces from being attached to spans finishing // with an error. This is useful in situations where errors are frequent and // performance is critical. @@ -79,4 +82,15 @@ func WithIgnoreRequest(ignoreRequestFunc IgnoreRequestFunc) Option { return func(cfg *config) { cfg.ignoreRequestFunc = ignoreRequestFunc } + +// WithStatusCheck specifies a function fn which reports whether the passed +// statusCode should be considered an error. +func WithStatusCheck(fn func(statusCode int) bool) Option { + return func(cfg *config) { + cfg.isStatusError = fn + } +} + +func isServerError(statusCode int) bool { + return statusCode >= 500 && statusCode < 600 } diff --git a/contrib/labstack/echo/echotrace.go b/contrib/labstack/echo/echotrace.go index a7584f4250..7106a2eb62 100644 --- a/contrib/labstack/echo/echotrace.go +++ b/contrib/labstack/echo/echotrace.go @@ -7,7 +7,9 @@ package echo import ( + "fmt" "math" + "net/http" "strconv" "gopkg.in/DataDog/dd-trace-go.v1/contrib/internal/httptrace" @@ -65,16 +67,28 @@ func Middleware(opts ...Option) echo.MiddlewareFunc { // This is the best we can do. switch err := err.(type) { case *echo.HTTPError: - if err.Code >= 500 { + if cfg.isStatusError(err.Code) { finishOpts = append(finishOpts, tracer.WithError(err)) } span.SetTag(ext.HTTPCode, strconv.Itoa(err.Code)) default: // Any non-HTTPError errors appear as 5xx errors. - finishOpts = append(finishOpts, tracer.WithError(err)) + if cfg.isStatusError(500) { + finishOpts = append(finishOpts, tracer.WithError(err)) + } span.SetTag(ext.HTTPCode, "500") } + } else if status := c.Response().Status; status > 0 { + if cfg.isStatusError(status) { + // mark 5xx server error + finishOpts = append(finishOpts, tracer.WithError(fmt.Errorf("%d: %s", status, http.StatusText(status)))) + } + span.SetTag(ext.HTTPCode, strconv.Itoa(status)) } else { + if cfg.isStatusError(200) { + // mark 5xx server error + finishOpts = append(finishOpts, tracer.WithError(fmt.Errorf("%d: %s", 200, http.StatusText(200)))) + } span.SetTag(ext.HTTPCode, "200") } return err diff --git a/contrib/labstack/echo/echotrace_test.go b/contrib/labstack/echo/echotrace_test.go index f67fe3c1da..b7f3a3ab73 100644 --- a/contrib/labstack/echo/echotrace_test.go +++ b/contrib/labstack/echo/echotrace_test.go @@ -7,6 +7,7 @@ package echo import ( "errors" + "fmt" "net/http" "net/http/httptest" "testing" @@ -221,9 +222,10 @@ func TestErrorHandling(t *testing.T) { func TestStatusError(t *testing.T) { for _, tt := range []struct { - err error - code string - handler func(c echo.Context) error + isStatusError func(statusCode int) bool + err error + code string + handler func(c echo.Context) error }{ { err: errors.New("oh no"), @@ -246,6 +248,49 @@ func TestStatusError(t *testing.T) { return echo.NewHTTPError(http.StatusBadRequest, "my error message") }, }, + { + isStatusError: func(statusCode int) bool { return statusCode >= 400 && statusCode < 500 }, + err: nil, + code: "500", + handler: func(c echo.Context) error { + return errors.New("oh no") + }, + }, + { + isStatusError: func(statusCode int) bool { return statusCode >= 400 && statusCode < 500 }, + err: nil, + code: "500", + handler: func(c echo.Context) error { + return echo.NewHTTPError(http.StatusInternalServerError, "my error message") + }, + }, + { + isStatusError: func(statusCode int) bool { return statusCode >= 400 }, + err: echo.NewHTTPError(http.StatusBadRequest, "my error message"), + code: "400", + handler: func(c echo.Context) error { + return echo.NewHTTPError(http.StatusBadRequest, "my error message") + }, + }, + { + isStatusError: func(statusCode int) bool { return statusCode >= 200 }, + err: fmt.Errorf("201: Created"), + code: "201", + handler: func(c echo.Context) error { + c.JSON(201, map[string]string{"status": "ok", "type": "test"}) + return nil + }, + }, + { + isStatusError: func(statusCode int) bool { return statusCode >= 200 }, + err: fmt.Errorf("200: OK"), + code: "200", + handler: func(c echo.Context) error { + // It's not clear if unset (0) status is possible naturally, but we can simulate that situation. + c.Response().Status = 0 + return nil + }, + }, } { t.Run("", func(t *testing.T) { assert := assert.New(t) @@ -253,7 +298,11 @@ func TestStatusError(t *testing.T) { defer mt.Stop() router := echo.New() - router.Use(Middleware(WithServiceName("foobar"))) + opts := []Option{WithServiceName("foobar")} + if tt.isStatusError != nil { + opts = append(opts, WithStatusCheck(tt.isStatusError)) + } + router.Use(Middleware(opts...)) router.GET("/err", tt.handler) r := httptest.NewRequest("GET", "/err", nil) w := httptest.NewRecorder() diff --git a/contrib/labstack/echo/option.go b/contrib/labstack/echo/option.go index 4af218c5bb..a3ae547ed6 100644 --- a/contrib/labstack/echo/option.go +++ b/contrib/labstack/echo/option.go @@ -16,6 +16,7 @@ type config struct { serviceName string analyticsRate float64 noDebugStack bool + isStatusError func(statusCode int) bool } // Option represents an option that can be passed to Middleware. @@ -31,6 +32,7 @@ func defaults(cfg *config) { } else { cfg.analyticsRate = math.NaN() } + cfg.isStatusError = isServerError } // WithServiceName sets the given service name for the system. @@ -63,6 +65,7 @@ func WithAnalyticsRate(rate float64) Option { } } +<<<<<<< HEAD // NoDebugStack prevents stack traces from being attached to spans finishing // with an error. This is useful in situations where errors are frequent and // performance is critical. @@ -71,3 +74,16 @@ func NoDebugStack() Option { cfg.noDebugStack = true } } +======= +// WithStatusCheck specifies a function fn which reports whether the passed +// statusCode should be considered an error. +func WithStatusCheck(fn func(statusCode int) bool) Option { + return func(cfg *config) { + cfg.isStatusError = fn + } +} + +func isServerError(statusCode int) bool { + return statusCode >= 500 && statusCode < 600 +} +>>>>>>> add WithStatusCheck option From 61a12f32b292bc6d45aba67c23811d388c9be4d3 Mon Sep 17 00:00:00 2001 From: Kyle Nusbaum Date: Thu, 8 Dec 2022 13:12:38 -0600 Subject: [PATCH 4/7] fix and update --- contrib/labstack/echo.v4/echotrace.go | 3 ++- contrib/labstack/echo.v4/option.go | 4 ++-- contrib/labstack/echo/echotrace.go | 7 ++++++- contrib/labstack/echo/echotrace_test.go | 9 +++++++-- contrib/labstack/echo/option.go | 4 +--- 5 files changed, 18 insertions(+), 9 deletions(-) diff --git a/contrib/labstack/echo.v4/echotrace.go b/contrib/labstack/echo.v4/echotrace.go index ef24309158..15d09eb16d 100644 --- a/contrib/labstack/echo.v4/echotrace.go +++ b/contrib/labstack/echo.v4/echotrace.go @@ -63,7 +63,8 @@ func Middleware(opts ...Option) echo.MiddlewareFunc { span, ctx := httptrace.StartRequestSpan(request, opts...) defer func() { - httptrace.FinishRequestSpan(span, c.Response().Status, finishOpts...) + //httptrace.FinishRequestSpan(span, c.Response().Status, finishOpts...) + span.Finish(finishOpts...) }() // pass the span through the request context diff --git a/contrib/labstack/echo.v4/option.go b/contrib/labstack/echo.v4/option.go index 1d1f4ca66c..d634b3136b 100644 --- a/contrib/labstack/echo.v4/option.go +++ b/contrib/labstack/echo.v4/option.go @@ -18,7 +18,7 @@ type config struct { analyticsRate float64 noDebugStack bool ignoreRequestFunc IgnoreRequestFunc - isStatusError func(statusCode int) bool + isStatusError func(statusCode int) bool } // Option represents an option that can be passed to Middleware. @@ -66,7 +66,6 @@ func WithAnalyticsRate(rate float64) Option { } } - // NoDebugStack prevents stack traces from being attached to spans finishing // with an error. This is useful in situations where errors are frequent and // performance is critical. @@ -82,6 +81,7 @@ func WithIgnoreRequest(ignoreRequestFunc IgnoreRequestFunc) Option { return func(cfg *config) { cfg.ignoreRequestFunc = ignoreRequestFunc } +} // WithStatusCheck specifies a function fn which reports whether the passed // statusCode should be considered an error. diff --git a/contrib/labstack/echo/echotrace.go b/contrib/labstack/echo/echotrace.go index 7106a2eb62..ffd7244cdd 100644 --- a/contrib/labstack/echo/echotrace.go +++ b/contrib/labstack/echo/echotrace.go @@ -51,7 +51,8 @@ func Middleware(opts ...Option) echo.MiddlewareFunc { span, ctx := httptrace.StartRequestSpan(request, opts...) defer func() { - httptrace.FinishRequestSpan(span, c.Response().Status, finishOpts...) + //httptrace.FinishRequestSpan(span, c.Response().Status, finishOpts...) + span.Finish(finishOpts...) }() // pass the span through the request context @@ -68,12 +69,14 @@ func Middleware(opts ...Option) echo.MiddlewareFunc { switch err := err.(type) { case *echo.HTTPError: if cfg.isStatusError(err.Code) { + fmt.Printf("FINISH OPTS ADDING ERROR!\n") finishOpts = append(finishOpts, tracer.WithError(err)) } span.SetTag(ext.HTTPCode, strconv.Itoa(err.Code)) default: // Any non-HTTPError errors appear as 5xx errors. if cfg.isStatusError(500) { + fmt.Printf("FINISH OPTS ADDING ERROR!\n") finishOpts = append(finishOpts, tracer.WithError(err)) } span.SetTag(ext.HTTPCode, "500") @@ -81,12 +84,14 @@ func Middleware(opts ...Option) echo.MiddlewareFunc { } else if status := c.Response().Status; status > 0 { if cfg.isStatusError(status) { // mark 5xx server error + fmt.Printf("FINISH OPTS ADDING ERROR!\n") finishOpts = append(finishOpts, tracer.WithError(fmt.Errorf("%d: %s", status, http.StatusText(status)))) } span.SetTag(ext.HTTPCode, strconv.Itoa(status)) } else { if cfg.isStatusError(200) { // mark 5xx server error + fmt.Printf("FINISH OPTS ADDING ERROR!\n") finishOpts = append(finishOpts, tracer.WithError(fmt.Errorf("%d: %s", 200, http.StatusText(200)))) } span.SetTag(ext.HTTPCode, "200") diff --git a/contrib/labstack/echo/echotrace_test.go b/contrib/labstack/echo/echotrace_test.go index b7f3a3ab73..4e70c23c54 100644 --- a/contrib/labstack/echo/echotrace_test.go +++ b/contrib/labstack/echo/echotrace_test.go @@ -18,6 +18,7 @@ import ( "github.com/labstack/echo" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestChildSpan(t *testing.T) { @@ -175,6 +176,7 @@ func TestError(t *testing.T) { assert.Equal("http.request", span.OperationName()) assert.Equal("foobar", span.Tag(ext.ServiceName)) assert.Equal("500", span.Tag(ext.HTTPCode)) + require.NotNil(t, span.Tag(ext.Error)) assert.Equal(wantErr.Error(), span.Tag(ext.Error).(error).Error()) assert.Equal("labstack/echo", span.Tag(ext.Component)) assert.Equal(ext.SpanKindServer, span.Tag(ext.SpanKind)) @@ -215,6 +217,7 @@ func TestErrorHandling(t *testing.T) { assert.Equal("http.request", span.OperationName()) assert.Equal("foobar", span.Tag(ext.ServiceName)) assert.Equal("500", span.Tag(ext.HTTPCode)) + require.NotNil(t, span.Tag(ext.Error)) assert.Equal(wantErr.Error(), span.Tag(ext.Error).(error).Error()) assert.Equal("labstack/echo", span.Tag(ext.Component)) assert.Equal(ext.SpanKindServer, span.Tag(ext.SpanKind)) @@ -248,7 +251,7 @@ func TestStatusError(t *testing.T) { return echo.NewHTTPError(http.StatusBadRequest, "my error message") }, }, - { + { //03 isStatusError: func(statusCode int) bool { return statusCode >= 400 && statusCode < 500 }, err: nil, code: "500", @@ -256,7 +259,7 @@ func TestStatusError(t *testing.T) { return errors.New("oh no") }, }, - { + { //04 isStatusError: func(statusCode int) bool { return statusCode >= 400 && statusCode < 500 }, err: nil, code: "500", @@ -320,6 +323,7 @@ func TestStatusError(t *testing.T) { err := span.Tag(ext.Error) if tt.err != nil { assert.NotNil(err) + require.NotNil(t, span.Tag(ext.Error)) assert.Equal(tt.err.Error(), err.(error).Error()) } else { assert.Nil(err) @@ -380,6 +384,7 @@ func TestNoDebugStack(t *testing.T) { assert.Len(spans, 1) span := spans[0] + require.NotNil(t, span.Tag(ext.Error)) assert.Equal(wantErr.Error(), span.Tag(ext.Error).(error).Error()) assert.Equal("", span.Tag(ext.ErrorStack)) assert.Equal("labstack/echo", span.Tag(ext.Component)) diff --git a/contrib/labstack/echo/option.go b/contrib/labstack/echo/option.go index a3ae547ed6..959a0627d6 100644 --- a/contrib/labstack/echo/option.go +++ b/contrib/labstack/echo/option.go @@ -65,7 +65,6 @@ func WithAnalyticsRate(rate float64) Option { } } -<<<<<<< HEAD // NoDebugStack prevents stack traces from being attached to spans finishing // with an error. This is useful in situations where errors are frequent and // performance is critical. @@ -74,7 +73,7 @@ func NoDebugStack() Option { cfg.noDebugStack = true } } -======= + // WithStatusCheck specifies a function fn which reports whether the passed // statusCode should be considered an error. func WithStatusCheck(fn func(statusCode int) bool) Option { @@ -86,4 +85,3 @@ func WithStatusCheck(fn func(statusCode int) bool) Option { func isServerError(statusCode int) bool { return statusCode >= 500 && statusCode < 600 } ->>>>>>> add WithStatusCheck option From 47535f9ffc361822af4ea91b84976fd589769477 Mon Sep 17 00:00:00 2001 From: Kyle Nusbaum Date: Thu, 8 Dec 2022 13:18:34 -0600 Subject: [PATCH 5/7] cleanup --- contrib/labstack/echo.v4/echotrace.go | 8 +++----- contrib/labstack/echo/echotrace.go | 8 +------- 2 files changed, 4 insertions(+), 12 deletions(-) diff --git a/contrib/labstack/echo.v4/echotrace.go b/contrib/labstack/echo.v4/echotrace.go index 15d09eb16d..3b1e3ea8f5 100644 --- a/contrib/labstack/echo.v4/echotrace.go +++ b/contrib/labstack/echo.v4/echotrace.go @@ -63,17 +63,17 @@ func Middleware(opts ...Option) echo.MiddlewareFunc { span, ctx := httptrace.StartRequestSpan(request, opts...) defer func() { - //httptrace.FinishRequestSpan(span, c.Response().Status, finishOpts...) span.Finish(finishOpts...) }() // pass the span through the request context c.SetRequest(request.WithContext(ctx)) - // serve the request to the next middleware + if appsecEnabled { afterMiddleware := useAppSec(c, span) defer afterMiddleware() } + // serve the request to the next middleware err := next(c) if err != nil { // invokes the registered HTTP error handler @@ -88,7 +88,7 @@ func Middleware(opts ...Option) echo.MiddlewareFunc { } span.SetTag(ext.HTTPCode, strconv.Itoa(err.Code)) default: - // Any non-HTTPError errors appear as 5xx errors. + // Any error that is not an *echo.HTTPError will be treated as an error with 500 status code. if cfg.isStatusError(500) { finishOpts = append(finishOpts, tracer.WithError(err)) } @@ -96,13 +96,11 @@ func Middleware(opts ...Option) echo.MiddlewareFunc { } } else if status := c.Response().Status; status > 0 { if cfg.isStatusError(status) { - // mark 5xx server error finishOpts = append(finishOpts, tracer.WithError(fmt.Errorf("%d: %s", status, http.StatusText(status)))) } span.SetTag(ext.HTTPCode, strconv.Itoa(status)) } else { if cfg.isStatusError(200) { - // mark 5xx server error finishOpts = append(finishOpts, tracer.WithError(fmt.Errorf("%d: %s", 200, http.StatusText(200)))) } span.SetTag(ext.HTTPCode, "200") diff --git a/contrib/labstack/echo/echotrace.go b/contrib/labstack/echo/echotrace.go index ffd7244cdd..35fbacbdd0 100644 --- a/contrib/labstack/echo/echotrace.go +++ b/contrib/labstack/echo/echotrace.go @@ -69,29 +69,23 @@ func Middleware(opts ...Option) echo.MiddlewareFunc { switch err := err.(type) { case *echo.HTTPError: if cfg.isStatusError(err.Code) { - fmt.Printf("FINISH OPTS ADDING ERROR!\n") finishOpts = append(finishOpts, tracer.WithError(err)) } span.SetTag(ext.HTTPCode, strconv.Itoa(err.Code)) default: - // Any non-HTTPError errors appear as 5xx errors. + // Any error that is not an *echo.HTTPError will be treated as an error with 500 status code. if cfg.isStatusError(500) { - fmt.Printf("FINISH OPTS ADDING ERROR!\n") finishOpts = append(finishOpts, tracer.WithError(err)) } span.SetTag(ext.HTTPCode, "500") } } else if status := c.Response().Status; status > 0 { if cfg.isStatusError(status) { - // mark 5xx server error - fmt.Printf("FINISH OPTS ADDING ERROR!\n") finishOpts = append(finishOpts, tracer.WithError(fmt.Errorf("%d: %s", status, http.StatusText(status)))) } span.SetTag(ext.HTTPCode, strconv.Itoa(status)) } else { if cfg.isStatusError(200) { - // mark 5xx server error - fmt.Printf("FINISH OPTS ADDING ERROR!\n") finishOpts = append(finishOpts, tracer.WithError(fmt.Errorf("%d: %s", 200, http.StatusText(200)))) } span.SetTag(ext.HTTPCode, "200") From be36744445fe5bff194e5499793b2d6d7f12ab69 Mon Sep 17 00:00:00 2001 From: Kyle Nusbaum Date: Thu, 8 Dec 2022 16:30:44 -0600 Subject: [PATCH 6/7] address PR comments --- contrib/labstack/echo.v4/echotrace.go | 11 ++++++----- contrib/labstack/echo.v4/echotrace_test.go | 12 ++++++------ contrib/labstack/echo/echotrace.go | 11 ++++++----- 3 files changed, 18 insertions(+), 16 deletions(-) diff --git a/contrib/labstack/echo.v4/echotrace.go b/contrib/labstack/echo.v4/echotrace.go index 3b1e3ea8f5..1628711d9b 100644 --- a/contrib/labstack/echo.v4/echotrace.go +++ b/contrib/labstack/echo.v4/echotrace.go @@ -7,6 +7,7 @@ package echo import ( + "errors" "fmt" "math" "net/http" @@ -81,13 +82,13 @@ func Middleware(opts ...Option) echo.MiddlewareFunc { // It is impossible to determine what the final status code of a request is in echo. // This is the best we can do. - switch err := err.(type) { - case *echo.HTTPError: - if cfg.isStatusError(err.Code) { + var echoErr *echo.HTTPError + if errors.As(err, &echoErr) { + if cfg.isStatusError(echoErr.Code) { finishOpts = append(finishOpts, tracer.WithError(err)) } - span.SetTag(ext.HTTPCode, strconv.Itoa(err.Code)) - default: + span.SetTag(ext.HTTPCode, strconv.Itoa(echoErr.Code)) + } else { // Any error that is not an *echo.HTTPError will be treated as an error with 500 status code. if cfg.isStatusError(500) { finishOpts = append(finishOpts, tracer.WithError(err)) diff --git a/contrib/labstack/echo.v4/echotrace_test.go b/contrib/labstack/echo.v4/echotrace_test.go index 1d1c7bf580..c3894e1fbf 100644 --- a/contrib/labstack/echo.v4/echotrace_test.go +++ b/contrib/labstack/echo.v4/echotrace_test.go @@ -79,7 +79,7 @@ func TestTrace200(t *testing.T) { assert.True(traced) spans := mt.FinishedSpans() - assert.Len(spans, 1) + require.Len(t, spans, 1) span := spans[0] assert.Equal("http.request", span.OperationName()) @@ -127,7 +127,7 @@ func TestTraceAnalytics(t *testing.T) { assert.True(traced) spans := mt.FinishedSpans() - assert.Len(spans, 1) + require.Len(t, spans, 1) span := spans[0] assert.Equal("http.request", span.OperationName()) @@ -174,7 +174,7 @@ func TestError(t *testing.T) { assert.True(traced) spans := mt.FinishedSpans() - assert.Len(spans, 1) + require.Len(t, spans, 1) span := spans[0] assert.Equal("http.request", span.OperationName()) @@ -214,7 +214,7 @@ func TestErrorHandling(t *testing.T) { assert.True(traced) spans := mt.FinishedSpans() - assert.Len(spans, 1) + require.Len(t, spans, 1) span := spans[0] assert.Equal("http.request", span.OperationName()) @@ -314,7 +314,7 @@ func TestStatusError(t *testing.T) { router.ServeHTTP(w, r) spans := mt.FinishedSpans() - assert.Len(spans, 1) + require.Len(t, spans, 1) span := spans[0] assert.Equal("http.request", span.OperationName()) assert.Equal(ext.SpanTypeWeb, span.Tag(ext.SpanType)) @@ -384,7 +384,7 @@ func TestNoDebugStack(t *testing.T) { assert.True(traced) spans := mt.FinishedSpans() - assert.Len(spans, 1) + require.Len(t, spans, 1) span := spans[0] assert.Equal(wantErr.Error(), span.Tag(ext.Error).(error).Error()) diff --git a/contrib/labstack/echo/echotrace.go b/contrib/labstack/echo/echotrace.go index 35fbacbdd0..d1614b6766 100644 --- a/contrib/labstack/echo/echotrace.go +++ b/contrib/labstack/echo/echotrace.go @@ -7,6 +7,7 @@ package echo import ( + "errors" "fmt" "math" "net/http" @@ -66,13 +67,13 @@ func Middleware(opts ...Option) echo.MiddlewareFunc { // It is impossible to determine what the final status code of a request is in echo. // This is the best we can do. - switch err := err.(type) { - case *echo.HTTPError: - if cfg.isStatusError(err.Code) { + var echoErr *echo.HTTPError + if errors.As(err, &echoErr) { + if cfg.isStatusError(echoErr.Code) { finishOpts = append(finishOpts, tracer.WithError(err)) } - span.SetTag(ext.HTTPCode, strconv.Itoa(err.Code)) - default: + span.SetTag(ext.HTTPCode, strconv.Itoa(echoErr.Code)) + } else { // Any error that is not an *echo.HTTPError will be treated as an error with 500 status code. if cfg.isStatusError(500) { finishOpts = append(finishOpts, tracer.WithError(err)) From 649a37b8dc775bda4a56efc8c079900e8d947376 Mon Sep 17 00:00:00 2001 From: Kyle Nusbaum Date: Thu, 8 Dec 2022 16:43:07 -0600 Subject: [PATCH 7/7] cleanup --- contrib/labstack/echo/echotrace_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contrib/labstack/echo/echotrace_test.go b/contrib/labstack/echo/echotrace_test.go index 4e70c23c54..625811a81d 100644 --- a/contrib/labstack/echo/echotrace_test.go +++ b/contrib/labstack/echo/echotrace_test.go @@ -251,7 +251,7 @@ func TestStatusError(t *testing.T) { return echo.NewHTTPError(http.StatusBadRequest, "my error message") }, }, - { //03 + { isStatusError: func(statusCode int) bool { return statusCode >= 400 && statusCode < 500 }, err: nil, code: "500", @@ -259,7 +259,7 @@ func TestStatusError(t *testing.T) { return errors.New("oh no") }, }, - { //04 + { isStatusError: func(statusCode int) bool { return statusCode >= 400 && statusCode < 500 }, err: nil, code: "500",