From 028aed0b3a12f4d8497a2644fd4c9e2315264ff2 Mon Sep 17 00:00:00 2001 From: "thariq.satyagraha" Date: Sun, 12 Apr 2026 16:33:11 +0700 Subject: [PATCH 1/2] feat: extensible OTEL span filtering and naming Add configurable span filtering and transformation for OpenTelemetry traces: - SpanFilter/SpanTransformer interfaces for client extensibility - OTEL_GRPC_SPAN_NAME_FORMAT=short|full for gRPC span naming - OTEL_FILTER_SPAN_NAMES for exact-match span filtering - AddOTELSpanFilter() and AddOTELSpanTransformer() runtime APIs --- config/config.go | 8 + core.go | 49 ++--- initializers.go | 51 ++++- otel/spanprocessor.go | 183 ++++++++++++++++ otel/spanprocessor_test.go | 412 +++++++++++++++++++++++++++++++++++++ 5 files changed, 674 insertions(+), 29 deletions(-) create mode 100644 otel/spanprocessor.go create mode 100644 otel/spanprocessor_test.go diff --git a/config/config.go b/config/config.go index e7303ad..9980603 100644 --- a/config/config.go +++ b/config/config.go @@ -179,6 +179,14 @@ type Config struct { // EnableOTELMetrics enables OpenTelemetry metrics export via OTLP alongside // Prometheus. Does not replace Prometheus. Default false. EnableOTELMetrics bool `envconfig:"ENABLE_OTEL_METRICS" default:"false"` + + // OTELGRPCSpanNameFormat controls gRPC span naming. + // "short" extracts just the method name (e.g., "V0GetStats") + // "full" keeps the full path (e.g., "/pkg.Service/V0GetStats") - default + OTELGRPCSpanNameFormat string `envconfig:"OTEL_GRPC_SPAN_NAME_FORMAT" default:"full"` + // OTELFilterSpanNames is a comma-separated list of span names to filter out (exact match). + // Common use: "ServeHTTP" to filter HTTP transport spans. + OTELFilterSpanNames string `envconfig:"OTEL_FILTER_SPAN_NAMES" default:""` // OTELMetricsInterval controls the export interval in seconds for OTEL // metrics. Default 60. OTELMetricsInterval int `envconfig:"OTEL_METRICS_INTERVAL" default:"60"` diff --git a/core.go b/core.go index a85700d..19c1dc8 100644 --- a/core.go +++ b/core.go @@ -180,38 +180,33 @@ func (c *cb) processConfig() { if c.config.OTLPEndpoint != "" { headers := parseHeaders(c.config.OTLPHeaders) otlpConfig = OTLPConfig{ - Endpoint: c.config.OTLPEndpoint, - Headers: headers, - ServiceName: c.config.AppName, - ServiceVersion: c.config.ReleaseName, - SamplingRatio: c.config.OTLPSamplingRatio, - Compression: c.config.OTLPCompression, - Insecure: c.config.OTLPInsecure, + Endpoint: c.config.OTLPEndpoint, + Headers: headers, + ServiceName: c.config.AppName, + ServiceVersion: c.config.ReleaseName, + SamplingRatio: c.config.OTLPSamplingRatio, + Compression: c.config.OTLPCompression, + Insecure: c.config.OTLPInsecure, + GRPCSpanNameFormat: c.config.OTELGRPCSpanNameFormat, + FilterSpanNames: c.config.OTELFilterSpanNames, } if err := SetupOpenTelemetry(otlpConfig); err != nil { log.Error(context.Background(), "msg", "Failed to setup custom OTLP", "err", err) } - } else if c.config.NewRelicOpentelemetry { - err := SetupNROpenTelemetry( - nrName, - c.config.NewRelicLicenseKey, - c.config.ReleaseName, - c.config.NewRelicOpentelemetrySample, - ) - if err != nil { - log.Error(context.Background(), "msg", "Failed to setup New Relic OpenTelemetry", "err", err) + } else if c.config.NewRelicOpentelemetry && strings.TrimSpace(c.config.NewRelicLicenseKey) != "" { + // Build full config for NR path to include filter/transformer settings. + otlpConfig = OTLPConfig{ + Endpoint: nrOTLPEndpoint, + Headers: map[string]string{"api-key": c.config.NewRelicLicenseKey}, + ServiceName: nrName, + ServiceVersion: c.config.ReleaseName, + SamplingRatio: c.config.NewRelicOpentelemetrySample, + Compression: "gzip", + GRPCSpanNameFormat: c.config.OTELGRPCSpanNameFormat, + FilterSpanNames: c.config.OTELFilterSpanNames, } - // Build otlpConfig for NR path so OTEL metrics can reuse the endpoint. - // Only populate when the license key is non-empty (SetupNROpenTelemetry - // no-ops without it, so metrics would just get auth failures). - if strings.TrimSpace(c.config.NewRelicLicenseKey) != "" { - otlpConfig = OTLPConfig{ - Endpoint: nrOTLPEndpoint, - Headers: map[string]string{"api-key": c.config.NewRelicLicenseKey}, - ServiceName: nrName, - ServiceVersion: c.config.ReleaseName, - Compression: "gzip", - } + if err := SetupOpenTelemetry(otlpConfig); err != nil { + log.Error(context.Background(), "msg", "Failed to setup New Relic OpenTelemetry", "err", err) } } diff --git a/initializers.go b/initializers.go index 48b19a1..b7889c8 100644 --- a/initializers.go +++ b/initializers.go @@ -13,6 +13,7 @@ import ( "time" metricCollector "github.com/afex/hystrix-go/hystrix/metric_collector" + cbotel "github.com/go-coldbrew/core/otel" "github.com/go-coldbrew/errors/notifier" "github.com/go-coldbrew/hystrixprometheus" //nolint:staticcheck // deprecated but still in use "github.com/go-coldbrew/interceptors" @@ -26,9 +27,9 @@ import ( "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetricgrpc" "go.opentelemetry.io/otel/exporters/otlp/otlptrace" - "go.opentelemetry.io/otel/propagation" "go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc" otelmetric "go.opentelemetry.io/otel/metric" + "go.opentelemetry.io/otel/propagation" "go.opentelemetry.io/otel/sdk/resource" sdkmetric "go.opentelemetry.io/otel/sdk/metric" sdktrace "go.opentelemetry.io/otel/sdk/trace" @@ -145,6 +146,15 @@ type OTLPConfig struct { // Insecure disables TLS verification for the connection // Only use this for local development or testing Insecure bool + + // GRPCSpanNameFormat controls gRPC span naming. + // "short" extracts just the method name (e.g., "V0GetStats") + // "full" keeps the full path (e.g., "/pkg.Service/V0GetStats") - default + GRPCSpanNameFormat string + + // FilterSpanNames is a comma-separated string of span names to filter out. + // Common use: "ServeHTTP" to filter HTTP transport spans. + FilterSpanNames string } // nrOTLPEndpoint is the New Relic OTLP gRPC endpoint. @@ -157,6 +167,9 @@ var otelResource *resource.Resource // otelTracerProvider stores the concrete TracerProvider for shutdown. var otelTracerProvider *sdktrace.TracerProvider +// otelSpanProcessor stores the custom span processor for runtime filter/transformer additions. +var otelSpanProcessor *cbotel.SpanProcessor + // buildOTELResource builds a resource with service name, version, build info, // and VCS metadata. The result is cached in otelResource for reuse. func buildOTELResource(serviceName, serviceVersion string) (*resource.Resource, error) { @@ -268,9 +281,25 @@ func SetupOpenTelemetry(config OTLPConfig) error { ratio = 0.2 } + // Wrap the batcher with custom SpanProcessor for filtering/transformation. + batcher := sdktrace.NewBatchSpanProcessor(otlpExporter) + var filterNames []string + if config.FilterSpanNames != "" { + for _, name := range strings.Split(config.FilterSpanNames, ",") { + if trimmed := strings.TrimSpace(name); trimmed != "" { + filterNames = append(filterNames, trimmed) + } + } + } + processor := cbotel.NewSpanProcessor(batcher, cbotel.SpanProcessorConfig{ + GRPCSpanNameFormat: config.GRPCSpanNameFormat, + FilterSpanNames: filterNames, + }) + otelSpanProcessor = processor + tracerProvider := sdktrace.NewTracerProvider( sdktrace.WithSampler(sdktrace.ParentBased(sdktrace.TraceIDRatioBased(ratio))), - sdktrace.WithBatcher(otlpExporter), + sdktrace.WithSpanProcessor(processor), sdktrace.WithResource(r), ) otelTracerProvider = tracerProvider @@ -506,3 +535,21 @@ func (vtprotoCodec) Name() string { // name registered for the proto compressor return "proto" } + +// AddOTELSpanFilter adds a custom span filter at runtime. +// Filters are checked for each span; if any filter returns true, the span is dropped. +// Must be called after SetupOpenTelemetry; no-op if OTEL is not initialized. +func AddOTELSpanFilter(f cbotel.SpanFilter) { + if otelSpanProcessor != nil { + otelSpanProcessor.AddFilter(f) + } +} + +// AddOTELSpanTransformer adds a custom span transformer at runtime. +// Transformers are applied in order; first non-empty result wins. +// Must be called after SetupOpenTelemetry; no-op if OTEL is not initialized. +func AddOTELSpanTransformer(t cbotel.SpanTransformer) { + if otelSpanProcessor != nil { + otelSpanProcessor.AddTransformer(t) + } +} diff --git a/otel/spanprocessor.go b/otel/spanprocessor.go new file mode 100644 index 0000000..408fb31 --- /dev/null +++ b/otel/spanprocessor.go @@ -0,0 +1,183 @@ +// Package otel provides OpenTelemetry utilities for ColdBrew services. +package otel + +import ( + "context" + "strings" + "sync" + + sdktrace "go.opentelemetry.io/otel/sdk/trace" +) + +// SpanFilter allows clients to implement custom span filtering logic. +// Return true to drop the span (not export it), false to keep it. +type SpanFilter interface { + // ShouldDrop returns true if the span should be filtered out. + ShouldDrop(span sdktrace.ReadOnlySpan) bool +} + +// SpanFilterFunc is a function adapter for SpanFilter interface. +type SpanFilterFunc func(span sdktrace.ReadOnlySpan) bool + +// ShouldDrop implements SpanFilter. +func (f SpanFilterFunc) ShouldDrop(span sdktrace.ReadOnlySpan) bool { + return f(span) +} + +// SpanTransformer allows clients to transform span data before export. +// This is useful for renaming spans, adding attributes, etc. +type SpanTransformer interface { + // Transform returns a modified span name. Return empty string to keep original. + Transform(span sdktrace.ReadOnlySpan) string +} + +// SpanTransformerFunc is a function adapter for SpanTransformer interface. +type SpanTransformerFunc func(span sdktrace.ReadOnlySpan) string + +// Transform implements SpanTransformer. +func (f SpanTransformerFunc) Transform(span sdktrace.ReadOnlySpan) string { + return f(span) +} + +// SpanProcessorConfig configures the custom span processor. +type SpanProcessorConfig struct { + // GRPCSpanNameFormat controls gRPC span naming. + // "short" extracts just the method name (e.g., "V0GetStats") + // "full" keeps the full path (e.g., "/pkg.Service/V0GetStats") - default + GRPCSpanNameFormat string + + // FilterSpanNames is a list of span names to filter out (exact match). + // Common use: []string{"ServeHTTP"} to filter HTTP transport spans. + FilterSpanNames []string + + // Filters are custom filters provided by the client. + // All filters are checked; if any returns true, the span is dropped. + Filters []SpanFilter + + // Transformers are custom transformers provided by the client. + // Applied in order; first non-empty result wins. + Transformers []SpanTransformer +} + +// SpanProcessor wraps a SpanProcessor with filtering and transformation. +// It implements sdktrace.SpanProcessor. +type SpanProcessor struct { + next sdktrace.SpanProcessor + config SpanProcessorConfig + filterNames map[string]struct{} + mu sync.RWMutex +} + +// NewSpanProcessor creates a new SpanProcessor wrapping the given processor. +func NewSpanProcessor(next sdktrace.SpanProcessor, config SpanProcessorConfig) *SpanProcessor { + filterNames := make(map[string]struct{}, len(config.FilterSpanNames)) + for _, name := range config.FilterSpanNames { + filterNames[name] = struct{}{} + } + return &SpanProcessor{ + next: next, + config: config, + filterNames: filterNames, + } +} + +// OnStart is called when a span starts. +func (p *SpanProcessor) OnStart(parent context.Context, s sdktrace.ReadWriteSpan) { + p.next.OnStart(parent, s) +} + +// OnEnd is called when a span ends. This is where filtering and transformation happen. +func (p *SpanProcessor) OnEnd(s sdktrace.ReadOnlySpan) { + // Check exact name filter + if _, ok := p.filterNames[s.Name()]; ok { + return // filtered out + } + + // Check custom filters (need lock since AddFilter can modify concurrently) + p.mu.RLock() + filters := p.config.Filters + p.mu.RUnlock() + for _, filter := range filters { + if filter.ShouldDrop(s) { + return // filtered out + } + } + + // Apply transformations if needed + // Note: ReadOnlySpan doesn't allow modification, so we use a wrapper + // that applies name transformation on read. + transformed := p.maybeTransform(s) + + p.next.OnEnd(transformed) +} + +// maybeTransform applies transformations and returns a potentially wrapped span. +func (p *SpanProcessor) maybeTransform(s sdktrace.ReadOnlySpan) sdktrace.ReadOnlySpan { + newName := "" + + // Apply short gRPC span name format + if p.config.GRPCSpanNameFormat == "short" { + name := s.Name() + // gRPC spans have format "/pkg.Service/Method" + if strings.HasPrefix(name, "/") && strings.Contains(name, "/") { + parts := strings.Split(name, "/") + if len(parts) >= 2 { + newName = parts[len(parts)-1] + } + } + } + + // Apply custom transformers (first non-empty wins) + // Need lock since AddTransformer can modify concurrently + p.mu.RLock() + transformers := p.config.Transformers + p.mu.RUnlock() + for _, t := range transformers { + if result := t.Transform(s); result != "" { + newName = result + break + } + } + + if newName != "" && newName != s.Name() { + return &renamedSpan{ReadOnlySpan: s, name: newName} + } + return s +} + +// Shutdown shuts down the processor. +func (p *SpanProcessor) Shutdown(ctx context.Context) error { + return p.next.Shutdown(ctx) +} + +// ForceFlush forces a flush of the processor. +func (p *SpanProcessor) ForceFlush(ctx context.Context) error { + return p.next.ForceFlush(ctx) +} + +// AddFilter adds a custom filter at runtime. +// Thread-safe. +func (p *SpanProcessor) AddFilter(f SpanFilter) { + p.mu.Lock() + defer p.mu.Unlock() + p.config.Filters = append(p.config.Filters, f) +} + +// AddTransformer adds a custom transformer at runtime. +// Thread-safe. +func (p *SpanProcessor) AddTransformer(t SpanTransformer) { + p.mu.Lock() + defer p.mu.Unlock() + p.config.Transformers = append(p.config.Transformers, t) +} + +// renamedSpan wraps a ReadOnlySpan to return a different name. +type renamedSpan struct { + sdktrace.ReadOnlySpan + name string +} + +// Name returns the transformed span name. +func (s *renamedSpan) Name() string { + return s.name +} diff --git a/otel/spanprocessor_test.go b/otel/spanprocessor_test.go new file mode 100644 index 0000000..156fe06 --- /dev/null +++ b/otel/spanprocessor_test.go @@ -0,0 +1,412 @@ +package otel + +import ( + "context" + "sync" + "testing" + "time" + + "go.opentelemetry.io/otel/attribute" + "go.opentelemetry.io/otel/codes" + sdktrace "go.opentelemetry.io/otel/sdk/trace" + "go.opentelemetry.io/otel/sdk/trace/tracetest" + "go.opentelemetry.io/otel/trace" +) + +// mockSpanProcessor is a test double that records spans passed to OnEnd. +type mockSpanProcessor struct { + mu sync.Mutex + spans []sdktrace.ReadOnlySpan +} + +func (m *mockSpanProcessor) OnStart(ctx context.Context, s sdktrace.ReadWriteSpan) {} + +func (m *mockSpanProcessor) OnEnd(s sdktrace.ReadOnlySpan) { + m.mu.Lock() + defer m.mu.Unlock() + m.spans = append(m.spans, s) +} + +func (m *mockSpanProcessor) Shutdown(ctx context.Context) error { return nil } + +func (m *mockSpanProcessor) ForceFlush(ctx context.Context) error { return nil } + +func (m *mockSpanProcessor) getSpans() []sdktrace.ReadOnlySpan { + m.mu.Lock() + defer m.mu.Unlock() + result := make([]sdktrace.ReadOnlySpan, len(m.spans)) + copy(result, m.spans) + return result +} + +// createTestSpan creates a span with the given name and ends it immediately. +func createTestSpan(tp *sdktrace.TracerProvider, name string) { + _, span := tp.Tracer("test").Start(context.Background(), name) + span.End() +} + +func TestSpanProcessor_FilterByName(t *testing.T) { + mock := &mockSpanProcessor{} + processor := NewSpanProcessor(mock, SpanProcessorConfig{ + FilterSpanNames: []string{"ServeHTTP", "ignored"}, + }) + + tp := sdktrace.NewTracerProvider( + sdktrace.WithSpanProcessor(processor), + ) + + // Create spans + createTestSpan(tp, "ServeHTTP") // filtered + createTestSpan(tp, "MyHandler") // kept + createTestSpan(tp, "ignored") // filtered + createTestSpan(tp, "/api/v1/users") // kept + + spans := mock.getSpans() + if len(spans) != 2 { + t.Errorf("expected 2 spans, got %d", len(spans)) + } + + names := make(map[string]bool) + for _, s := range spans { + names[s.Name()] = true + } + if names["ServeHTTP"] || names["ignored"] { + t.Error("filtered spans should not appear") + } + if !names["MyHandler"] || !names["/api/v1/users"] { + t.Error("expected spans missing") + } +} + +func TestSpanProcessor_ShortGRPCFormat(t *testing.T) { + mock := &mockSpanProcessor{} + processor := NewSpanProcessor(mock, SpanProcessorConfig{ + GRPCSpanNameFormat: "short", + }) + + tp := sdktrace.NewTracerProvider( + sdktrace.WithSpanProcessor(processor), + ) + + // gRPC-style span names + createTestSpan(tp, "/pkg.Service/V0GetStats") + createTestSpan(tp, "/another.pkg.Svc/DoThing") + createTestSpan(tp, "NotGRPC") // should remain unchanged + + spans := mock.getSpans() + if len(spans) != 3 { + t.Fatalf("expected 3 spans, got %d", len(spans)) + } + + names := make(map[string]bool) + for _, s := range spans { + names[s.Name()] = true + } + + // Short format extracts method name + if !names["V0GetStats"] { + t.Error("expected V0GetStats") + } + if !names["DoThing"] { + t.Error("expected DoThing") + } + if !names["NotGRPC"] { + t.Error("expected NotGRPC unchanged") + } +} + +func TestSpanProcessor_FullGRPCFormat(t *testing.T) { + mock := &mockSpanProcessor{} + processor := NewSpanProcessor(mock, SpanProcessorConfig{ + GRPCSpanNameFormat: "full", + }) + + tp := sdktrace.NewTracerProvider( + sdktrace.WithSpanProcessor(processor), + ) + + createTestSpan(tp, "/pkg.Service/V0GetStats") + + spans := mock.getSpans() + if len(spans) != 1 { + t.Fatalf("expected 1 span, got %d", len(spans)) + } + if spans[0].Name() != "/pkg.Service/V0GetStats" { + t.Errorf("expected full name, got %s", spans[0].Name()) + } +} + +func TestSpanProcessor_CustomFilter(t *testing.T) { + mock := &mockSpanProcessor{} + processor := NewSpanProcessor(mock, SpanProcessorConfig{}) + + // Add custom filter that drops spans with "debug" in name + processor.AddFilter(SpanFilterFunc(func(s sdktrace.ReadOnlySpan) bool { + return s.Name() == "debug-span" + })) + + tp := sdktrace.NewTracerProvider( + sdktrace.WithSpanProcessor(processor), + ) + + createTestSpan(tp, "debug-span") // filtered by custom filter + createTestSpan(tp, "normal-span") + + spans := mock.getSpans() + if len(spans) != 1 { + t.Fatalf("expected 1 span, got %d", len(spans)) + } + if spans[0].Name() != "normal-span" { + t.Errorf("expected normal-span, got %s", spans[0].Name()) + } +} + +func TestSpanProcessor_CustomTransformer(t *testing.T) { + mock := &mockSpanProcessor{} + processor := NewSpanProcessor(mock, SpanProcessorConfig{}) + + // Add custom transformer that prefixes names + processor.AddTransformer(SpanTransformerFunc(func(s sdktrace.ReadOnlySpan) string { + if s.Name() == "transform-me" { + return "prefix:" + s.Name() + } + return "" // no transformation + })) + + tp := sdktrace.NewTracerProvider( + sdktrace.WithSpanProcessor(processor), + ) + + createTestSpan(tp, "transform-me") + createTestSpan(tp, "leave-alone") + + spans := mock.getSpans() + if len(spans) != 2 { + t.Fatalf("expected 2 spans, got %d", len(spans)) + } + + names := make(map[string]bool) + for _, s := range spans { + names[s.Name()] = true + } + + if !names["prefix:transform-me"] { + t.Error("expected transformed name prefix:transform-me") + } + if !names["leave-alone"] { + t.Error("expected leave-alone unchanged") + } +} + +func TestSpanProcessor_TransformerOverridesShortFormat(t *testing.T) { + mock := &mockSpanProcessor{} + processor := NewSpanProcessor(mock, SpanProcessorConfig{ + GRPCSpanNameFormat: "short", + }) + + // Custom transformer wins over built-in short format + processor.AddTransformer(SpanTransformerFunc(func(s sdktrace.ReadOnlySpan) string { + return "custom-name" + })) + + tp := sdktrace.NewTracerProvider( + sdktrace.WithSpanProcessor(processor), + ) + + createTestSpan(tp, "/pkg.Service/Method") + + spans := mock.getSpans() + if len(spans) != 1 { + t.Fatalf("expected 1 span, got %d", len(spans)) + } + if spans[0].Name() != "custom-name" { + t.Errorf("expected custom-name, got %s", spans[0].Name()) + } +} + +func TestSpanProcessor_ConcurrentAddFilter(t *testing.T) { + mock := &mockSpanProcessor{} + processor := NewSpanProcessor(mock, SpanProcessorConfig{}) + + tp := sdktrace.NewTracerProvider( + sdktrace.WithSpanProcessor(processor), + ) + + var wg sync.WaitGroup + // Concurrent filter additions + for i := 0; i < 10; i++ { + wg.Add(1) + go func() { + defer wg.Done() + processor.AddFilter(SpanFilterFunc(func(s sdktrace.ReadOnlySpan) bool { + return false + })) + }() + } + + // Concurrent span creation + for i := 0; i < 10; i++ { + wg.Add(1) + go func(n int) { + defer wg.Done() + createTestSpan(tp, "span-"+string(rune('a'+n))) + }(i) + } + + wg.Wait() + // Just verify no race/panic +} + +func TestSpanProcessor_ForceFlushAndShutdown(t *testing.T) { + mock := &mockSpanProcessor{} + processor := NewSpanProcessor(mock, SpanProcessorConfig{}) + + ctx := context.Background() + if err := processor.ForceFlush(ctx); err != nil { + t.Errorf("ForceFlush failed: %v", err) + } + if err := processor.Shutdown(ctx); err != nil { + t.Errorf("Shutdown failed: %v", err) + } +} + +func TestRenamedSpan_PreservesOtherFields(t *testing.T) { + // Create a real span to wrap + exporter := tracetest.NewInMemoryExporter() + tp := sdktrace.NewTracerProvider( + sdktrace.WithSyncer(exporter), + ) + + ctx, span := tp.Tracer("test").Start(context.Background(), "original-name", + trace.WithAttributes(attribute.String("key", "value")), + ) + span.SetStatus(codes.Error, "test error") + span.End() + _ = ctx + + // Get the exported span + spans := exporter.GetSpans() + if len(spans) != 1 { + t.Fatalf("expected 1 span, got %d", len(spans)) + } + + // Wrap it with renamedSpan + original := spans[0].Snapshot() + renamed := &renamedSpan{ReadOnlySpan: original, name: "new-name"} + + // Verify name is changed + if renamed.Name() != "new-name" { + t.Errorf("expected new-name, got %s", renamed.Name()) + } + + // Verify other fields preserved + if renamed.SpanContext().TraceID() != original.SpanContext().TraceID() { + t.Error("SpanContext TraceID not preserved") + } + if renamed.SpanContext().SpanID() != original.SpanContext().SpanID() { + t.Error("SpanContext SpanID not preserved") + } + if renamed.StartTime() != original.StartTime() { + t.Error("StartTime not preserved") + } + if renamed.EndTime() != original.EndTime() { + t.Error("EndTime not preserved") + } + if renamed.Status().Code != original.Status().Code { + t.Error("Status not preserved") + } +} + +func TestSpanProcessor_EmptyFilterNames(t *testing.T) { + mock := &mockSpanProcessor{} + processor := NewSpanProcessor(mock, SpanProcessorConfig{ + FilterSpanNames: []string{}, // empty list + }) + + tp := sdktrace.NewTracerProvider( + sdktrace.WithSpanProcessor(processor), + ) + + createTestSpan(tp, "any-span") + + spans := mock.getSpans() + if len(spans) != 1 { + t.Errorf("expected 1 span with empty filter list, got %d", len(spans)) + } +} + +func TestSpanProcessor_MultipleFiltersAllChecked(t *testing.T) { + mock := &mockSpanProcessor{} + processor := NewSpanProcessor(mock, SpanProcessorConfig{ + FilterSpanNames: []string{"first"}, + }) + + // Add another filter + processor.AddFilter(SpanFilterFunc(func(s sdktrace.ReadOnlySpan) bool { + return s.Name() == "second" + })) + + tp := sdktrace.NewTracerProvider( + sdktrace.WithSpanProcessor(processor), + ) + + createTestSpan(tp, "first") // filtered by name list + createTestSpan(tp, "second") // filtered by custom filter + createTestSpan(tp, "third") // kept + + spans := mock.getSpans() + if len(spans) != 1 { + t.Errorf("expected 1 span, got %d", len(spans)) + } + if spans[0].Name() != "third" { + t.Errorf("expected third, got %s", spans[0].Name()) + } +} + +// TestSpanFilterFunc verifies the function adapter implements the interface. +func TestSpanFilterFunc_Interface(t *testing.T) { + var f SpanFilter = SpanFilterFunc(func(s sdktrace.ReadOnlySpan) bool { + return true + }) + _ = f // compile-time check +} + +// TestSpanTransformerFunc verifies the function adapter implements the interface. +func TestSpanTransformerFunc_Interface(t *testing.T) { + var f SpanTransformer = SpanTransformerFunc(func(s sdktrace.ReadOnlySpan) string { + return "transformed" + }) + _ = f // compile-time check +} + +// TestSpanProcessor_FilterDurationBased demonstrates filtering spans by duration. +func TestSpanProcessor_FilterDurationBased(t *testing.T) { + mock := &mockSpanProcessor{} + processor := NewSpanProcessor(mock, SpanProcessorConfig{}) + + // Filter spans shorter than 10ms + processor.AddFilter(SpanFilterFunc(func(s sdktrace.ReadOnlySpan) bool { + return s.EndTime().Sub(s.StartTime()) < 10*time.Millisecond + })) + + tp := sdktrace.NewTracerProvider( + sdktrace.WithSpanProcessor(processor), + ) + + // Create a quick span (should be filtered) + _, span1 := tp.Tracer("test").Start(context.Background(), "quick") + span1.End() + + // Create a slower span (should be kept) + _, span2 := tp.Tracer("test").Start(context.Background(), "slow") + time.Sleep(15 * time.Millisecond) + span2.End() + + spans := mock.getSpans() + if len(spans) != 1 { + t.Errorf("expected 1 span (slow), got %d", len(spans)) + } + if len(spans) > 0 && spans[0].Name() != "slow" { + t.Errorf("expected slow span, got %s", spans[0].Name()) + } +} From 3ef1438b3036a9302ac4cb1f9706e272d2f780d0 Mon Sep 17 00:00:00 2001 From: "thariq.satyagraha" Date: Tue, 14 Apr 2026 09:43:42 +0700 Subject: [PATCH 2/2] fix: address MR review feedback - Add config validation for OTELGRPCSpanNameFormat (warn if not short/full) - Clarify SpanTransformer docs: renaming-only contract - Nil-safe AddFilter/AddTransformer to prevent OnEnd panics - Add tests for validation and nil handling --- config/config.go | 4 ++++ config/config_test.go | 30 ++++++++++++++++++++++++++++ otel/spanprocessor.go | 17 +++++++++++----- otel/spanprocessor_test.go | 41 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 87 insertions(+), 5 deletions(-) diff --git a/config/config.go b/config/config.go index 9980603..128fcf1 100644 --- a/config/config.go +++ b/config/config.go @@ -299,5 +299,9 @@ func (c Config) Validate() []string { warnings = append(warnings, "RateLimitBurst should be positive when RateLimitPerSecond is set") } + if c.OTELGRPCSpanNameFormat != "" && c.OTELGRPCSpanNameFormat != "short" && c.OTELGRPCSpanNameFormat != "full" { + warnings = append(warnings, "OTELGRPCSpanNameFormat must be 'short' or 'full', got '"+c.OTELGRPCSpanNameFormat+"'; defaulting to 'full'") + } + return warnings } diff --git a/config/config_test.go b/config/config_test.go index 4ef2364..b4b0065 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -272,3 +272,33 @@ func TestValidateTimeoutExceedsShutdown(t *testing.T) { t.Error("timeout exceeding shutdown duration should produce a warning") } } + +func TestValidateOTELGRPCSpanNameFormat(t *testing.T) { + // Invalid format + c := Config{ + GRPCPort: 9090, + HTTPPort: 9091, + OTELGRPCSpanNameFormat: "invalid", + } + warnings := c.Validate() + found := false + for _, w := range warnings { + if strings.Contains(w, "OTELGRPCSpanNameFormat") { + found = true + } + } + if !found { + t.Error("invalid OTELGRPCSpanNameFormat should produce a warning") + } + + // Valid formats should not warn + for _, format := range []string{"short", "full", ""} { + c.OTELGRPCSpanNameFormat = format + warnings = c.Validate() + for _, w := range warnings { + if strings.Contains(w, "OTELGRPCSpanNameFormat") { + t.Errorf("format %q should not produce a warning, got: %s", format, w) + } + } + } +} diff --git a/otel/spanprocessor.go b/otel/spanprocessor.go index 408fb31..ac1691a 100644 --- a/otel/spanprocessor.go +++ b/otel/spanprocessor.go @@ -24,10 +24,11 @@ func (f SpanFilterFunc) ShouldDrop(span sdktrace.ReadOnlySpan) bool { return f(span) } -// SpanTransformer allows clients to transform span data before export. -// This is useful for renaming spans, adding attributes, etc. +// SpanTransformer allows clients to rename spans before export. +// Note: This is a renaming-only contract; no other span data is modified. type SpanTransformer interface { - // Transform returns a modified span name. Return empty string to keep original. + // Transform returns a new span name. Return empty string to keep the original name. + // Only the span name is changed; attributes and other span data remain unmodified. Transform(span sdktrace.ReadOnlySpan) string } @@ -156,16 +157,22 @@ func (p *SpanProcessor) ForceFlush(ctx context.Context) error { } // AddFilter adds a custom filter at runtime. -// Thread-safe. +// Thread-safe. Nil filters are ignored to prevent panics in OnEnd. func (p *SpanProcessor) AddFilter(f SpanFilter) { + if f == nil { + return + } p.mu.Lock() defer p.mu.Unlock() p.config.Filters = append(p.config.Filters, f) } // AddTransformer adds a custom transformer at runtime. -// Thread-safe. +// Thread-safe. Nil transformers are ignored to prevent panics in OnEnd. func (p *SpanProcessor) AddTransformer(t SpanTransformer) { + if t == nil { + return + } p.mu.Lock() defer p.mu.Unlock() p.config.Transformers = append(p.config.Transformers, t) diff --git a/otel/spanprocessor_test.go b/otel/spanprocessor_test.go index 156fe06..2035ad6 100644 --- a/otel/spanprocessor_test.go +++ b/otel/spanprocessor_test.go @@ -410,3 +410,44 @@ func TestSpanProcessor_FilterDurationBased(t *testing.T) { t.Errorf("expected slow span, got %s", spans[0].Name()) } } + +func TestSpanProcessor_NilFilterIgnored(t *testing.T) { + mock := &mockSpanProcessor{} + processor := NewSpanProcessor(mock, SpanProcessorConfig{}) + + // Adding nil filter should be ignored (no panic) + processor.AddFilter(nil) + + tp := sdktrace.NewTracerProvider( + sdktrace.WithSpanProcessor(processor), + ) + + createTestSpan(tp, "test-span") + + spans := mock.getSpans() + if len(spans) != 1 { + t.Errorf("expected 1 span, got %d", len(spans)) + } +} + +func TestSpanProcessor_NilTransformerIgnored(t *testing.T) { + mock := &mockSpanProcessor{} + processor := NewSpanProcessor(mock, SpanProcessorConfig{}) + + // Adding nil transformer should be ignored (no panic) + processor.AddTransformer(nil) + + tp := sdktrace.NewTracerProvider( + sdktrace.WithSpanProcessor(processor), + ) + + createTestSpan(tp, "test-span") + + spans := mock.getSpans() + if len(spans) != 1 { + t.Errorf("expected 1 span, got %d", len(spans)) + } + if spans[0].Name() != "test-span" { + t.Errorf("expected test-span, got %s", spans[0].Name()) + } +}