diff --git a/go.mod b/go.mod index 067e3a1..39540e6 100644 --- a/go.mod +++ b/go.mod @@ -18,3 +18,4 @@ require ( github.com/uber/jaeger-client-go v2.25.0+incompatible github.com/uber/jaeger-lib v2.4.0+incompatible // indirect ) + diff --git a/jaegertracing/jaegertracing.go b/jaegertracing/jaegertracing.go index 70385cd..c3dbdcc 100644 --- a/jaegertracing/jaegertracing.go +++ b/jaegertracing/jaegertracing.go @@ -22,6 +22,7 @@ package jaegertracing import ( "bytes" + "errors" "fmt" "io" "io/ioutil" @@ -48,8 +49,8 @@ type ( // OpenTracing Tracer instance which should be got before Tracer opentracing.Tracer - // componentName used for describing the tracing component name - componentName string + // ComponentName used for describing the tracing component name + ComponentName string // add req body & resp body to tracing tags IsBodyDump bool @@ -60,7 +61,7 @@ var ( // DefaultTraceConfig is the default Trace middleware config. DefaultTraceConfig = TraceConfig{ Skipper: middleware.DefaultSkipper, - componentName: defaultComponentName, + ComponentName: defaultComponentName, IsBodyDump: false, } ) @@ -102,7 +103,7 @@ func New(e *echo.Echo, skipper middleware.Skipper) io.Closer { func Trace(tracer opentracing.Tracer) echo.MiddlewareFunc { c := DefaultTraceConfig c.Tracer = tracer - c.componentName = defaultComponentName + c.ComponentName = defaultComponentName return TraceWithConfig(c) } @@ -115,8 +116,8 @@ func TraceWithConfig(config TraceConfig) echo.MiddlewareFunc { if config.Skipper == nil { config.Skipper = middleware.DefaultSkipper } - if config.componentName == "" { - config.componentName = defaultComponentName + if config.ComponentName == "" { + config.ComponentName = defaultComponentName } return func(next echo.HandlerFunc) echo.HandlerFunc { @@ -138,7 +139,7 @@ func TraceWithConfig(config TraceConfig) echo.MiddlewareFunc { ext.HTTPMethod.Set(sp, req.Method) ext.HTTPUrl.Set(sp, req.URL.String()) - ext.Component.Set(sp, config.componentName) + ext.Component.Set(sp, config.ComponentName) // Dump request & response body resBody := new(bytes.Buffer) @@ -161,9 +162,30 @@ func TraceWithConfig(config TraceConfig) echo.MiddlewareFunc { req = req.WithContext(opentracing.ContextWithSpan(req.Context(), sp)) c.SetRequest(req) + var err error defer func() { - status := c.Response().Status committed := c.Response().Committed + status := c.Response().Status + + if err != nil { + var httpError *echo.HTTPError + if errors.As(err, &httpError) { + if httpError.Code != 0 { + status = httpError.Code + } + sp.SetTag("error.message", httpError.Message) + } else { + sp.SetTag("error.message", err.Error()) + } + if status == http.StatusOK { + // this is ugly workaround for cases when httpError.code == 0 or error was not httpError and status + // in request was 200 (OK). In these cases replace status with something that represents an error + // it could be that error handlers or middlewares up in chain will output different status code to + // client. but at least we send something better than 200 to jaeger + status = http.StatusInternalServerError + } + } + ext.HTTPStatusCode.Set(sp, uint16(status)) if status >= http.StatusInternalServerError || !committed { ext.Error.Set(sp, true) @@ -176,7 +198,8 @@ func TraceWithConfig(config TraceConfig) echo.MiddlewareFunc { sp.Finish() }() - return next(c) + err = next(c) + return err } } } diff --git a/jaegertracing/jaegertracing_test.go b/jaegertracing/jaegertracing_test.go index 5f2dbde..8f60530 100644 --- a/jaegertracing/jaegertracing_test.go +++ b/jaegertracing/jaegertracing_test.go @@ -3,6 +3,7 @@ package jaegertracing import ( "bytes" "errors" + "fmt" "net/http" "net/http/httptest" "testing" @@ -117,15 +118,62 @@ func TestTraceWithDefaultConfig(t *testing.T) { e := echo.New() e.Use(Trace(tracer)) - req := httptest.NewRequest(http.MethodGet, "/", nil) - rec := httptest.NewRecorder() - e.ServeHTTP(rec, req) - assert.Equal(t, "GET", tracer.currentSpan().getTag("http.method")) - assert.Equal(t, "/", tracer.currentSpan().getTag("http.url")) - assert.Equal(t, defaultComponentName, tracer.currentSpan().getTag("component")) - assert.Equal(t, uint16(200), tracer.currentSpan().getTag("http.status_code")) - assert.Equal(t, true, tracer.currentSpan().getTag("error")) + e.GET("/hello", func(c echo.Context) error { + return c.String(http.StatusOK, "world") + }) + + e.GET("/giveme400", func(c echo.Context) error { + return echo.NewHTTPError(http.StatusBadRequest, "baaaad request") + }) + + e.GET("/givemeerror", func(c echo.Context) error { + return fmt.Errorf("internal stuff went wrong") + }) + + t.Run("successful call", func(t *testing.T) { + req := httptest.NewRequest(http.MethodGet, "/hello", nil) + rec := httptest.NewRecorder() + e.ServeHTTP(rec, req) + + assert.Equal(t, "GET", tracer.currentSpan().getTag("http.method")) + assert.Equal(t, "/hello", tracer.currentSpan().getTag("http.url")) + assert.Equal(t, defaultComponentName, tracer.currentSpan().getTag("component")) + assert.Equal(t, uint16(200), tracer.currentSpan().getTag("http.status_code")) + assert.NotEqual(t, true, tracer.currentSpan().getTag("error")) + }) + + t.Run("error from echo", func(t *testing.T) { + req := httptest.NewRequest(http.MethodGet, "/idontexist", nil) + rec := httptest.NewRecorder() + e.ServeHTTP(rec, req) + + assert.Equal(t, "GET", tracer.currentSpan().getTag("http.method")) + assert.Equal(t, "/idontexist", tracer.currentSpan().getTag("http.url")) + assert.Equal(t, defaultComponentName, tracer.currentSpan().getTag("component")) + assert.Equal(t, uint16(404), tracer.currentSpan().getTag("http.status_code")) + assert.Equal(t, true, tracer.currentSpan().getTag("error")) + }) + + t.Run("custom http error", func(t *testing.T) { + req := httptest.NewRequest(http.MethodGet, "/giveme400", nil) + rec := httptest.NewRecorder() + e.ServeHTTP(rec, req) + + assert.Equal(t, uint16(400), tracer.currentSpan().getTag("http.status_code")) + assert.Equal(t, true, tracer.currentSpan().getTag("error")) + assert.Equal(t, "baaaad request", tracer.currentSpan().getTag("error.message")) + }) + + t.Run("unknown error", func(t *testing.T) { + req := httptest.NewRequest(http.MethodGet, "/givemeerror", nil) + rec := httptest.NewRecorder() + e.ServeHTTP(rec, req) + + assert.Equal(t, uint16(500), tracer.currentSpan().getTag("http.status_code")) + assert.Equal(t, true, tracer.currentSpan().getTag("error")) + assert.Equal(t, "internal stuff went wrong", tracer.currentSpan().getTag("error.message")) + }) } func TestTraceWithConfig(t *testing.T) { @@ -134,7 +182,7 @@ func TestTraceWithConfig(t *testing.T) { e := echo.New() e.Use(TraceWithConfig(TraceConfig{ Tracer: tracer, - componentName: "EchoTracer", + ComponentName: "EchoTracer", })) req := httptest.NewRequest(http.MethodGet, "/trace", nil) rec := httptest.NewRecorder() @@ -153,7 +201,7 @@ func TestTraceWithConfigOfBodyDump(t *testing.T) { e := echo.New() e.Use(TraceWithConfig(TraceConfig{ Tracer: tracer, - componentName: "EchoTracer", + ComponentName: "EchoTracer", IsBodyDump: true, })) e.POST("/trace", func(c echo.Context) error { @@ -169,6 +217,8 @@ func TestTraceWithConfigOfBodyDump(t *testing.T) { assert.Equal(t, "/trace", tracer.currentSpan().getTag("http.url")) assert.Equal(t, `{"name": "Lorem"}`, tracer.currentSpan().getTag("http.req.body")) assert.Equal(t, `Hi`, tracer.currentSpan().getTag("http.resp.body")) + assert.Equal(t, uint16(200), tracer.currentSpan().getTag("http.status_code")) + assert.Equal(t, nil, tracer.currentSpan().getTag("error")) assert.Equal(t, true, tracer.hasStartSpanWithOption) }