httpbp/thriftbp: Tracing middlewares v2 compat fix

For the following 3 tracing middlewares: - thriftbp server - thriftbp client - httpbp client The current behavior is to short-circuit to v2 compat middleware if injected, or fallback to v0 tracing implementation if not injected. Change the fallback logic to no-op, as the v0 tracing implementation is not really working any more and it will spam logs. For httpbp server tracing middleware, remove it from defaults, as the current v2 compat implementation is to skip it when injected as the API is not sufficient for the v2 compat implementation to work. This replaces and closes reddit#664.
fishy · Oct 7, 2024 · f2e6a2d · f2e6a2d
1 parent c7e0658
commit f2e6a2d
Show file tree

Hide file tree

Showing 10 changed files with 16 additions and 555 deletions.
diff --git a/httpbp/client_middlewares.go b/httpbp/client_middlewares.go
@@ -12,14 +12,12 @@ import (
 	"time"
 
 	"github.com/avast/retry-go"
-	"github.com/opentracing/opentracing-go"
 	"github.com/prometheus/client_golang/prometheus"
 
 	"github.com/reddit/baseplate.go/breakerbp"
 	//lint:ignore SA1019 This library is internal only, not actually deprecated
 	"github.com/reddit/baseplate.go/internalv2compat"
 	"github.com/reddit/baseplate.go/retrybp"
-	"github.com/reddit/baseplate.go/tracing"
 	"github.com/reddit/baseplate.go/transport"
 )
 
@@ -266,28 +264,16 @@ func MaxConcurrency(maxConcurrency int64) ClientMiddleware {
 
 // MonitorClient is an HTTP client middleware that wraps HTTP requests in a
 // client span.
+//
+// This middleware always use the injected v2 tracing http client middleware.
+// If there's no v2 tracing http client middleware injected, it's no-op.
 func MonitorClient(slug string) ClientMiddleware {
 	if mw := internalv2compat.V2TracingHTTPClientMiddleware(); mw != nil {
 		return mw
 	}
 	return func(next http.RoundTripper) http.RoundTripper {
-		return roundTripperFunc(func(req *http.Request) (resp *http.Response, err error) {
-			span, ctx := opentracing.StartSpanFromContext(
-				req.Context(),
-				slug+".request",
-				tracing.SpanTypeOption{Type: tracing.SpanTypeClient},
-			)
-			span.SetTag("http.method", req.Method)
-			span.SetTag("http.url", req.URL)
-
-			defer func() {
-				span.FinishWithOptions(tracing.FinishOptions{
-					Ctx: req.Context(),
-					Err: err,
-				}.Convert())
-			}()
-			return next.RoundTrip(req.WithContext(ctx))
-		})
+		// no-op
+		return next
 	}
 }
 

diff --git a/httpbp/client_middlewares_test.go b/httpbp/client_middlewares_test.go
@@ -2,8 +2,6 @@ package httpbp
 
 import (
 	"bytes"
-	"context"
-	"encoding/json"
 	"errors"
 	"io"
 	"net/http"
@@ -18,8 +16,6 @@ import (
 	"github.com/sony/gobreaker"
 
 	"github.com/reddit/baseplate.go/breakerbp"
-	"github.com/reddit/baseplate.go/mqsend"
-	"github.com/reddit/baseplate.go/tracing"
 )
 
 func TestNewClient(t *testing.T) {
@@ -57,18 +53,6 @@ func TestNewClient(t *testing.T) {
 		}))
 		defer server.Close()
 
-		recorder := mqsend.OpenMockMessageQueue(mqsend.MessageQueueConfig{
-			MaxQueueSize:   tracing.MaxQueueSize,
-			MaxMessageSize: tracing.MaxSpanSize,
-		})
-		err := tracing.InitGlobalTracer(tracing.Config{
-			SampleRate:               1,
-			TestOnlyMockMessageQueue: recorder,
-		})
-		if err != nil {
-			t.Fatal(err)
-		}
-
 		client, err := NewClient(ClientConfig{
 			Slug: "test",
 		})
@@ -85,21 +69,6 @@ func TestNewClient(t *testing.T) {
 		if !errors.As(err, &e) {
 			t.Errorf("expected error wrap error of type %T", *e)
 		}
-
-		// MonitorClient is applied
-		b, err := recorder.Receive(context.Background())
-		if err != nil {
-			t.Fatal(err)
-		}
-		var span tracing.ZipkinSpan
-		err = json.Unmarshal(b, &span)
-		if err != nil {
-			t.Fatal(err)
-		}
-		expected := "test.request"
-		if span.Name != expected {
-			t.Errorf("expected %s, actual: %q", expected, span.Name)
-		}
 	})
 }
 
@@ -149,47 +118,6 @@ func TestNewClientConcurrency(t *testing.T) {
 	wg.Wait()
 }
 
