Simplify GET trace sampling

Author: sjmiller609

cmd/api/main.go

@@ -324,7 +324,6 @@ func run() error {
 		r.Use(middleware.Recoverer)
 		if cfg.Otel.Enabled {
 			r.Use(otelchi.Middleware(cfg.Otel.ServiceName, otelchi.WithChiRoutes(r)))
-			r.Use(otel.NewSuccessfulGETErrorTraceMiddleware(cfg.Otel.ServiceName))
 		}
 		r.Use(mw.InjectLogger(logger))
 		r.Use(mw.AccessLogger(accessLogger))
@@ -347,7 +346,6 @@ func run() error {
 		// OpenTelemetry tracing middleware FIRST (creates span context)
 		if cfg.Otel.Enabled {
 			r.Use(otelchi.Middleware(cfg.Otel.ServiceName, otelchi.WithChiRoutes(r)))
-			r.Use(otel.NewSuccessfulGETErrorTraceMiddleware(cfg.Otel.ServiceName))
 		}
 
 		// Inject logger into request context for handlers to use

lib/otel/http_sampling.go

@@ -1,14 +1,10 @@
 package otel
 
 import (
-	"context"
 	"fmt"
 	"net/http"
 
-	"github.com/go-chi/chi/v5"
-	otelapi "go.opentelemetry.io/otel"
 	"go.opentelemetry.io/otel/attribute"
-	"go.opentelemetry.io/otel/codes"
 	sdktrace "go.opentelemetry.io/otel/sdk/trace"
 	"go.opentelemetry.io/otel/trace"
 )
@@ -26,9 +22,6 @@ func newSuccessfulGETSampler(ratio float64) sdktrace.Sampler {
 }
 
 func (s *successfulGETSampler) ShouldSample(params sdktrace.SamplingParameters) sdktrace.SamplingResult {
-	if attrValueFromAttrs(params.Attributes, "sampled_from") != "" {
-		return sdktrace.SamplingResult{Decision: sdktrace.RecordAndSample}
-	}
 	if params.Kind == trace.SpanKindServer && httpMethodFromAttrs(params.Attributes) == http.MethodGet {
 		return s.getSampler.ShouldSample(params)
 	}
@@ -40,95 +33,11 @@ func (s *successfulGETSampler) Description() string {
 }
 
 func httpMethodFromAttrs(attrs []attribute.KeyValue) string {
-	return attrValueFromAttrs(attrs, "http.method", "http.request.method")
-}
-
-func attrValueFromAttrs(attrs []attribute.KeyValue, keys ...string) string {
 	for _, attr := range attrs {
-		for _, key := range keys {
-			if string(attr.Key) == key {
-				return attr.Value.AsString()
-			}
+		switch string(attr.Key) {
+		case "http.method", "http.request.method":
+			return attr.Value.AsString()
 		}
 	}
 	return ""
 }
-
-type statusCapturingResponseWriter struct {
-	http.ResponseWriter
-	status int
-	wrote  bool
-}
-
-func (w *statusCapturingResponseWriter) WriteHeader(status int) {
-	if w.wrote {
-		return
-	}
-	w.wrote = true
-	w.status = status
-	w.ResponseWriter.WriteHeader(status)
-}
-
-func (w *statusCapturingResponseWriter) Write(body []byte) (int, error) {
-	if !w.wrote {
-		w.WriteHeader(http.StatusOK)
-	}
-	return w.ResponseWriter.Write(body)
-}
-
-// NewSuccessfulGETErrorTraceMiddleware records a compact fallback span for GET
-// requests that were sampled out but still returned an error status.
-func NewSuccessfulGETErrorTraceMiddleware(serviceName string) func(http.Handler) http.Handler {
-	tracer := otelapi.Tracer(serviceName + "/http")
-
-	return func(next http.Handler) http.Handler {
-		return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-			if r.Method != http.MethodGet {
-				next.ServeHTTP(w, r)
-				return
-			}
-
-			rw := &statusCapturingResponseWriter{
-				ResponseWriter: w,
-				status:         http.StatusOK,
-			}
-			next.ServeHTTP(rw, r)
-
-			if rw.status < http.StatusBadRequest {
-				return
-			}
-
-			spanCtx := trace.SpanContextFromContext(r.Context())
-			if spanCtx.IsValid() && spanCtx.IsSampled() {
-				return
-			}
-
-			route := ""
-			if routeCtx := chi.RouteContext(r.Context()); routeCtx != nil {
-				route = routeCtx.RoutePattern()
-			}
-			if route == "" {
-				route = r.URL.Path
-			}
-			if route == "" {
-				route = "/"
-			}
-
-			_, span := tracer.Start(
-				context.Background(),
-				route,
-				trace.WithNewRoot(),
-				trace.WithSpanKind(trace.SpanKindServer),
-				trace.WithAttributes(
-					attribute.String("http.method", r.Method),
-					attribute.String("http.route", route),
-					attribute.String("url.path", r.URL.Path),
-					attribute.Int("http.status_code", rw.status),
-					attribute.String("sampled_from", "unsampled_get_error"),
-				),
-			)
-			span.SetStatus(codes.Error, http.StatusText(rw.status))
-			span.End()
-		})
-	}
-}

lib/otel/http_sampling_test.go

@@ -18,9 +18,7 @@ func TestSuccessfulGETSamplerDropsSuccessfulGETRequests(t *testing.T) {
 	recorder, router, shutdown := newHTTPTraceTestHarness(t, 0)
 	defer shutdown()
 
-	router.Get("/instances", func(w http.ResponseWriter, r *http.Request) {
-		w.WriteHeader(http.StatusOK)
-	})
+	router.Get("/instances", func(w http.ResponseWriter, r *http.Request) {})
 
 	rr := httptest.NewRecorder()
 	req := httptest.NewRequest(http.MethodGet, "/instances", nil)
@@ -44,32 +42,11 @@ func TestSuccessfulGETSamplerKeepsSuccessfulPOSTRequests(t *testing.T) {
 	router.ServeHTTP(rr, req)
 
 	span := findEndedSpanByName(t, recorder.Ended(), "/instances")
-	if attrValue(span.Attributes(), "http.method") != http.MethodPost {
+	if got := attrValue(span.Attributes(), "http.method"); got != http.MethodPost {
 		t.Fatalf("expected POST span, got attrs %v", span.Attributes())
 	}
 }
 
-func TestSuccessfulGETSamplerKeepsUnsampledGETErrors(t *testing.T) {
-	recorder, router, shutdown := newHTTPTraceTestHarness(t, 0)
-	defer shutdown()
-
-	router.Get("/instances", func(w http.ResponseWriter, r *http.Request) {
-		http.Error(w, "boom", http.StatusInternalServerError)
-	})
-
-	rr := httptest.NewRecorder()
-	req := httptest.NewRequest(http.MethodGet, "/instances", nil)
-	router.ServeHTTP(rr, req)
-
-	span := findEndedSpanByName(t, recorder.Ended(), "/instances")
-	if attrValue(span.Attributes(), "sampled_from") != "unsampled_get_error" {
-		t.Fatalf("expected unsampled GET error fallback span, got attrs %v", span.Attributes())
-	}
-	if attrValue(span.Attributes(), "http.status_code") != "500" {
-		t.Fatalf("expected status code attr on fallback span, got attrs %v", span.Attributes())
-	}
-}
-
 func newHTTPTraceTestHarness(t *testing.T, getRatio float64) (*tracetest.SpanRecorder, *chi.Mux, func()) {
 	t.Helper()
 
@@ -83,7 +60,6 @@ func newHTTPTraceTestHarness(t *testing.T, getRatio float64) (*tracetest.SpanRec
 
 	router := chi.NewRouter()
 	router.Use(otelchi.Middleware("hypeman-test", otelchi.WithChiRoutes(router)))
-	router.Use(NewSuccessfulGETErrorTraceMiddleware("hypeman-test"))
 
 	return recorder, router, func() {
 		otelapi.SetTracerProvider(previous)