contrib: refactor http request span tags (#1286)

Refactor the start and finish of http request spans into functions shared across contribs so that the expected set of http request span tags are properly implemented.
DataDog · May 31, 2022 · b2387df · b2387df
1 parent 70e9456
commit b2387df
Show file tree

Hide file tree

Showing 13 changed files with 174 additions and 220 deletions.
diff --git a/contrib/emicklei/go-restful/restful.go b/contrib/emicklei/go-restful/restful.go
@@ -8,8 +8,8 @@ package restful
 
 import (
 	"math"
-	"strconv"
 
+	"gopkg.in/DataDog/dd-trace-go.v1/contrib/internal/httptrace"
 	"gopkg.in/DataDog/dd-trace-go.v1/ddtrace"
 	"gopkg.in/DataDog/dd-trace-go.v1/ddtrace/ext"
 	"gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer"
@@ -25,52 +25,31 @@ func FilterFunc(configOpts ...Option) restful.FilterFunction {
 		opt(cfg)
 	}
 	log.Debug("contrib/emicklei/go-restful: Creating tracing filter: %#v", cfg)
+	spanOpts := []ddtrace.StartSpanOption{tracer.ServiceName(cfg.serviceName)}
 	return func(req *restful.Request, resp *restful.Response, chain *restful.FilterChain) {
-		opts := []ddtrace.StartSpanOption{
-			tracer.ServiceName(cfg.serviceName),
-			tracer.ResourceName(req.SelectedRoutePath()),
-			tracer.SpanType(ext.SpanTypeWeb),
-			tracer.Tag(ext.HTTPMethod, req.Request.Method),
-			tracer.Tag(ext.HTTPURL, req.Request.URL.Path),
-		}
+		spanOpts := append(spanOpts, tracer.ResourceName(req.SelectedRoutePath()))
 		if !math.IsNaN(cfg.analyticsRate) {
-			opts = append(opts, tracer.Tag(ext.EventSampleRate, cfg.analyticsRate))
-		}
-		if spanctx, err := tracer.Extract(tracer.HTTPHeadersCarrier(req.Request.Header)); err == nil {
-			opts = append(opts, tracer.ChildOf(spanctx))
+			spanOpts = append(spanOpts, tracer.Tag(ext.EventSampleRate, cfg.analyticsRate))
 		}
-		span, ctx := tracer.StartSpanFromContext(req.Request.Context(), "http.request", opts...)
-		defer span.Finish()
+		span, ctx := httptrace.StartRequestSpan(req.Request, spanOpts...)
+		defer func() {
+			httptrace.FinishRequestSpan(span, resp.StatusCode(), tracer.WithError(resp.Error()))
+		}()
 
 		// pass the span through the request context
 		req.Request = req.Request.WithContext(ctx)
-
 		chain.ProcessFilter(req, resp)
-
-		span.SetTag(ext.HTTPCode, strconv.Itoa(resp.StatusCode()))
-		span.SetTag(ext.Error, resp.Error())
 	}
 }
 
 // Filter is deprecated. Please use FilterFunc.
 func Filter(req *restful.Request, resp *restful.Response, chain *restful.FilterChain) {
-	opts := []ddtrace.StartSpanOption{
-		tracer.ResourceName(req.SelectedRoutePath()),
-		tracer.SpanType(ext.SpanTypeWeb),
-		tracer.Tag(ext.HTTPMethod, req.Request.Method),
-		tracer.Tag(ext.HTTPURL, req.Request.URL.Path),
-	}
-	if spanctx, err := tracer.Extract(tracer.HTTPHeadersCarrier(req.Request.Header)); err == nil {
-		opts = append(opts, tracer.ChildOf(spanctx))
-	}
-	span, ctx := tracer.StartSpanFromContext(req.Request.Context(), "http.request", opts...)
-	defer span.Finish()
+	span, ctx := httptrace.StartRequestSpan(req.Request, tracer.ResourceName(req.SelectedRoutePath()))
+	defer func() {
+		httptrace.FinishRequestSpan(span, resp.StatusCode(), tracer.WithError(resp.Error()))
+	}()
 
 	// pass the span through the request context
 	req.Request = req.Request.WithContext(ctx)
-
 	chain.ProcessFilter(req, resp)
-
-	span.SetTag(ext.HTTPCode, strconv.Itoa(resp.StatusCode()))
-	span.SetTag(ext.Error, resp.Error())
 }
diff --git a/contrib/gin-gonic/gin/gintrace.go b/contrib/gin-gonic/gin/gintrace.go
@@ -9,10 +9,8 @@ package gin // import "gopkg.in/DataDog/dd-trace-go.v1/contrib/gin-gonic/gin"
 import (
 	"fmt"
 	"math"
-	"net/http"
-	"strconv"
 
-	"gopkg.in/DataDog/dd-trace-go.v1/ddtrace"
+	"gopkg.in/DataDog/dd-trace-go.v1/contrib/internal/httptrace"
 	"gopkg.in/DataDog/dd-trace-go.v1/ddtrace/ext"
 	"gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer"
 	"gopkg.in/DataDog/dd-trace-go.v1/internal/appsec"
@@ -29,28 +27,22 @@ func Middleware(service string, opts ...Option) gin.HandlerFunc {
 	for _, opt := range opts {
 		opt(cfg)
 	}
-	log.Debug("contrib/gin-gonic/gin: Configuring Middleware: Service: %s, %#v", service, cfg)
+	log.Debug("contrib/gin-gonic/gin: Configuring Middleware: Service: %s, %#v", cfg.serviceName, cfg)
+	spanOpts := []tracer.StartSpanOption{
+		tracer.ServiceName(cfg.serviceName),
+	}
 	return func(c *gin.Context) {
 		if cfg.ignoreRequest(c) {
 			return
 		}
-		resource := cfg.resourceNamer(c)
-		opts := []ddtrace.StartSpanOption{
-			tracer.ServiceName(cfg.serviceName),
-			tracer.ResourceName(resource),
-			tracer.SpanType(ext.SpanTypeWeb),
-			tracer.Tag(ext.HTTPMethod, c.Request.Method),
-			tracer.Tag(ext.HTTPURL, c.Request.URL.Path),
-			tracer.Measured(),
-		}
+		opts := append(spanOpts, tracer.ResourceName(cfg.resourceNamer(c)))
 		if !math.IsNaN(cfg.analyticsRate) {
 			opts = append(opts, tracer.Tag(ext.EventSampleRate, cfg.analyticsRate))
 		}
-		if spanctx, err := tracer.Extract(tracer.HTTPHeadersCarrier(c.Request.Header)); err == nil {
-			opts = append(opts, tracer.ChildOf(spanctx))
-		}
-		span, ctx := tracer.StartSpanFromContext(c.Request.Context(), "http.request", opts...)
-		defer span.Finish()
+		span, ctx := httptrace.StartRequestSpan(c.Request, opts...)
+		defer func() {
+			httptrace.FinishRequestSpan(span, c.Writer.Status())
+		}()
 
 		// pass the span through the request context
 		c.Request = c.Request.WithContext(ctx)
@@ -64,12 +56,6 @@ func Middleware(service string, opts ...Option) gin.HandlerFunc {
 		// serve the request to the next middleware
 		c.Next()
 
-		status := c.Writer.Status()
-		span.SetTag(ext.HTTPCode, strconv.Itoa(status))
-		if status >= 500 && status < 600 {
-			span.SetTag(ext.Error, fmt.Errorf("%d: %s", status, http.StatusText(status)))
-		}
-
 		if len(c.Errors) > 0 {
 			span.SetTag("gin.errors", c.Errors.String())
 		}

diff --git a/contrib/go-chi/chi.v5/chi.go b/contrib/go-chi/chi.v5/chi.go
@@ -10,9 +10,8 @@ import (
 	"fmt"
 	"math"
 	"net/http"
-	"strconv"
 
-	"gopkg.in/DataDog/dd-trace-go.v1/ddtrace"
+	"gopkg.in/DataDog/dd-trace-go.v1/contrib/internal/httptrace"
 	"gopkg.in/DataDog/dd-trace-go.v1/ddtrace/ext"
 	"gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer"
 	"gopkg.in/DataDog/dd-trace-go.v1/internal/appsec"
@@ -30,28 +29,30 @@ func Middleware(opts ...Option) func(next http.Handler) http.Handler {
 		fn(cfg)
 	}
 	log.Debug("contrib/go-chi/chi.v5: Configuring Middleware: %#v", cfg)
+	spanOpts := append(cfg.spanOpts, tracer.ServiceName(cfg.serviceName))
 	return func(next http.Handler) http.Handler {
 		return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 			if cfg.ignoreRequest(r) {
 				next.ServeHTTP(w, r)
 				return
 			}
-			opts := []ddtrace.StartSpanOption{
-				tracer.SpanType(ext.SpanTypeWeb),
-				tracer.ServiceName(cfg.serviceName),
-				tracer.Tag(ext.HTTPMethod, r.Method),
-				tracer.Tag(ext.HTTPURL, r.URL.Path),
-				tracer.Measured(),
-			}
+			opts := spanOpts
 			if !math.IsNaN(cfg.analyticsRate) {
 				opts = append(opts, tracer.Tag(ext.EventSampleRate, cfg.analyticsRate))
 			}
-			if spanctx, err := tracer.Extract(tracer.HTTPHeadersCarrier(r.Header)); err == nil {
-				opts = append(opts, tracer.ChildOf(spanctx))
-			}
-			opts = append(opts, cfg.spanOpts...)
-			span, ctx := tracer.StartSpanFromContext(r.Context(), "http.request", opts...)
-			defer span.Finish()
+			span, ctx := httptrace.StartRequestSpan(r, opts...)
+			ww := middleware.NewWrapResponseWriter(w, r.ProtoMajor)
+			defer func() {
+				status := ww.Status()
+				var opts []tracer.FinishOption
+				if cfg.isStatusError(status) {
+					opts = []tracer.FinishOption{tracer.WithError(fmt.Errorf("%d: %s", status, http.StatusText(status)))}
+				}
+				httptrace.FinishRequestSpan(span, status, opts...)
+			}()
+
+			// pass the span through the request context
+			r = r.WithContext(ctx)
 
 			next := next // avoid modifying the value of next in the outer closure scope
 			if appsec.Enabled() {
@@ -60,10 +61,8 @@ func Middleware(opts ...Option) func(next http.Handler) http.Handler {
 				// implements the `interface { Status() int }` expected by httpsec.
 			}
 
-			ww := middleware.NewWrapResponseWriter(w, r.ProtoMajor)
-
 			// pass the span through the request context and serve the request to the next middleware
-			next.ServeHTTP(ww, r.WithContext(ctx))
+			next.ServeHTTP(ww, r)
 
 			// set the resource name as we get it only once the handler is executed
 			resourceName := chi.RouteContext(r.Context()).RoutePattern()
@@ -72,19 +71,6 @@ func Middleware(opts ...Option) func(next http.Handler) http.Handler {
 			}
 			resourceName = r.Method + " " + resourceName
 			span.SetTag(ext.ResourceName, resourceName)
-
-			// set the status code
-			status := ww.Status()
-			// 0 status means one has not yet been sent in which case net/http library will write StatusOK
-			if ww.Status() == 0 {
-				status = http.StatusOK
-			}
-			span.SetTag(ext.HTTPCode, strconv.Itoa(status))
-
-			if cfg.isStatusError(status) {
-				// mark 5xx server error
-				span.SetTag(ext.Error, fmt.Errorf("%d: %s", status, http.StatusText(status)))
-			}
 		})
 	}
 }
diff --git a/contrib/go-chi/chi/chi.go b/contrib/go-chi/chi/chi.go
@@ -10,9 +10,8 @@ import (
 	"fmt"
 	"math"
 	"net/http"
-	"strconv"
 
-	"gopkg.in/DataDog/dd-trace-go.v1/ddtrace"
+	"gopkg.in/DataDog/dd-trace-go.v1/contrib/internal/httptrace"
 	"gopkg.in/DataDog/dd-trace-go.v1/ddtrace/ext"
 	"gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer"
 	"gopkg.in/DataDog/dd-trace-go.v1/internal/appsec"
@@ -30,28 +29,30 @@ func Middleware(opts ...Option) func(next http.Handler) http.Handler {
 		fn(cfg)
 	}
 	log.Debug("contrib/go-chi/chi: Configuring Middleware: %#v", cfg)
+	spanOpts := append(cfg.spanOpts, tracer.ServiceName(cfg.serviceName))
 	return func(next http.Handler) http.Handler {
 		return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 			if cfg.ignoreRequest(r) {
 				next.ServeHTTP(w, r)
 				return
 			}
-			opts := []ddtrace.StartSpanOption{
-				tracer.SpanType(ext.SpanTypeWeb),
-				tracer.ServiceName(cfg.serviceName),
-				tracer.Tag(ext.HTTPMethod, r.Method),
-				tracer.Tag(ext.HTTPURL, r.URL.Path),
-				tracer.Measured(),
-			}
+			opts := spanOpts
 			if !math.IsNaN(cfg.analyticsRate) {
 				opts = append(opts, tracer.Tag(ext.EventSampleRate, cfg.analyticsRate))
 			}
-			if spanctx, err := tracer.Extract(tracer.HTTPHeadersCarrier(r.Header)); err == nil {
-				opts = append(opts, tracer.ChildOf(spanctx))
-			}
-			opts = append(opts, cfg.spanOpts...)
-			span, ctx := tracer.StartSpanFromContext(r.Context(), "http.request", opts...)
-			defer span.Finish()
+			span, ctx := httptrace.StartRequestSpan(r, opts...)
+			ww := middleware.NewWrapResponseWriter(w, r.ProtoMajor)
+			defer func() {
+				status := ww.Status()
+				var opts []tracer.FinishOption
+				if cfg.isStatusError(status) {
+					opts = []tracer.FinishOption{tracer.WithError(fmt.Errorf("%d: %s", status, http.StatusText(status)))}
+				}
+				httptrace.FinishRequestSpan(span, status, opts...)
+			}()
+
+			// pass the span through the request context
+			r = r.WithContext(ctx)
 
 			next := next // avoid modifying the value of next in the outer closure scope
 			if appsec.Enabled() {
@@ -60,10 +61,8 @@ func Middleware(opts ...Option) func(next http.Handler) http.Handler {
 				// implements the `interface { Status() int }` expected by httpsec.
 			}
 
-			ww := middleware.NewWrapResponseWriter(w, r.ProtoMajor)
-
 			// pass the span through the request context and serve the request to the next middleware
-			next.ServeHTTP(ww, r.WithContext(ctx))
+			next.ServeHTTP(ww, r)
 
 			// set the resource name as we get it only once the handler is executed
 			resourceName := chi.RouteContext(r.Context()).RoutePattern()
@@ -72,19 +71,6 @@ func Middleware(opts ...Option) func(next http.Handler) http.Handler {
 			}
 			resourceName = r.Method + " " + resourceName
 			span.SetTag(ext.ResourceName, resourceName)
-
-			// set the status code
-			status := ww.Status()
-			// 0 status means one has not yet been sent in which case net/http library will write StatusOK
-			if ww.Status() == 0 {
-				status = http.StatusOK
-			}
-			span.SetTag(ext.HTTPCode, strconv.Itoa(status))
-
-			if cfg.isStatusError(status) {
-				// mark 5xx server error
-				span.SetTag(ext.Error, fmt.Errorf("%d: %s", status, http.StatusText(status)))
-			}
 		})
 	}
 }
diff --git a/contrib/gorilla/mux/option.go b/contrib/gorilla/mux/option.go
@@ -39,7 +39,6 @@ func newConfig(opts []RouterOption) *routerConfig {
 	if !math.IsNaN(cfg.analyticsRate) {
 		cfg.spanOpts = append(cfg.spanOpts, tracer.Tag(ext.EventSampleRate, cfg.analyticsRate))
 	}
-	cfg.spanOpts = append(cfg.spanOpts, tracer.Measured())
 	return cfg
 }
 

diff --git a/contrib/internal/httptrace/httptrace.go b/contrib/internal/httptrace/httptrace.go
@@ -0,0 +1,57 @@
+// Unless explicitly stated otherwise all files in this repository are licensed
+// under the Apache License Version 2.0.
+// This product includes software developed at Datadog (https://www.datadoghq.com/).
+// Copyright 2016 Datadog, Inc.
+
+// Package httptrace provides functionalities to trace HTTP requests that are commonly required and used across
+// contrib/** integrations.
+package httptrace
+
+import (
+	"context"
+	"fmt"
+	"net/http"
+	"strconv"
+
+	"gopkg.in/DataDog/dd-trace-go.v1/ddtrace"
+	"gopkg.in/DataDog/dd-trace-go.v1/ddtrace/ext"
+	"gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer"
+)
+
+// StartRequestSpan starts an HTTP request span with the standard list of HTTP request span tags (http.method, http.url,
+// http.useragent). Any further span start option can be added with opts.
+func StartRequestSpan(r *http.Request, opts ...ddtrace.StartSpanOption) (tracer.Span, context.Context) {
+	// Append our span options before the given ones so that the caller can "overwrite" them.
+	opts = append([]ddtrace.StartSpanOption{
+		tracer.SpanType(ext.SpanTypeWeb),
+		tracer.Tag(ext.HTTPMethod, r.Method),
+		tracer.Tag(ext.HTTPURL, r.URL.Path),
+		tracer.Tag(ext.HTTPUserAgent, r.UserAgent()),
+		tracer.Measured(),
+	}, opts...)
+	if r.URL.Host != "" {
+		opts = append([]ddtrace.StartSpanOption{
+			tracer.Tag("http.host", r.URL.Host),
+		}, opts...)
+	}
+	if spanctx, err := tracer.Extract(tracer.HTTPHeadersCarrier(r.Header)); err == nil {
+		opts = append(opts, tracer.ChildOf(spanctx))
+	}
+	return tracer.StartSpanFromContext(r.Context(), "http.request", opts...)
+}
+
+// FinishRequestSpan finishes the given HTTP request span and sets the expected response-related tags such as the status
+// code. Any further span finish option can be added with opts.
+func FinishRequestSpan(s tracer.Span, status int, opts ...tracer.FinishOption) {
+	var statusStr string
+	if status == 0 {
+		statusStr = "200"
+	} else {
+		statusStr = strconv.Itoa(status)
+	}
+	s.SetTag(ext.HTTPCode, statusStr)
+	if status >= 500 && status < 600 {
+		s.SetTag(ext.Error, fmt.Errorf("%s: %s", statusStr, http.StatusText(status)))
+	}
+	s.Finish(opts...)
+}
diff --git a/contrib/julienschmidt/httprouter/httprouter.go b/contrib/julienschmidt/httprouter/httprouter.go
@@ -35,7 +35,6 @@ func New(opts ...RouterOption) *Router {
 	if !math.IsNaN(cfg.analyticsRate) {
 		cfg.spanOpts = append(cfg.spanOpts, tracer.Tag(ext.EventSampleRate, cfg.analyticsRate))
 	}
-	cfg.spanOpts = append(cfg.spanOpts, tracer.Measured())
 	log.Debug("contrib/julienschmidt/httprouter: Configuring Router: %#v", cfg)
 	return &Router{httprouter.New(), cfg}
 }