-func TestMonitorClient(t *testing.T) {
-	recorder := mqsend.OpenMockMessageQueue(mqsend.MessageQueueConfig{
-		MaxQueueSize:   tracing.MaxQueueSize,
-		MaxMessageSize: tracing.MaxSpanSize,
-	})
-	err := tracing.InitGlobalTracer(tracing.Config{
-		SampleRate:               1,
-		TestOnlyMockMessageQueue: recorder,
-	})
-	if err != nil {
-		t.Fatal(err)
-	}
-	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-	}))
-	defer server.Close()
-
-	middleware := MonitorClient("test")
-	client := &http.Client{
-		Transport: middleware(http.DefaultTransport),
-	}
-	_, err = client.Get(server.URL)
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	b, err := recorder.Receive(context.Background())
-	if err != nil {
-		t.Fatal(err)
-	}
-	var span tracing.ZipkinSpan
-	err = json.Unmarshal(b, &span)
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	expected := "test.request"
-	if span.Name != expected {
-		t.Errorf("expected %s, actual: %q", expected, span.Name)
-	}
-}
-
 func TestClientErrorWrapper(t *testing.T) {
 	t.Run("HTTP 200", func(t *testing.T) {
 		server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {

diff --git a/httpbp/fixtures_test.go b/httpbp/fixtures_test.go
@@ -6,7 +6,6 @@ import (
 	"net/http"
 	"strings"
 	"testing"
-	"time"
 
 	"github.com/reddit/baseplate.go/ecinterface"
 	"github.com/reddit/baseplate.go/httpbp"
@@ -17,10 +16,6 @@ type jsonResponseBody struct {
 	X int `json:"x"`
 }
 
-const (
-	testTimeout = time.Millisecond * 100
-)
-
 var testSecrets = map[string]secrets.GenericSecret{
 	"secret/http/edge-context-signature": {
 		Type:     secrets.VersionedType,

diff --git a/httpbp/middlewares.go b/httpbp/middlewares.go
@@ -70,15 +70,13 @@ type DefaultMiddlewareArgs struct {
 // DefaultMiddleware returns a slice of all the default Middleware for a
 // Baseplate HTTP server. The default middleware are (in order):
 //
-//  1. InjectServerSpan
-//  2. InjectEdgeRequestContext
-//  3. PrometheusServerMetrics
+//  1. InjectEdgeRequestContext
+//  2. PrometheusServerMetrics
 func DefaultMiddleware(args DefaultMiddlewareArgs) []Middleware {
 	if args.TrustHandler == nil {
 		args.TrustHandler = NeverTrustHeaders{}
 	}
 	return []Middleware{
-		InjectServerSpan(args.TrustHandler),
 		InjectEdgeRequestContext(InjectEdgeRequestContextArgs(args)),
 		PrometheusServerMetrics(""),
 	}

diff --git a/httpbp/middlewares_test.go b/httpbp/middlewares_test.go
@@ -3,7 +3,6 @@ package httpbp_test
 import (
 	"bufio"
 	"context"
-	"encoding/json"
 	"errors"
 	"net"
 	"net/http"
@@ -16,8 +15,6 @@ import (
 	"github.com/reddit/baseplate.go/ecinterface"
 	"github.com/reddit/baseplate.go/httpbp"
 	"github.com/reddit/baseplate.go/log"
-	"github.com/reddit/baseplate.go/mqsend"
-	"github.com/reddit/baseplate.go/tracing"
 )
 
 func TestWrap(t *testing.T) {
@@ -40,125 +37,6 @@ func TestWrap(t *testing.T) {
 	}
 }
 
-func TestInjectServerSpan(t *testing.T) {
-	t.Parallel()
-
-	defer func() {
-		tracing.CloseTracer()
-		tracing.InitGlobalTracer(tracing.Config{})
-	}()
-	mmq := mqsend.OpenMockMessageQueue(mqsend.MessageQueueConfig{
-		MaxQueueSize:   100,
-		MaxMessageSize: 1024,
-	})
-	logger, startFailing := tracing.TestWrapper(t)
-	tracing.InitGlobalTracer(tracing.Config{
-		SampleRate:               0,
-		MaxRecordTimeout:         testTimeout,
-		Logger:                   logger,
-		TestOnlyMockMessageQueue: mmq,
-	})
-	startFailing()
-
-	req := newRequest(t, "")
-
-	cases := []struct {
-		name           string
-		truster        httpbp.HeaderTrustHandler
-		err            error
-		hasAnnotations bool
-	}{
-		{
-			name:           "trust/no-err",
-			truster:        httpbp.AlwaysTrustHeaders{},
-			hasAnnotations: true,
-		},
-		{
-			name:           "trust/err",
-			truster:        httpbp.AlwaysTrustHeaders{},
-			hasAnnotations: true,
-			err:            errors.New("test"),
-		},
-		{
-			name:           "trust/http-err/4xx",
-			truster:        httpbp.AlwaysTrustHeaders{},
-			hasAnnotations: true,
-			err:            httpbp.JSONError(httpbp.BadRequest(), nil),
-		},
-		{
-			name:           "trust/http-err/5xx",
-			truster:        httpbp.AlwaysTrustHeaders{},
-			hasAnnotations: true,
-			err:            httpbp.JSONError(httpbp.InternalServerError(), nil),
-		},
-		{
-			name:           "no-trust/no-err",
-			truster:        httpbp.NeverTrustHeaders{},
-			hasAnnotations: false,
-		},
-		{
-			name:           "no-trust/err",
-			truster:        httpbp.NeverTrustHeaders{},
-			hasAnnotations: false,
-			err:            errors.New("test"),
-		},
-	}
-	for _, _c := range cases {
-		c := _c
-		t.Run(
-			c.name,
-			func(t *testing.T) {
-				handle := httpbp.Wrap(
-					"test",
-					newTestHandler(testHandlerPlan{err: c.err}),
-					httpbp.InjectServerSpan(c.truster),
-				)
-				handle(req.Context(), httptest.NewRecorder(), req)
-
-				ctx, cancel := context.WithTimeout(context.Background(), testTimeout)
-				defer cancel()
-				msg, err := mmq.Receive(ctx)
-				if !c.hasAnnotations && err == nil {
-					t.Fatal("expected error, got nil")
-				} else if c.hasAnnotations && err != nil {
-					t.Fatal(err)
-				}
-				if !c.hasAnnotations {
-					return
-				}
-
-				var trace tracing.ZipkinSpan
-				err = json.Unmarshal(msg, &trace)
-				if err != nil {
-					t.Fatal(err)
-				}
-				if len(trace.BinaryAnnotations) == 0 {
-					t.Fatal("no binary annotations")
-				}
-				t.Logf("%#v", trace.BinaryAnnotations)
-				hasError := false
-				for _, annotation := range trace.BinaryAnnotations {
-					if annotation.Key == "error" {
-						hasError = true
-					}
-				}
-				expectedErr := c.err
-				var httpErr httpbp.HTTPError
-				if errors.As(c.err, &httpErr) {
-					if httpErr.Response().Code < 500 {
-						expectedErr = nil
-					}
-				}
-				if expectedErr != nil && !hasError {
-					t.Error("error binary annotation was not present.")
-				} else if expectedErr == nil && hasError {
-					t.Error("unexpected error binary annotation")
-				}
-			},
-		)
-	}
-}
-
 func TestInjectEdgeRequestContext(t *testing.T) {
 	t.Parallel()
 

diff --git a/httpbp/server_test.go b/httpbp/server_test.go
@@ -14,8 +14,6 @@ import (
 	"github.com/reddit/baseplate.go/ecinterface"
 	"github.com/reddit/baseplate.go/httpbp"
 	"github.com/reddit/baseplate.go/log"
-	"github.com/reddit/baseplate.go/mqsend"
-	"github.com/reddit/baseplate.go/tracing"
 )
 
 func TestEndpoint(t *testing.T) {
@@ -289,23 +287,6 @@ func TestServerArgsSetupEndpoints(t *testing.T) {
 				t.Fatalf("no handler at %q: %#v", pattern, registry.registry)
 			}
 
-			defer func() {
-				tracing.CloseTracer()
-				tracing.InitGlobalTracer(tracing.Config{})
-			}()
-			mmq := mqsend.OpenMockMessageQueue(mqsend.MessageQueueConfig{
-				MaxQueueSize:   100,
-				MaxMessageSize: 1024,
-			})
-			logger, startFailing := tracing.TestWrapper(t)
-			tracing.InitGlobalTracer(tracing.Config{
-				SampleRate:               1,
-				MaxRecordTimeout:         testTimeout,
-				Logger:                   logger,
-				TestOnlyMockMessageQueue: mmq,
-			})
-			startFailing()
-
 			req := newRequest(t, "foo")
 			req.Method = http.MethodGet
 			handle.ServeHTTP(httptest.NewRecorder(), req)
@@ -314,23 +295,6 @@ func TestServerArgsSetupEndpoints(t *testing.T) {
 			if recorder.header == "" {
 				t.Error("edge request context not set")
 			}
-
-			// Test that the Span middleware was set up
-			ctx, cancel := context.WithTimeout(context.Background(), testTimeout)
-			defer cancel()
-			msg, err := mmq.Receive(ctx)
-			if err != nil {
-				t.Fatal(err)
-			}
-
-			var trace tracing.ZipkinSpan
-			err = json.Unmarshal(msg, &trace)
-			if err != nil {
-				t.Fatal(err)
-			}
-			if len(trace.BinaryAnnotations) == 0 {
-				t.Fatal("no binary annotations")
-			}
 		},
 	)
 }