From 5150b68f3a418f765db4d06a14cf0ab7d60e4c79 Mon Sep 17 00:00:00 2001 From: Tom Scanlan Date: Wed, 29 Jul 2026 11:51:40 -0400 Subject: [PATCH] fix(telemetry): make tracing opt-in instead of always-on InitTracing defaulted OTEL_EXPORTER_OTLP_ENDPOINT to "localhost:4318", so unsetting the variable did not disable tracing -- it silently retargeted the exporter at localhost and the batch processor logged export failures on a loop. There was no way to turn tracing off from configuration at all, which became a problem when we retired our collector: the service kept retrying into the void and filling logs with connection errors. Gate initialization on two conditions, matching how our Node services (openmeet-api, bsky-event-processor, bsky-firehose-consumer) already work, so the whole fleet has one consistent switch: 1. ENABLE_TRACING must be exactly "true" 2. OTEL_EXPORTER_OTLP_ENDPOINT must be set (no default) Either gate failing returns a no-op shutdown and leaves the global TracerProvider untouched. Callers always defer the returned function, so it is never nil. Tests: the previous suite used bare os.Setenv/os.Unsetenv, which leaked state between tests -- TestInitTracing_DefaultValues unset both variables and never restored them, so the two tests after it ran against whatever was left over. Switched to t.Setenv for automatic per-test restore, and added coverage for both disabled paths, for non-"true" values, and for the http:// scheme stripping that the deployment config actually exercises. --- internal/telemetry/tracing.go | 34 ++++++- internal/telemetry/tracing_test.go | 151 +++++++++++++++++------------ 2 files changed, 119 insertions(+), 66 deletions(-) diff --git a/internal/telemetry/tracing.go b/internal/telemetry/tracing.go index 408380d..2644bc6 100644 --- a/internal/telemetry/tracing.go +++ b/internal/telemetry/tracing.go @@ -14,12 +14,36 @@ import ( semconv "go.opentelemetry.io/otel/semconv/v1.28.0" ) -// InitTracing initializes OpenTelemetry tracing with OTLP HTTP exporter -// Returns a shutdown function that should be deferred +// InitTracing initializes OpenTelemetry tracing with OTLP HTTP exporter. +// Returns a shutdown function that should be deferred. +// +// Tracing is opt-in behind two gates, mirroring the Node services +// (openmeet-api, bsky-event-processor, bsky-firehose-consumer) so the whole +// fleet has one consistent switch: +// +// 1. ENABLE_TRACING must be exactly "true". +// 2. OTEL_EXPORTER_OTLP_ENDPOINT must be set. +// +// Gate 2 previously defaulted to "localhost:4318", which meant unsetting the +// variable did not disable tracing -- it silently retargeted the exporter at +// localhost and the batch processor logged export failures forever. Removing +// the collector left no way to turn this off from config at all. func InitTracing(ctx context.Context) (func(context.Context) error, error) { - // Get configuration from environment - // Default to HTTP OTLP port (4318) - more compatible than gRPC (4317) - endpoint := getEnvOrDefault("OTEL_EXPORTER_OTLP_ENDPOINT", "localhost:4318") + // No-op shutdown, returned whenever tracing is disabled. Callers always + // defer the result, so it must be safe to call and must not be nil. + noop := func(context.Context) error { return nil } + + if os.Getenv("ENABLE_TRACING") != "true" { + log.Printf("Tracing disabled (ENABLE_TRACING != \"true\")") + return noop, nil + } + + endpoint := os.Getenv("OTEL_EXPORTER_OTLP_ENDPOINT") + if endpoint == "" { + log.Printf("Tracing disabled (OTEL_EXPORTER_OTLP_ENDPOINT not set)") + return noop, nil + } + serviceName := getEnvOrDefault("OTEL_SERVICE_NAME", "survey-api") // Strip http:// or https:// scheme if present (HTTP exporter expects host:port only) diff --git a/internal/telemetry/tracing_test.go b/internal/telemetry/tracing_test.go index 73c9670..9106092 100644 --- a/internal/telemetry/tracing_test.go +++ b/internal/telemetry/tracing_test.go @@ -2,7 +2,6 @@ package telemetry import ( "context" - "os" "testing" "github.com/stretchr/testify/assert" @@ -10,105 +9,135 @@ import ( "go.opentelemetry.io/otel" ) -func TestInitTracing_Success(t *testing.T) { - // Set up environment - os.Setenv("OTEL_EXPORTER_OTLP_ENDPOINT", "localhost:4317") - os.Setenv("OTEL_SERVICE_NAME", "test-service") - defer os.Unsetenv("OTEL_EXPORTER_OTLP_ENDPOINT") - defer os.Unsetenv("OTEL_SERVICE_NAME") +// enableTracing sets both gates so InitTracing takes the live path. t.Setenv +// restores the previous values when the test ends, so these no longer leak +// into later tests the way the old os.Setenv/os.Unsetenv pairs did. +func enableTracing(t *testing.T, endpoint string) { + t.Helper() + t.Setenv("ENABLE_TRACING", "true") + t.Setenv("OTEL_EXPORTER_OTLP_ENDPOINT", endpoint) +} + +// assertTracingDisabled checks the observable signature of "tracing is off": +// InitTracing returned a usable no-op shutdown without installing a global +// TracerProvider. +func assertTracingDisabled(t *testing.T, prev any, shutdown func(context.Context) error, err error) { + t.Helper() + require.NoError(t, err) + require.NotNil(t, shutdown, "shutdown must be non-nil -- callers always defer it") + assert.True(t, any(otel.GetTracerProvider()) == prev, + "global TracerProvider should be untouched when tracing is disabled") + assert.NoError(t, shutdown(context.Background()), "no-op shutdown must be safe to call") +} + +func TestInitTracing_DisabledWhenEnableTracingUnset(t *testing.T) { + t.Setenv("ENABLE_TRACING", "") + t.Setenv("OTEL_EXPORTER_OTLP_ENDPOINT", "localhost:4318") - // Call InitTracing + prev := any(otel.GetTracerProvider()) shutdown, err := InitTracing(context.Background()) - // Should not error even if endpoint is unavailable (warning only) - assert.NoError(t, err) - assert.NotNil(t, shutdown) + assertTracingDisabled(t, prev, shutdown, err) +} + +func TestInitTracing_DisabledWhenEnableTracingNotTrue(t *testing.T) { + // Only the exact string "true" enables tracing -- "1", "yes", "TRUE" do not. + for _, v := range []string{"false", "1", "yes", "TRUE"} { + t.Run(v, func(t *testing.T) { + t.Setenv("ENABLE_TRACING", v) + t.Setenv("OTEL_EXPORTER_OTLP_ENDPOINT", "localhost:4318") - // Should set global tracer provider - tp := otel.GetTracerProvider() - assert.NotNil(t, tp) + prev := any(otel.GetTracerProvider()) + shutdown, err := InitTracing(context.Background()) - // Cleanup - if shutdown != nil { - err := shutdown(context.Background()) - assert.NoError(t, err) + assertTracingDisabled(t, prev, shutdown, err) + }) } } -func TestInitTracing_DefaultValues(t *testing.T) { - // Clear environment variables to test defaults - os.Unsetenv("OTEL_EXPORTER_OTLP_ENDPOINT") - os.Unsetenv("OTEL_SERVICE_NAME") +// The regression this change exists for: with ENABLE_TRACING=true but no +// endpoint, the old code silently fell back to "localhost:4318" and exported +// into the void forever. Unsetting the variable must actually disable tracing. +func TestInitTracing_DisabledWhenEndpointUnset(t *testing.T) { + t.Setenv("ENABLE_TRACING", "true") + t.Setenv("OTEL_EXPORTER_OTLP_ENDPOINT", "") - // Call InitTracing + prev := any(otel.GetTracerProvider()) shutdown, err := InitTracing(context.Background()) - // Should use defaults - assert.NoError(t, err) - assert.NotNil(t, shutdown) + assertTracingDisabled(t, prev, shutdown, err) +} - // Cleanup - if shutdown != nil { - err := shutdown(context.Background()) - assert.NoError(t, err) - } +func TestInitTracing_Success(t *testing.T) { + enableTracing(t, "localhost:4317") + t.Setenv("OTEL_SERVICE_NAME", "test-service") + + prev := any(otel.GetTracerProvider()) + shutdown, err := InitTracing(context.Background()) + + // Should not error even if the endpoint is unavailable -- export failures + // surface later, in the batch processor, not at init. + require.NoError(t, err) + require.NotNil(t, shutdown) + assert.False(t, any(otel.GetTracerProvider()) == prev, + "a global TracerProvider should be installed when both gates pass") + + assert.NoError(t, shutdown(context.Background())) } func TestInitTracing_WithCustomEndpoint(t *testing.T) { - // Set custom endpoint - os.Setenv("OTEL_EXPORTER_OTLP_ENDPOINT", "custom-endpoint:4318") - os.Setenv("OTEL_SERVICE_NAME", "custom-service") - defer os.Unsetenv("OTEL_EXPORTER_OTLP_ENDPOINT") - defer os.Unsetenv("OTEL_SERVICE_NAME") + enableTracing(t, "custom-endpoint:4318") + t.Setenv("OTEL_SERVICE_NAME", "custom-service") - // Call InitTracing shutdown, err := InitTracing(context.Background()) - // Should not error (endpoint may not exist, but that's OK) - assert.NoError(t, err) - assert.NotNil(t, shutdown) + require.NoError(t, err) + require.NotNil(t, shutdown) + assert.NoError(t, shutdown(context.Background())) +} + +// The http:// prefix is stripped before reaching the exporter, which expects +// host:port. This is the form the k8s configmap actually supplied. +func TestInitTracing_StripsSchemeFromEndpoint(t *testing.T) { + enableTracing(t, "http://jaeger.monitoring.svc.cluster.local:4318") - // Cleanup - if shutdown != nil { - err := shutdown(context.Background()) - assert.NoError(t, err) - } + shutdown, err := InitTracing(context.Background()) + + require.NoError(t, err) + require.NotNil(t, shutdown) + assert.NoError(t, shutdown(context.Background())) } func TestInitTracing_ShutdownIsIdempotent(t *testing.T) { - // Initialize tracing + enableTracing(t, "localhost:4318") + shutdown, err := InitTracing(context.Background()) require.NoError(t, err) require.NotNil(t, shutdown) - // Call shutdown multiple times - should not panic or error - err = shutdown(context.Background()) - assert.NoError(t, err) - - err = shutdown(context.Background()) - assert.NoError(t, err) + // Calling shutdown repeatedly should neither panic nor error. + assert.NoError(t, shutdown(context.Background())) + assert.NoError(t, shutdown(context.Background())) } func TestInitTracing_CreatesSpans(t *testing.T) { - // Initialize tracing + enableTracing(t, "localhost:4318") + shutdown, err := InitTracing(context.Background()) require.NoError(t, err) require.NotNil(t, shutdown) defer shutdown(context.Background()) - // Get tracer from global provider tracer := otel.Tracer("test-tracer") - assert.NotNil(t, tracer) + require.NotNil(t, tracer) - // Create a test span ctx, span := tracer.Start(context.Background(), "test-operation") - assert.NotNil(t, span) + require.NotNil(t, span) - // Span should be recording (even if export fails due to no Jaeger) - assert.True(t, span.IsRecording(), "Span should be recording") + // Recording is independent of whether export succeeds -- there is no + // collector listening in tests. + assert.True(t, span.IsRecording(), "span should be recording when tracing is enabled") span.End() - - // Verify context contains trace info assert.NotNil(t, ctx) }