diff --git a/v1/features/tracing/tracing.go b/v1/features/tracing/tracing.go index 47199b913ac..f1f52f164bb 100644 --- a/v1/features/tracing/tracing.go +++ b/v1/features/tracing/tracing.go @@ -18,11 +18,56 @@ func init() { type factory struct{} func (*factory) NewTransport(tr http.RoundTripper, opts pkg_tracing.Options) http.RoundTripper { - return otelhttp.NewTransport(tr, convertOpts(opts)...) + return otelhttp.NewTransport(tr, withDefaultSpanNameFormatter(opts, clientSpanName)...) } func (*factory) NewHandler(f http.Handler, label string, opts pkg_tracing.Options) http.Handler { - return otelhttp.NewHandler(f, label, convertOpts(opts)...) + return otelhttp.NewHandler(f, label, withDefaultSpanNameFormatter(opts, serverSpanName)...) +} + +// serverSpanName names server-side HTTP spans as "{method} {operation}", where +// `operation` is the route label supplied to NewHandler (for OPA, e.g. +// "v1/data"). This follows the OpenTelemetry HTTP semantic conventions, which +// require the HTTP method as the first token of the span name: +// https://opentelemetry.io/docs/specs/semconv/http/http-spans/#name +func serverSpanName(operation string, r *http.Request) string { + if r == nil || r.Method == "" { + return operation + } + if operation == "" { + return r.Method + } + return r.Method + " " + operation +} + +// clientSpanName names client-side HTTP spans as "{method} {url.path}" where a +// path is available, falling back to "{method}" otherwise. The OpenTelemetry +// HTTP semantic conventions prefer "{method} {target}" over the legacy +// "HTTP {method}" form used by otelhttp's default formatter. +func clientSpanName(_ string, r *http.Request) string { + if r == nil { + return "HTTP" + } + method := r.Method + if method == "" { + method = "HTTP" + } + if r.URL != nil && r.URL.Path != "" { + return method + " " + r.URL.Path + } + return method +} + +// withDefaultSpanNameFormatter prepends the provided span name formatter to the +// converted otelhttp options. Because otelhttp applies options in order with +// last-write-wins semantics, any user-supplied WithSpanNameFormatter passed +// through pkg_tracing.Options still takes precedence. +func withDefaultSpanNameFormatter(opts pkg_tracing.Options, fn func(string, *http.Request) string) []otelhttp.Option { + converted := convertOpts(opts) + out := make([]otelhttp.Option, 0, len(converted)+1) + out = append(out, otelhttp.WithSpanNameFormatter(fn)) + out = append(out, converted...) + return out } func convertOpts(opts pkg_tracing.Options) []otelhttp.Option { diff --git a/v1/features/tracing/tracing_test.go b/v1/features/tracing/tracing_test.go new file mode 100644 index 00000000000..e58c5abcee2 --- /dev/null +++ b/v1/features/tracing/tracing_test.go @@ -0,0 +1,180 @@ +// Copyright 2026 The OPA Authors. All rights reserved. +// Use of this source code is governed by an Apache2 +// license that can be found in the LICENSE file. + +package tracing + +import ( + "context" + "net/http" + "net/http/httptest" + "net/url" + "testing" + + pkg_tracing "github.com/open-policy-agent/opa/v1/tracing" + "go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp" + "go.opentelemetry.io/otel/sdk/trace" + "go.opentelemetry.io/otel/sdk/trace/tracetest" +) + +func TestServerSpanNameFormatter(t *testing.T) { + cases := []struct { + name string + method string + operation string + want string + }{ + {"GET v1/data", "GET", "v1/data", "GET v1/data"}, + {"POST v1/data", "POST", "v1/data", "POST v1/data"}, + {"DELETE v1/policies", "DELETE", "v1/policies", "DELETE v1/policies"}, + {"empty operation", "GET", "", "GET"}, + {"empty method", "", "v1/data", "v1/data"}, + {"nil request", "", "v1/data", "v1/data"}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + var r *http.Request + if tc.name != "nil request" { + r = &http.Request{Method: tc.method, URL: &url.URL{Path: "/whatever"}} + } + if got := serverSpanName(tc.operation, r); got != tc.want { + t.Fatalf("serverSpanName(%q, %+v) = %q, want %q", tc.operation, r, got, tc.want) + } + }) + } +} + +func TestClientSpanNameFormatter(t *testing.T) { + cases := []struct { + name string + method string + path string + want string + }{ + {"GET with path", "GET", "/v1/data", "GET /v1/data"}, + {"POST with path", "POST", "/v1/logs", "POST /v1/logs"}, + {"GET no path", "GET", "", "GET"}, + {"empty method with path", "", "/health", "HTTP /health"}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + r := &http.Request{Method: tc.method, URL: &url.URL{Path: tc.path}} + if got := clientSpanName("", r); got != tc.want { + t.Fatalf("clientSpanName(%q, path=%q) = %q, want %q", tc.method, tc.path, got, tc.want) + } + }) + } + + t.Run("nil request", func(t *testing.T) { + if got := clientSpanName("", nil); got != "HTTP" { + t.Fatalf("clientSpanName(nil) = %q, want %q", got, "HTTP") + } + }) +} + +// TestHandlerSpanNameEndToEnd asserts that the handler factory wraps +// otelhttp.NewHandler such that the recorded server span is named +// "{method} {operation}" per OpenTelemetry HTTP semantic conventions. +func TestHandlerSpanNameEndToEnd(t *testing.T) { + exporter := tracetest.NewInMemoryExporter() + tp := trace.NewTracerProvider(trace.WithSpanProcessor(trace.NewSimpleSpanProcessor(exporter))) + t.Cleanup(func() { _ = tp.Shutdown(context.Background()) }) + + opts := pkg_tracing.NewOptions(otelhttp.WithTracerProvider(tp)) + + f := &factory{} + h := f.NewHandler(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + }), "v1/data", opts) + + srv := httptest.NewServer(h) + t.Cleanup(srv.Close) + + resp, err := http.Post(srv.URL+"/v1/data/foo", "application/json", nil) + if err != nil { + t.Fatal(err) + } + _ = resp.Body.Close() + + spans := exporter.GetSpans() + if got, want := len(spans), 1; got != want { + t.Fatalf("got %d span(s), want %d", got, want) + } + if got, want := spans[0].Name, "POST v1/data"; got != want { + t.Fatalf("span name = %q, want %q", got, want) + } +} + +// TestUserSpanNameFormatterOverrides asserts that any user-supplied +// WithSpanNameFormatter in the Options still wins over OPA's default. +func TestUserSpanNameFormatterOverrides(t *testing.T) { + exporter := tracetest.NewInMemoryExporter() + tp := trace.NewTracerProvider(trace.WithSpanProcessor(trace.NewSimpleSpanProcessor(exporter))) + t.Cleanup(func() { _ = tp.Shutdown(context.Background()) }) + + custom := func(string, *http.Request) string { return "custom-name" } + opts := pkg_tracing.NewOptions( + otelhttp.WithTracerProvider(tp), + otelhttp.WithSpanNameFormatter(custom), + ) + + f := &factory{} + h := f.NewHandler(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + }), "v1/data", opts) + + srv := httptest.NewServer(h) + t.Cleanup(srv.Close) + + resp, err := http.Get(srv.URL + "/anything") + if err != nil { + t.Fatal(err) + } + _ = resp.Body.Close() + + spans := exporter.GetSpans() + if got, want := len(spans), 1; got != want { + t.Fatalf("got %d span(s), want %d", got, want) + } + if got, want := spans[0].Name, "custom-name"; got != want { + t.Fatalf("span name = %q, want %q", got, want) + } +} + +// TestTransportSpanNameEndToEnd asserts that the transport factory names +// outbound HTTP client spans as "{method} {path}". +func TestTransportSpanNameEndToEnd(t *testing.T) { + exporter := tracetest.NewInMemoryExporter() + tp := trace.NewTracerProvider(trace.WithSpanProcessor(trace.NewSimpleSpanProcessor(exporter))) + t.Cleanup(func() { _ = tp.Shutdown(context.Background()) }) + + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + })) + t.Cleanup(upstream.Close) + + opts := pkg_tracing.NewOptions(otelhttp.WithTracerProvider(tp)) + + f := &factory{} + client := &http.Client{Transport: f.NewTransport(http.DefaultTransport, opts)} + + req, err := http.NewRequest(http.MethodPost, upstream.URL+"/v1/logs", nil) + if err != nil { + t.Fatal(err) + } + resp, err := client.Do(req) + if err != nil { + t.Fatal(err) + } + _ = resp.Body.Close() + + spans := exporter.GetSpans() + if got, want := len(spans), 1; got != want { + t.Fatalf("got %d span(s), want %d", got, want) + } + if got, want := spans[0].Name, "POST /v1/logs"; got != want { + t.Fatalf("span name = %q, want %q", got, want) + } +} diff --git a/v1/test/e2e/distributedtracing/distributedtracing_test.go b/v1/test/e2e/distributedtracing/distributedtracing_test.go index 025c4c32f5c..e8e2965e177 100644 --- a/v1/test/e2e/distributedtracing/distributedtracing_test.go +++ b/v1/test/e2e/distributedtracing/distributedtracing_test.go @@ -78,7 +78,7 @@ func TestServerSpan(t *testing.T) { if !spans[0].SpanContext.IsValid() { t.Fatalf("invalid span created: %#v", spans[0].SpanContext) } - if got, expected := spans[0].Name, "v0/data"; got != expected { + if got, expected := spans[0].Name, "POST v0/data"; got != expected { t.Fatalf("Expected span name to be %q but got %q", expected, got) } if got, expected := spans[0].SpanKind.String(), "server"; got != expected { @@ -126,7 +126,7 @@ func TestServerSpan(t *testing.T) { if !spans[0].SpanContext.IsValid() { t.Fatalf("invalid span created: %#v", spans[0].SpanContext) } - if got, expected := spans[0].Name, "v1/data"; got != expected { + if got, expected := spans[0].Name, "GET v1/data"; got != expected { t.Fatalf("Expected span name to be %q but got %q", expected, got) } if got, expected := spans[0].SpanKind.String(), "server"; got != expected { @@ -644,7 +644,7 @@ allow if { if !spans[0].SpanContext.IsValid() { t.Fatalf("invalid span created: %#v", spans[0].SpanContext) } - if got, expected := spans[0].Name, server.PromHandlerAPIAuthz; got != expected { + if got, expected := spans[0].Name, "POST "+server.PromHandlerAPIAuthz; got != expected { t.Fatalf("Expected span name to be %q but got %q", expected, got) } if got, expected := spans[0].SpanKind.String(), "server"; got != expected { @@ -830,13 +830,13 @@ func TestControlPlaneSpans(t *testing.T) { t.Fatalf("invalid span created: %#v", span.SpanContext) } } - if got, expected := spans[0].Name, "v1/data"; got != expected { + if got, expected := spans[0].Name, "POST v1/data"; got != expected { t.Fatalf("Expected span name to be %q but got %q", expected, got) } if got, expected := spans[0].SpanKind.String(), "server"; got != expected { t.Fatalf("Expected span kind to be %q but got %q", expected, got) } - if got, expected := spans[1].Name, "HTTP POST"; got != expected { + if got, expected := spans[1].Name, "POST /logs"; got != expected { t.Fatalf("Expected span name to be %q but got %q", expected, got) } if got, expected := spans[1].SpanKind.String(), "client"; got != expected {