Make OTel tracing opt-in instead of always-on

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)

internal/telemetry/tracing_test.go

@@ -2,113 +2,142 @@ package telemetry
 
 import (
 	"context"
-	"os"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"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)
 }