Handle echo 400s as not errors in datadog APM

contrib/labstack/echo.v4/echotrace.go

@@ -7,7 +7,10 @@
 package echo
 
 import (
+	"fmt"
 	"math"
+	"net/http"
+	"strconv"
 
 	"gopkg.in/DataDog/dd-trace-go.v1/contrib/internal/httptrace"
 	"gopkg.in/DataDog/dd-trace-go.v1/ddtrace"
@@ -70,11 +73,35 @@ 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)
+				// 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) {
+						// mark 5xx server error
+						span.SetTag(ext.Error, err)
+					}
+					span.SetTag(ext.HTTPCode, strconv.Itoa(err.Code))
+				default:
+					// Any non-HTTPError errors appear as 5xx errors.
+					if cfg.isStatusError(500) {
+						span.SetTag(ext.Error, err)
+					}
+					span.SetTag(ext.HTTPCode, "500")
+				}
+			} else if status := c.Response().Status; status > 0 {
+				if cfg.isStatusError(status) {
+					// mark 5xx server error
+					span.SetTag(ext.Error, fmt.Errorf("%d: %s", status, http.StatusText(status)))
+				}
+				span.SetTag(ext.HTTPCode, strconv.Itoa(status))
+			} else {
+				if cfg.isStatusError(200) {
+					// mark 5xx server error
+					span.SetTag(ext.Error, fmt.Errorf("%d: %s", 200, http.StatusText(200)))
+				}
+				span.SetTag(ext.HTTPCode, "200")
 			}
-
 			return err
 		}
 	}

contrib/labstack/echo.v4/echotrace_test.go

@@ -7,6 +7,7 @@ package echo
 
 import (
 	"errors"
+	"fmt"
 	"io"
 	"net/http"
 	"net/http/httptest"
@@ -141,79 +142,113 @@ func TestTraceAnalytics(t *testing.T) {
 }
 
 func TestError(t *testing.T) {
-	assert := assert.New(t)
-	mt := mocktracer.Start()
-	defer mt.Stop()
-	var called, traced bool
-
-	// setup
-	router := echo.New()
-	router.Use(Middleware(WithServiceName("foobar")))
-	wantErr := errors.New("oh no")
-
-	// a handler with an error and make the requests
-	router.GET("/err", func(c echo.Context) error {
-		_, traced = tracer.SpanFromContext(c.Request().Context())
-		called = true
-
-		err := wantErr
-		c.Error(err)
-		return err
-	})
-	r := httptest.NewRequest("GET", "/err", nil)
-	w := httptest.NewRecorder()
-	router.ServeHTTP(w, r)
-
-	// verify the errors and status are correct
-	assert.True(called)
-	assert.True(traced)
-
-	spans := mt.FinishedSpans()
-	assert.Len(spans, 1)
-
-	span := spans[0]
-	assert.Equal("http.request", span.OperationName())
-	assert.Equal("foobar", span.Tag(ext.ServiceName))
-	assert.Equal("500", span.Tag(ext.HTTPCode))
-	assert.Equal(wantErr.Error(), span.Tag(ext.Error).(error).Error())
-}
-
-func TestErrorHandling(t *testing.T) {
-	assert := assert.New(t)
-	mt := mocktracer.Start()
-	defer mt.Stop()
-	var called, traced bool
+	for _, tt := range []struct {
+		isStatusError func(statusCode int) bool
+		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")
+			},
+		},
+		{
+			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)
+			mt := mocktracer.Start()
+			defer mt.Stop()
 
-	// setup
-	router := echo.New()
-	router.HTTPErrorHandler = func(err error, ctx echo.Context) {
-		ctx.Response().WriteHeader(http.StatusInternalServerError)
+			router := echo.New()
+			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()
+			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 {
+				if !assert.NotNil(err) {
+					return
+				}
+				assert.Equal(tt.err.Error(), err.(error).Error())
+			} else {
+				assert.Nil(err)
+			}
+		})
 	}
-	router.Use(Middleware(WithServiceName("foobar")))
-	wantErr := errors.New("oh no")
-
-	// a handler with an error and make the requests
-	router.GET("/err", func(c echo.Context) error {
-		_, traced = tracer.SpanFromContext(c.Request().Context())
-		called = true
-		return wantErr
-	})
-	r := httptest.NewRequest("GET", "/err", nil)
-	w := httptest.NewRecorder()
-	router.ServeHTTP(w, r)
-
-	// verify the errors and status are correct
-	assert.True(called)
-	assert.True(traced)
-
-	spans := mt.FinishedSpans()
-	assert.Len(spans, 1)
-
-	span := spans[0]
-	assert.Equal("http.request", span.OperationName())
-	assert.Equal("foobar", span.Tag(ext.ServiceName))
-	assert.Equal("500", span.Tag(ext.HTTPCode))
-	assert.Equal(wantErr.Error(), span.Tag(ext.Error).(error).Error())
 }
 
 func TestGetSpanNotInstrumented(t *testing.T) {

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.
@@ -80,3 +82,15 @@ func WithIgnoreRequest(ignoreRequestFunc IgnoreRequestFunc) Option {
 		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
+}

contrib/labstack/echo/echotrace.go

@@ -7,7 +7,10 @@
 package echo
 
 import (
+	"fmt"
 	"math"
+	"net/http"
+	"strconv"
 
 	"gopkg.in/DataDog/dd-trace-go.v1/contrib/internal/httptrace"
 	"gopkg.in/DataDog/dd-trace-go.v1/ddtrace"
@@ -55,11 +58,35 @@ 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)
+				// 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) {
+						// mark 5xx server error
+						span.SetTag(ext.Error, err)
+					}
+					span.SetTag(ext.HTTPCode, strconv.Itoa(err.Code))
+				default:
+					// Any non-HTTPError errors appear as 5xx errors.
+					if cfg.isStatusError(500) {
+						span.SetTag(ext.Error, err)
+					}
+					span.SetTag(ext.HTTPCode, "500")
+				}
+			} else if status := c.Response().Status; status > 0 {
+				if cfg.isStatusError(status) {
+					// mark 5xx server error
+					span.SetTag(ext.Error, fmt.Errorf("%d: %s", status, http.StatusText(status)))
+				}
+				span.SetTag(ext.HTTPCode, strconv.Itoa(status))
+			} else {
+				if cfg.isStatusError(200) {
+					// mark 5xx server error
+					span.SetTag(ext.Error, fmt.Errorf("%d: %s", 200, http.StatusText(200)))
+				}
+				span.SetTag(ext.HTTPCode, "200")
 			}
-
 			return err
 		}
 	}