Repository: goadesign/goa
Pull request #2293 (2019-09-26 18:59:51)

trace_discards_v3
added DiscardFromTrace() option for tracing in HTTP and gRPC.
useful to omit API calls such as health-checks, which happen frequently but are not interesting to trace.


raphael: Looks great, thank you. Just a tiny change that would be good: the change is in the generic tracing middleware - not specific to X-Ray so it would be better if the comments didn't mention X-Ray:
```go
// DiscardFromTrace adds a regular expression for matching a request path to be discarded from tracing
```
instead of
```go
// DiscardFromTrace adds a regular expression for matching a request path to be discarded from X-Ray tracing
```
Thank you!
raphael: Thanks!

Patch:
diff --git a/grpc/middleware/trace.go b/grpc/middleware/trace.go
index 465bd771..8175f654 100644
--- a/grpc/middleware/trace.go
+++ b/grpc/middleware/trace.go
@@ -2,6 +2,7 @@ package middleware
 
 import (
 	"context"
+	"regexp"
 
 	"goa.design/goa/v3/middleware"
 	"google.golang.org/grpc"
@@ -36,7 +37,7 @@ const (
 func UnaryServerTrace(opts ...middleware.TraceOption) grpc.UnaryServerInterceptor {
 	o := middleware.NewTraceOptions(opts...)
 	return grpc.UnaryServerInterceptor(func(ctx context.Context, req interface{}, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (resp interface{}, err error) {
-		ctx = withTrace(ctx, o)
+		ctx = withTrace(ctx, info.FullMethod, o)
 		return handler(ctx, req)
 	})
 }
@@ -55,7 +56,7 @@ func UnaryServerTrace(opts ...middleware.TraceOption) grpc.UnaryServerIntercepto
 func StreamServerTrace(opts ...middleware.TraceOption) grpc.StreamServerInterceptor {
 	o := middleware.NewTraceOptions(opts...)
 	return grpc.StreamServerInterceptor(func(srv interface{}, ss grpc.ServerStream, info *grpc.StreamServerInfo, handler grpc.StreamHandler) error {
-		ctx := withTrace(ss.Context(), o)
+		ctx := withTrace(ss.Context(), info.FullMethod, o)
 		wss := NewWrappedServerStream(ctx, ss)
 		return handler(srv, wss)
 	})
@@ -112,8 +113,14 @@ func SampleSize(s int) middleware.TraceOption {
 	return middleware.SampleSize(s)
 }
 
+// DiscardFromTrace adds a regular expression for matching a request path to be discarded from tracing.
+// see middleware.DiscardFromTrace() for more details.
+func DiscardFromTrace(discard *regexp.Regexp) middleware.TraceOption {
+	return middleware.DiscardFromTrace(discard)
+}
+
 // withTrace sets the trace ID, span ID, and parent span ID in the context.
-func withTrace(ctx context.Context, opts *middleware.TraceOptions) context.Context {
+func withTrace(ctx context.Context, fullMethod string, opts *middleware.TraceOptions) context.Context {
 	sampler := opts.NewSampler()
 	md, ok := metadata.FromIncomingContext(ctx)
 	if !ok {
@@ -123,9 +130,18 @@ func withTrace(ctx context.Context, opts *middleware.TraceOptions) context.Conte
 	var traceID string
 	{
 		traceID = MetadataValue(md, TraceIDMetadataKey)
-		if traceID == "" && sampler.Sample() {
-			// insert tracing only within sample.
-			traceID = opts.TraceID()
+		if traceID == "" {
+			var discarded bool
+			for _, discard := range opts.Discards() {
+				if discard.MatchString(fullMethod) {
+					discarded = true
+					break
+				}
+			}
+			if !discarded && sampler.Sample() {
+				// insert tracing only within sample.
+				traceID = opts.TraceID()
+			}
 		}
 	}
 	if traceID == "" {
diff --git a/http/middleware/trace.go b/http/middleware/trace.go
index ec74af1f..cddeda05 100644
--- a/http/middleware/trace.go
+++ b/http/middleware/trace.go
@@ -2,6 +2,7 @@ package middleware
 
 import (
 	"net/http"
+	"regexp"
 
 	"goa.design/goa/v3/middleware"
 )
@@ -38,9 +39,21 @@ func Trace(opts ...middleware.TraceOption) func(http.Handler) http.Handler {
 		return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 			// insert a new trace ID only if not already being traced.
 			traceID := r.Header.Get(TraceIDHeader)
-			if traceID == "" && sampler.Sample() {
-				// insert tracing only within sample.
-				traceID = o.TraceID()
+			if traceID == "" {
+				// check for discards only if we do not already have a trace ID and before sampling.
+				var discarded bool
+				if r.URL != nil { // docs imply but do not actually state that URL cannot be nil
+					for _, discard := range o.Discards() {
+						if discard.MatchString(r.URL.Path) {
+							discarded = true
+							break
+						}
+					}
+				}
+				if !discarded && sampler.Sample() {
+					// insert tracing only within sample.
+					traceID = o.TraceID()
+				}
 			}
 			if traceID == "" {
 				h.ServeHTTP(w, r)
@@ -80,6 +93,12 @@ func SampleSize(s int) middleware.TraceOption {
 	return middleware.SampleSize(s)
 }
 
+// DiscardFromTrace adds a regular expression for matching a request path to be discarded from tracing.
+// see middleware.DiscardFromTrace() for more details.
+func DiscardFromTrace(discard *regexp.Regexp) middleware.TraceOption {
+	return middleware.DiscardFromTrace(discard)
+}
+
 // WrapDoer wraps a goa client Doer and sets the trace headers so that the
 // downstream service may properly retrieve the parent span ID and trace ID.
 func WrapDoer(doer Doer) Doer {
diff --git a/middleware/trace.go b/middleware/trace.go
index 4c8e955f..7f279abb 100644
--- a/middleware/trace.go
+++ b/middleware/trace.go
@@ -2,6 +2,7 @@ package middleware
 
 import (
 	"context"
+	"regexp"
 )
 
 type (
@@ -20,6 +21,7 @@ type (
 		samplingPercent int
 		maxSamplingRate int
 		sampleSize      int
+		discards        []*regexp.Regexp
 	}
 
 	// tracedLogger is a logger which logs the trace ID with every log entry
@@ -67,6 +69,12 @@ func (o *TraceOptions) SpanID() string {
 	return o.spanIDFunc()
 }
 
+// Discards returns the list of regular expressions used to match paths to be
+// discarded from tracing.
+func (o *TraceOptions) Discards() []*regexp.Regexp {
+	return o.discards
+}
+
 // TraceIDFunc configures the function used to compute trace IDs. Use this
 // option to generate IDs compatible with backend tracing systems
 // (e.g. AWS XRay).
@@ -128,6 +136,22 @@ func SampleSize(s int) TraceOption {
 	}
 }
 
+// DiscardFromTrace adds a regular expression for matching a request path to be discarded from tracing.
+// this is useful for frequent API calls that are not important to trace, such as health checks.
+// the pattern can be a full or partial match and could even support both HTTP and gRPC paths with an
+// OR expression, etc.
+//
+// note that discards can be overridden if the incoming request already has a trace ID.
+func DiscardFromTrace(discard *regexp.Regexp) TraceOption {
+	if discard == nil {
+		panic("discard cannot be nil")
+	}
+	return func(o *TraceOptions) *TraceOptions {
+		o.discards = append(o.discards, discard)
+		return o
+	}
+}
+
 // WithSpan returns a context containing the given trace, span and parent span
 // IDs.
 func WithSpan(ctx context.Context, traceID, spanID, parentID string) context.Context {


Test patch:
diff --git a/grpc/middleware/trace_test.go b/grpc/middleware/trace_test.go
index 923a2fbb..6fe58c4a 100644
--- a/grpc/middleware/trace_test.go
+++ b/grpc/middleware/trace_test.go
@@ -3,6 +3,7 @@ package middleware_test
 import (
 	"context"
 	"fmt"
+	"regexp"
 	"testing"
 
 	grpcm "goa.design/goa/v3/grpc/middleware"
@@ -16,6 +17,7 @@ var (
 	spanID     = "testSpanID"
 	newTraceID = func() string { return traceID }
 	newID      = func() string { return spanID }
+	discard    = regexp.MustCompile("Test$")
 )
 
 func TestUnaryServerTrace(t *testing.T) {
@@ -26,16 +28,22 @@ func TestUnaryServerTrace(t *testing.T) {
 	cases := map[string]struct {
 		Rate                  int
 		TraceID, ParentSpanID string
+		Discard               *regexp.Regexp
 		// output
 		CtxTraceID, CtxSpanID, CtxParentID string
 	}{
-		"no-trace": {100, "", "", traceID, spanID, ""},
-		"trace":    {100, "trace", "", "trace", spanID, ""},
-		"parent":   {100, "trace", "parent", "trace", spanID, "parent"},
+		"no-trace":             {100, "", "", nil, traceID, spanID, ""},
+		"no-trace-discarded":   {100, "", "", discard, "", "", ""},
+		"trace":                {100, "trace", "", nil, "trace", spanID, ""},
+		"trace-not-discarded":  {100, "trace", "", discard, "trace", spanID, ""},
+		"parent":               {100, "trace", "parent", nil, "trace", spanID, "parent"},
+		"parent-not-discarded": {100, "trace", "parent", discard, "trace", spanID, "parent"},
 
-		"zero-rate-no-trace": {0, "", "", "", "", ""},
-		"zero-rate-trace":    {0, "trace", "", "trace", spanID, ""},
-		"zero-rate-parent":   {0, "trace", "parent", "trace", spanID, "parent"},
+		"zero-rate-no-trace":             {0, "", "", nil, "", "", ""},
+		"zero-rate-trace":                {0, "trace", "", nil, "trace", spanID, ""},
+		"zero-rate-trace-not-discarded":  {0, "trace", "", discard, "trace", spanID, ""},
+		"zero-rate-parent":               {0, "trace", "parent", nil, "trace", spanID, "parent"},
+		"zero-rate-parent-not-discarded": {0, "trace", "parent", discard, "trace", spanID, "parent"},
 	}
 	for k, c := range cases {
 		t.Run(k, func(t *testing.T) {
@@ -72,11 +80,15 @@ func TestUnaryServerTrace(t *testing.T) {
 				md.Set(grpcm.ParentSpanIDMetadataKey, c.ParentSpanID)
 			}
 			ctx := metadata.NewIncomingContext(context.Background(), md)
-
-			_, err := grpcm.UnaryServerTrace(
+			traceOptions := []middleware.TraceOption{
 				grpcm.SamplingPercent(c.Rate),
 				grpcm.SpanIDFunc(newID),
-				grpcm.TraceIDFunc(newTraceID))(ctx, "request", unary, handler)
+				grpcm.TraceIDFunc(newTraceID),
+			}
+			if c.Discard != nil {
+				traceOptions = append(traceOptions, grpcm.DiscardFromTrace(c.Discard))
+			}
+			_, err := grpcm.UnaryServerTrace(traceOptions...)(ctx, "request", unary, handler)
 			if err != nil {
 				t.Errorf("UnaryServerTrace error: %v", err)
 			}
@@ -92,16 +104,22 @@ func TestStreamServerTrace(t *testing.T) {
 	cases := map[string]struct {
 		Rate                  int
 		TraceID, ParentSpanID string
+		Discard               *regexp.Regexp
 		// output
 		CtxTraceID, CtxSpanID, CtxParentID string
 	}{
-		"no-trace": {100, "", "", traceID, spanID, ""},
-		"trace":    {100, "trace", "", "trace", spanID, ""},
-		"parent":   {100, "trace", "parent", "trace", spanID, "parent"},
+		"no-trace":             {100, "", "", nil, traceID, spanID, ""},
+		"no-trace-discarded":   {100, "", "", discard, "", "", ""},
+		"trace":                {100, "trace", "", nil, "trace", spanID, ""},
+		"trace-not-discarded":  {100, "trace", "", discard, "trace", spanID, ""},
+		"parent":               {100, "trace", "parent", nil, "trace", spanID, "parent"},
+		"parent-not-discarded": {100, "trace", "parent", discard, "trace", spanID, "parent"},
 
-		"zero-rate-no-trace": {0, "", "", "", "", ""},
-		"zero-rate-trace":    {0, "trace", "", "trace", spanID, ""},
-		"zero-rate-parent":   {0, "trace", "parent", "trace", spanID, "parent"},
+		"zero-rate-no-trace":             {0, "", "", nil, "", "", ""},
+		"zero-rate-trace":                {0, "trace", "", nil, "trace", spanID, ""},
+		"zero-rate-trace-not-discarded":  {0, "trace", "", discard, "trace", spanID, ""},
+		"zero-rate-parent":               {0, "trace", "parent", nil, "trace", spanID, "parent"},
+		"zero-rate-parent-not-discarded": {0, "trace", "parent", discard, "trace", spanID, "parent"},
 	}
 	for k, c := range cases {
 		t.Run(k, func(t *testing.T) {
@@ -140,11 +158,15 @@ func TestStreamServerTrace(t *testing.T) {
 			}
 			ctx := metadata.NewIncomingContext(context.Background(), md)
 			wss := grpcm.NewWrappedServerStream(ctx, &testServerStream{})
-
-			err := grpcm.StreamServerTrace(
+			traceOptions := []middleware.TraceOption{
 				grpcm.SamplingPercent(c.Rate),
 				grpcm.SpanIDFunc(newID),
-				grpcm.TraceIDFunc(newTraceID))(nil, wss, stream, handler)
+				grpcm.TraceIDFunc(newTraceID),
+			}
+			if c.Discard != nil {
+				traceOptions = append(traceOptions, grpcm.DiscardFromTrace(c.Discard))
+			}
+			err := grpcm.StreamServerTrace(traceOptions...)(nil, wss, stream, handler)
 			if err != nil {
 				t.Errorf("StreamServerTrace error: %v", err)
 			}
diff --git a/http/middleware/trace_test.go b/http/middleware/trace_test.go
index 67c979e8..0f7bc1a9 100644
--- a/http/middleware/trace_test.go
+++ b/http/middleware/trace_test.go
@@ -4,6 +4,7 @@ import (
 	"context"
 	"net/http"
 	"net/http/httptest"
+	"regexp"
 	"testing"
 
 	httpm "goa.design/goa/v3/http/middleware"
@@ -26,26 +27,40 @@ func TestMiddleware(t *testing.T) {
 		spanID     = "testSpanID"
 		newTraceID = func() string { return traceID }
 		newID      = func() string { return spanID }
+		discard    = regexp.MustCompile("^/$")
 	)
 
 	cases := map[string]struct {
 		Rate                  int
 		TraceID, ParentSpanID string
+		Discard               *regexp.Regexp
 		// output
 		CtxTraceID, CtxSpanID, CtxParentID string
 	}{
-		"no-trace": {100, "", "", traceID, spanID, ""},
-		"trace":    {100, "trace", "", "trace", spanID, ""},
-		"parent":   {100, "trace", "parent", "trace", spanID, "parent"},
+		"no-trace":             {100, "", "", nil, traceID, spanID, ""},
+		"no-trace-discarded":   {100, "", "", discard, "", "", ""},
+		"trace":                {100, "trace", "", nil, "trace", spanID, ""},
+		"trace-not-discarded":  {100, "trace", "", discard, "trace", spanID, ""},
+		"parent":               {100, "trace", "parent", nil, "trace", spanID, "parent"},
+		"parent-not-discarded": {100, "trace", "parent", discard, "trace", spanID, "parent"},
 
-		"zero-rate-no-trace": {0, "", "", "", "", ""},
-		"zero-rate-trace":    {0, "trace", "", "trace", spanID, ""},
-		"zero-rate-parent":   {0, "trace", "parent", "trace", spanID, "parent"},
+		"zero-rate-no-trace":             {0, "", "", nil, "", "", ""},
+		"zero-rate-trace":                {0, "trace", "", nil, "trace", spanID, ""},
+		"zero-rate-trace-not-discarded":  {0, "trace", "", discard, "trace", spanID, ""},
+		"zero-rate-parent-not-discarded": {0, "trace", "parent", discard, "trace", spanID, "parent"},
 	}
 
 	for k, c := range cases {
+		traceOptions := []middleware.TraceOption{
+			httpm.SamplingPercent(c.Rate),
+			httpm.SpanIDFunc(newID),
+			httpm.TraceIDFunc(newTraceID),
+		}
+		if c.Discard != nil {
+			traceOptions = append(traceOptions, httpm.DiscardFromTrace(c.Discard))
+		}
 		var (
-			m       = httpm.Trace(httpm.SamplingPercent(c.Rate), httpm.SpanIDFunc(newID), httpm.TraceIDFunc(newTraceID))
+			m       = httpm.Trace(traceOptions...)
 			h       = new(testHandler)
 			headers = make(http.Header)
 		)
diff --git a/middleware/trace_test.go b/middleware/trace_test.go
index 8a87687c..242013e1 100644
--- a/middleware/trace_test.go
+++ b/middleware/trace_test.go
@@ -2,6 +2,7 @@ package middleware
 
 import (
 	"math"
+	"regexp"
 	"testing"
 )
 
@@ -92,4 +93,22 @@ func TestNewTraceOptions(t *testing.T) {
 			}()
 		}
 	}
+
+	// invalid discard
+	{
+		cases := map[string]struct{ Discard *regexp.Regexp }{
+			"nil": {nil},
+		}
+		for k, c := range cases {
+			func() {
+				defer func() {
+					r := recover()
+					if r != "discard cannot be nil" {
+						t.Errorf("DiscardFromTrace(%s): NewTraceOptions did *not* panic as expected: %v", k, r)
+					}
+				}()
+				NewTraceOptions(DiscardFromTrace(c.Discard))
+			}()
+		}
+	}
 }