diff --git a/cmd/thv/app/server.go b/cmd/thv/app/server.go index 1cdb301f80..5058807ac9 100644 --- a/cmd/thv/app/server.go +++ b/cmd/thv/app/server.go @@ -58,9 +58,8 @@ var serveCmd = &cobra.Command{ env = os.Getenv("SENTRY_ENVIRONMENT") } - // Initialize Sentry for error reporting and panic capture. - // Must happen before telemetry.NewServeProvider so the Sentry span - // processor is registered in time to be picked up by NewProvider. + // Initialize Sentry for error reporting and trace export. This must happen + // before telemetry.NewServeProvider so its trace exporter is registered. sentryCfg := sentrypkg.Config{ DSN: dsn, Environment: env, @@ -72,18 +71,16 @@ var serveCmd = &cobra.Command{ } // Initialize OTEL provider from global config (thv config otel set-endpoint). - // If Sentry is also initialized, the Sentry span processor is wired in so spans - // are exported to both the configured OTLP backend and Sentry simultaneously. + // When Sentry is initialized, its trace exporter is added as a span processor, + // so spans reach both the configured OTLP backend and Sentry. otelProvider, otelEnabled, err := telemetry.NewServeProvider(ctx) if err != nil { return err } // Shutdown ordering is intentionally LIFO via defer: - // 1. OTEL provider shuts down first — flushes the Sentry span processor - // (which calls hub.Flush internally) before the Sentry client is closed. - // 2. Sentry client closes second — safe because the span processor has - // already flushed by the time sentrypkg.Close() runs. + // 1. OTEL provider shuts down first, flushing the Sentry trace exporter. + // 2. Sentry client closes second, after trace export has completed. // Using defer instead of a goroutine makes the ordering deterministic. if otelProvider != nil { defer func() { diff --git a/go.mod b/go.mod index 9e5fb06a7c..bb3fef575b 100644 --- a/go.mod +++ b/go.mod @@ -78,7 +78,9 @@ require ( sigs.k8s.io/yaml v1.6.0 ) -require github.com/getsentry/sentry-go/otel v0.44.1 +require github.com/getsentry/sentry-go/otel v0.49.0 + +require github.com/getsentry/sentry-go/otel/otlp v0.49.0 require github.com/hashicorp/golang-lru/v2 v2.0.7 @@ -169,7 +171,7 @@ require ( github.com/fatih/color v1.18.0 // indirect github.com/fsnotify/fsnotify v1.9.0 // indirect github.com/fxamacker/cbor/v2 v2.9.0 // indirect - github.com/getsentry/sentry-go v0.47.0 + github.com/getsentry/sentry-go v0.49.0 github.com/go-git/gcfg v1.5.1-0.20230307220236-3a3c6141e376 // indirect github.com/go-logr/zapr v1.3.0 // indirect github.com/go-ole/go-ole v1.3.0 // indirect diff --git a/go.sum b/go.sum index 0919fecab6..ebdc4aca0a 100644 --- a/go.sum +++ b/go.sum @@ -228,10 +228,12 @@ github.com/fsnotify/fsnotify v1.9.0 h1:2Ml+OJNzbYCTzsxtv8vKSFD9PbJjmhYF14k/jKC7S github.com/fsnotify/fsnotify v1.9.0/go.mod h1:8jBTzvmWwFyi3Pb8djgCCO5IBqzKJ/Jwo8TRcHyHii0= github.com/fxamacker/cbor/v2 v2.9.0 h1:NpKPmjDBgUfBms6tr6JZkTHtfFGcMKsw3eGcmD/sapM= github.com/fxamacker/cbor/v2 v2.9.0/go.mod h1:vM4b+DJCtHn+zz7h3FFp/hDAI9WNWCsZj23V5ytsSxQ= -github.com/getsentry/sentry-go v0.47.0 h1:AnSMSyrYA5qZCIN/2xpgAAwv63sVULV+vBq37ajouc8= -github.com/getsentry/sentry-go v0.47.0/go.mod h1:h+b4VHpKnK7aUXB5wc+KDnPgp9ZtfliRD4eV85FbiSA= -github.com/getsentry/sentry-go/otel v0.44.1 h1:RV2zUHEvGHJmCCpMaJ52tZZAlcbMgvtasQn/g3CcKKc= -github.com/getsentry/sentry-go/otel v0.44.1/go.mod h1:CfzTxocQJ6JX4SLFvnBrGULBAARFAd1fHmbJCTQlOP4= +github.com/getsentry/sentry-go v0.49.0 h1:Ehejknu1l023Ub7QoRBVLAI7g3Jnhqku4oWx4B4Sh5s= +github.com/getsentry/sentry-go v0.49.0/go.mod h1:nuMJAoCfe1u0Bts2ocyNI+TW8HT84vRMqwA5Qq/SKUI= +github.com/getsentry/sentry-go/otel v0.49.0 h1:BRMzf4PqYEGsgxNS8BMX54I0DYVcBWbCtQ1s+aS67n0= +github.com/getsentry/sentry-go/otel v0.49.0/go.mod h1:FVrBdl+7ofh9neiuGLYtaGwEIv8fsW8LmV62pYMIrKk= +github.com/getsentry/sentry-go/otel/otlp v0.49.0 h1:gZgeRBzBQ2utfwno7MqSGXXRLdxX+48pBjp1WDAcb00= +github.com/getsentry/sentry-go/otel/otlp v0.49.0/go.mod h1:xq+r0C0F5T6JZwNOud7168JZLXXfPJ6jT0Y0vkrhAds= github.com/github/smimesign v0.2.0 h1:Hho4YcX5N1I9XNqhq0fNx0Sts8MhLonHd+HRXVGNjvk= github.com/github/smimesign v0.2.0/go.mod h1:iZiiwNT4HbtGRVqCQu7uJPEZCuEE5sfSSttcnePkDl4= github.com/gkampitakis/ciinfo v0.3.2 h1:JcuOPk8ZU7nZQjdUhctuhQofk7BGHuIy0c9Ez8BNhXs= diff --git a/pkg/api/errors/handler.go b/pkg/api/errors/handler.go index cf774263de..1e5ae0561a 100644 --- a/pkg/api/errors/handler.go +++ b/pkg/api/errors/handler.go @@ -57,8 +57,7 @@ func ErrorHandler(fn HandlerWithError) http.HandlerFunc { // runtimes that may include connection strings) to external backends. span.RecordError(fmt.Errorf("internal server error")) span.SetStatus(codes.Error, "internal server error") - // Sentry span processor only creates transactions; call CaptureException - // explicitly so 5xx errors also appear as Issues in the Sentry Issues tab. + // Capture the exception explicitly so 5xx errors also appear as Sentry Issues. sentrypkg.CaptureException(r, err) if isUpstreamStatus(code) { diff --git a/pkg/sentry/sentry.go b/pkg/sentry/sentry.go index 84e5c4d21b..59057d59fa 100644 --- a/pkg/sentry/sentry.go +++ b/pkg/sentry/sentry.go @@ -5,14 +5,18 @@ package sentry import ( + "context" "fmt" "log/slog" "net/http" + "sync" "sync/atomic" "time" "github.com/getsentry/sentry-go" sentryotel "github.com/getsentry/sentry-go/otel" + sentryotlp "github.com/getsentry/sentry-go/otel/otlp" + sdktrace "go.opentelemetry.io/otel/sdk/trace" "github.com/stacklok/toolhive/pkg/telemetry" "github.com/stacklok/toolhive/pkg/updates" @@ -21,9 +25,30 @@ import ( const flushTimeout = 2 * time.Second +const ( + // environmentKey and releaseKey are the attribute names Sentry itself uses + // to carry Environment and Release on OTLP payloads (see sentry-go's + // log.go/metrics.go), so exported spans must use the same names to be + // grouped alongside Issues. + environmentKey = "sentry.environment" + releaseKey = "sentry.release" + // instanceIDKey carries the anonymous instance ID on both Sentry events + // (as a scope tag) and exported spans (as a resource attribute), so Issues + // and Traces can be correlated with toolhive-studio by the same value. + instanceIDKey = "custom.instance_id" +) + // initialized tracks whether Sentry was successfully initialized. var initialized atomic.Bool +// spanProcessor is the single Sentry OTLP span processor for this process, +// created on the first Init and reused by every subsequent one. Guarded by +// spanProcessorMu. +var ( + spanProcessorMu sync.Mutex + spanProcessor sdktrace.SpanProcessor +) + // Config holds the configuration for Sentry integration. type Config struct { // DSN is the Sentry Data Source Name. When empty, Sentry is disabled. @@ -46,21 +71,30 @@ func Init(cfg Config) error { } vi := versions.GetVersionInfo() + // Reused verbatim as a span resource attribute below so Issues and Traces + // report the same release string. + release := fmt.Sprintf("toolhive@%s", vi.Version) err := sentry.Init(sentry.ClientOptions{ Dsn: cfg.DSN, Environment: cfg.Environment, - Release: fmt.Sprintf("toolhive@%s", vi.Version), + Release: release, TracesSampleRate: cfg.TracesSampleRate, Debug: cfg.Debug, EnableTracing: true, AttachStacktrace: true, - SendDefaultPII: false, + DataCollection: noPIIDataCollection(), + Integrations: func(integrations []sentry.Integration) []sentry.Integration { + return append(integrations, sentryotel.NewOtelIntegration()) + }, }) if err != nil { return fmt.Errorf("sentry init: %w", err) } + if err := registerTraceExporter(cfg); err != nil { + return err + } initialized.Store(true) slog.Debug("sentry initialized", "environment", cfg.Environment) @@ -69,22 +103,121 @@ func Init(cfg Config) error { // toolhive-studio. Note: toolhive-studio currently uses "custom.user_id" // for the same value; these should be aligned to "custom.instance_id" in // both repos in a follow-up to avoid misleading PII detection heuristics. + instanceID := "" if id, err := updates.TryGetAnonymousID(); err == nil && id != "" { + instanceID = id sentry.ConfigureScope(func(scope *sentry.Scope) { - scope.SetTag("custom.instance_id", id) + scope.SetTag(instanceIDKey, id) }) slog.Debug("sentry anonymous instance ID tagged", "id", id) } - // Self-register the Sentry span processor with the global OTEL registry so - // that any telemetry.NewProvider call automatically includes it. This decouples - // the OTEL provider setup from Sentry-specific code. - telemetry.RegisterSpanProcessor(sentryotel.NewSentrySpanProcessor()) - slog.Debug("sentry span processor registered with OTEL registry") + // Spans are exported straight to Sentry's OTLP endpoint and never pass + // through the Sentry client, so neither ClientOptions nor the scope + // configured above reach them. Environment, release and instance ID have to + // travel as OTEL resource attributes instead, or Traces would lose the + // grouping that Issues keep and the two would disagree. + telemetry.RegisterResourceAttributes(resourceAttributes(cfg.Environment, release, instanceID)) + + return nil +} + +// registerTraceExporter registers the Sentry OTLP span processor with the global +// OTEL registry, creating it on first use. +// +// The processor is cached because the registry deduplicates by pointer identity: +// sdktrace.NewBatchSpanProcessor allocates a fresh processor on every call, so +// without this a second Init (config reload, or a test that does not reset the +// registry) would register a second processor and double-export every span while +// leaking the first exporter's goroutine. +// +// Caching means a second Init keeps the first call's DSN. thv serve calls Init +// exactly once per process, and the registry already only feeds processors to +// providers created after registration, so re-initialising is not supported +// either way. +func registerTraceExporter(cfg Config) error { + spanProcessorMu.Lock() + defer spanProcessorMu.Unlock() + if spanProcessor == nil { + exporter, err := sentryotlp.NewTraceExporter(context.Background(), cfg.DSN) + if err != nil { + return fmt.Errorf("create Sentry trace exporter: %w", err) + } + spanProcessor = sdktrace.NewBatchSpanProcessor(exporter) + } + + telemetry.RegisterSpanProcessor(spanProcessor) + // Spans no longer pass through the Sentry client, so TracesSampleRate has to + // reach the OTEL sampler or --sentry-traces-sample-rate would be ignored and + // every span would be exported. + telemetry.RegisterSamplingRate(cfg.TracesSampleRate) + slog.Debug("sentry trace exporter registered with OTEL registry", + "traces_sample_rate", cfg.TracesSampleRate) return nil } +// piiSensitiveTerms mirrors the deny-list that sentry-go applies internally for +// SendDefaultPII=false (its unexported extendedSensitiveTerms). These cover +// client-identifying data that an API server behind a proxy routinely sees: +// forwarding headers, remote addresses and user identifiers. +// +// The list has to be repeated here because sentry-go reaches it only through +// the deprecated SendDefaultPII path; the DataCollection API exposes no way to +// set it. In CollectionDenyList mode a behaviour's Terms are OR-ed with the +// SDK's built-in terms, so passing them per behaviour is equivalent. +// +// Re-check this against sentry-go's extendedSensitiveTerms on SDK upgrades — a +// term added upstream will not reach us automatically. +var piiSensitiveTerms = []string{ + "forwarded", + "-ip", + "remote-", + "via", + "-user", +} + +// noPIIDataCollection returns the DataCollection that replaces the deprecated +// SendDefaultPII=false. It is deliberately equivalent to what sentry-go's +// legacyDataCollection built for that flag: no auto-populated user info, no +// HTTP bodies, no cookies, and headers and query params scrubbed against both +// the built-in and the extended deny-lists. +func noPIIDataCollection() *sentry.DataCollection { + denyList := func() *sentry.KeyValueCollectionBehavior { + return &sentry.KeyValueCollectionBehavior{ + Mode: sentry.CollectionDenyList, + Terms: piiSensitiveTerms, + } + } + return &sentry.DataCollection{ + UserInfo: sentry.Set(false), + HTTPBodies: []sentry.BodyType{}, + Cookies: &sentry.KeyValueCollectionBehavior{Mode: sentry.CollectionOff}, + HTTPHeaders: &sentry.HeaderCollectionConfig{ + Request: denyList(), + Response: denyList(), + }, + QueryParams: denyList(), + } +} + +// resourceAttributes returns the OTEL resource attributes Sentry needs to group +// OTLP-ingested traces the same way it groups Issues. Empty values are omitted +// so they do not show up as blank attributes on other OTLP backends. +func resourceAttributes(environment, release, instanceID string) map[string]string { + attrs := make(map[string]string, 3) + for key, value := range map[string]string{ + environmentKey: environment, + releaseKey: release, + instanceIDKey: instanceID, + } { + if value != "" { + attrs[key] = value + } + } + return attrs +} + // Close flushes buffered Sentry events and shuts down the SDK. // Safe to call even when Sentry was not initialized. func Close() { @@ -107,8 +240,9 @@ func Enabled() bool { // // The API server's error handler calls this alongside span.RecordError so that // 5xx errors appear as both OTEL span errors (distributed tracing) and -// standalone Sentry Issues (error tracking). The Sentry span processor only -// creates transactions; explicit hub calls are required for Issues. +// standalone Sentry Issues (error tracking). The Sentry OTEL integration links +// those issues to the active OTEL trace; explicit hub calls are required to +// create Issues. func CaptureException(r *http.Request, err error) { if !initialized.Load() || err == nil { return @@ -117,7 +251,15 @@ func CaptureException(r *http.Request, err error) { if hub == nil { hub = sentry.CurrentHub().Clone() } - hub.CaptureException(err) + client := hub.Client() + if client == nil { + return + } + event := client.EventFromException(err, sentry.LevelError) + hub.CaptureEventWithHint(event, &sentry.EventHint{ + OriginalException: err, + Context: r.Context(), + }) } // RecoverPanic reports a recovered panic value to Sentry. diff --git a/pkg/sentry/sentry_test.go b/pkg/sentry/sentry_test.go index de804058fe..48a43f0b72 100644 --- a/pkg/sentry/sentry_test.go +++ b/pkg/sentry/sentry_test.go @@ -4,22 +4,26 @@ package sentry import ( + "context" "errors" "net/http" "net/http/httptest" "testing" gosentry "github.com/getsentry/sentry-go" + sentryotel "github.com/getsentry/sentry-go/otel" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + sdktrace "go.opentelemetry.io/otel/sdk/trace" "github.com/stacklok/toolhive/pkg/telemetry" + "github.com/stacklok/toolhive/pkg/versions" ) // These tests are deliberately NOT parallel because they mutate the package-level // `initialized` atomic, which is global shared state. -//nolint:paralleltest // mutates global initialized state +//nolint:paralleltest // mutates global initialized and telemetry registry state func TestInit(t *testing.T) { tests := []struct { name string @@ -33,7 +37,7 @@ func TestInit(t *testing.T) { wantEnabled: false, }, { - name: "valid DSN initializes Sentry", + name: "valid DSN initializes Sentry and registers its trace exporter", cfg: Config{ DSN: "https://examplePublicKey@o0.ingest.sentry.io/0", Environment: "test", @@ -52,8 +56,7 @@ func TestInit(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - initialized.Store(false) - defer initialized.Store(false) + resetSentryForTest(t) err := Init(tt.cfg) if tt.wantErr { @@ -62,11 +65,111 @@ func TestInit(t *testing.T) { } require.NoError(t, err) assert.Equal(t, tt.wantEnabled, Enabled()) + assert.Equal(t, tt.wantEnabled, telemetry.HasRegisteredSpanProcessors(), + "Sentry initialization should register its trace exporter") }) } } -//nolint:paralleltest // mutates global initialized state +//nolint:paralleltest // mutates global initialized and telemetry registry state +func TestInit_RegistersExactlyOneSpanProcessor(t *testing.T) { + // Regression test: sdktrace.NewBatchSpanProcessor allocates a fresh + // processor per call, so registering it directly defeated the registry's + // pointer-identity dedup and double-exported every span on a second Init. + resetSentryForTest(t) + + cfg := Config{ + DSN: "https://examplePublicKey@o0.ingest.sentry.io/0", + Environment: "test", + TracesSampleRate: 1.0, + } + require.NoError(t, Init(cfg)) + require.NoError(t, Init(cfg)) + + assert.Equal(t, 1, telemetry.RegisteredSpanProcessorCount(), + "repeated Init must reuse the same span processor, not register a second one") +} + +//nolint:paralleltest // mutates global initialized and telemetry registry state +func TestInit_RegistersSamplingRate(t *testing.T) { + // Regression test: spans are exported straight to Sentry's OTLP endpoint and + // no longer pass through the Sentry client's sampler, so the rate has to + // reach the OTEL sampler or --sentry-traces-sample-rate is silently ignored + // and every span is exported. + resetSentryForTest(t) + + require.NoError(t, Init(Config{ + DSN: "https://examplePublicKey@o0.ingest.sentry.io/0", + Environment: "test", + TracesSampleRate: 0.01, + })) + + assert.InDelta(t, 0.01, telemetry.RegisteredSamplingRate(), 1e-9, + "--sentry-traces-sample-rate must reach the OTEL sampler") +} + +//nolint:paralleltest // mutates global initialized and telemetry registry state +func TestInit_RegistersResourceAttributes(t *testing.T) { + // Regression test: spans are exported straight to Sentry's OTLP endpoint + // and never pass through the Sentry client, so without these resource + // attributes --sentry-environment no longer segregated Traces even though + // it still segregated Issues. + resetSentryForTest(t) + + require.NoError(t, Init(Config{ + DSN: "https://examplePublicKey@o0.ingest.sentry.io/0", + Environment: "staging", + TracesSampleRate: 1.0, + })) + + attrs := telemetry.RegisteredResourceAttributes() + require.NotNil(t, attrs) + assert.Equal(t, "staging", attrs[environmentKey], + "exported spans need the environment as a resource attribute to be grouped in Sentry") + assert.Equal(t, "toolhive@"+versions.GetVersionInfo().Version, attrs[releaseKey], + "Traces must report the same release string as Issues") +} + +//nolint:paralleltest // mutates global initialized and telemetry registry state +func TestResourceAttributes(t *testing.T) { + tests := []struct { + name string + environment string + release string + instanceID string + want map[string]string + }{ + { + name: "omits empty values so blank attributes are not exported", + want: map[string]string{}, + }, + { + name: "carries environment, release and instance ID", + environment: "production", + release: "toolhive@1.2.3", + instanceID: "abc123", + want: map[string]string{ + environmentKey: "production", + releaseKey: "toolhive@1.2.3", + instanceIDKey: "abc123", + }, + }, + { + name: "omits the instance ID alone when it is unavailable", + environment: "production", + release: "toolhive@1.2.3", + want: map[string]string{environmentKey: "production", releaseKey: "toolhive@1.2.3"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, resourceAttributes(tt.environment, tt.release, tt.instanceID)) + }) + } +} + +//nolint:paralleltest // mutates global initialized and telemetry registry state func TestClose(t *testing.T) { t.Run("no-op when not initialized", func(_ *testing.T) { initialized.Store(false) @@ -74,44 +177,18 @@ func TestClose(t *testing.T) { }) t.Run("flushes when initialized", func(t *testing.T) { - initialized.Store(false) + resetSentryForTest(t) err := Init(Config{ DSN: "https://examplePublicKey@o0.ingest.sentry.io/0", Environment: "test", TracesSampleRate: 1.0, }) require.NoError(t, err) - defer initialized.Store(false) Close() }) } -//nolint:paralleltest // mutates global initialized and telemetry registry state -func TestInit_RegistersSpanProcessor(t *testing.T) { - t.Run("does not register processor when not initialized", func(_ *testing.T) { - initialized.Store(false) - telemetry.ResetSpanProcessorsForTesting() - assert.False(t, telemetry.HasRegisteredSpanProcessors()) - }) - - t.Run("registers span processor with telemetry registry on init", func(t *testing.T) { - initialized.Store(false) - telemetry.ResetSpanProcessorsForTesting() - err := Init(Config{ - DSN: "https://examplePublicKey@o0.ingest.sentry.io/0", - TracesSampleRate: 1.0, - }) - require.NoError(t, err) - defer func() { - initialized.Store(false) - telemetry.ResetSpanProcessorsForTesting() - }() - - assert.True(t, telemetry.HasRegisteredSpanProcessors()) - }) -} - //nolint:paralleltest // mutates global initialized state func TestCaptureException(t *testing.T) { t.Run("no-op when not initialized", func(_ *testing.T) { @@ -127,28 +204,41 @@ func TestCaptureException(t *testing.T) { CaptureException(req, nil) }) - t.Run("captures exception when initialized", func(t *testing.T) { + t.Run("captures exception linked to active OTEL trace", func(t *testing.T) { initialized.Store(false) - telemetry.ResetSpanProcessorsForTesting() transport := &gosentry.MockTransport{} err := gosentry.Init(gosentry.ClientOptions{ Dsn: "https://examplePublicKey@o0.ingest.sentry.io/0", Transport: transport, + Integrations: func(integrations []gosentry.Integration) []gosentry.Integration { + return append(integrations, sentryotel.NewOtelIntegration()) + }, }) require.NoError(t, err) initialized.Store(true) - defer func() { + t.Cleanup(func() { initialized.Store(false) - telemetry.ResetSpanProcessorsForTesting() - }() + }) - req := httptest.NewRequest(http.MethodGet, "/", nil) + tracerProvider := sdktrace.NewTracerProvider(sdktrace.WithSampler(sdktrace.AlwaysSample())) + t.Cleanup(func() { + require.NoError(t, tracerProvider.Shutdown(context.Background())) + }) + ctx, span := tracerProvider.Tracer("test-tracer").Start(context.Background(), "test-span") + defer span.End() + + req := httptest.NewRequest(http.MethodGet, "/", nil).WithContext(ctx) CaptureException(req, errors.New("test capture")) - // hub.CaptureException enqueues the event; Flush delivers it to the transport. + // CaptureEventWithHint enqueues the event; Flush delivers it to the transport. gosentry.Flush(flushTimeout) - assert.Equal(t, 1, len(transport.Events())) + events := transport.Events() + require.Len(t, events, 1) + traceContext := events[0].Contexts["trace"] + require.NotNil(t, traceContext) + assert.Equal(t, span.SpanContext().TraceID().String(), traceContext["trace_id"]) + assert.Equal(t, span.SpanContext().SpanID().String(), traceContext["span_id"]) }) } @@ -201,3 +291,43 @@ func TestEnabled(t *testing.T) { assert.True(t, Enabled()) initialized.Store(false) } + +// TestNoPIIDataCollection guards the replacement for the deprecated +// SendDefaultPII=false against being loosened by accident. Every assertion here +// is a PII decision, not a style preference. +func TestNoPIIDataCollection(t *testing.T) { + t.Parallel() + + dc := noPIIDataCollection() + + assert.True(t, dc.UserInfo.IsSet, "UserInfo must be set explicitly, or the SDK defaults it to true") + assert.False(t, dc.UserInfo.Value, "user info must not be auto-populated") + assert.Empty(t, dc.HTTPBodies, "request and response bodies must never be collected") + assert.Equal(t, gosentry.CollectionOff, dc.Cookies.Mode, "cookies must not be collected") + + // Headers and query params are collected but scrubbed, so each needs the + // extended deny-list the SDK only applies via the deprecated flag. + for name, behavior := range map[string]*gosentry.KeyValueCollectionBehavior{ + "request headers": dc.HTTPHeaders.Request, + "response headers": dc.HTTPHeaders.Response, + "query params": dc.QueryParams, + } { + assert.Equal(t, gosentry.CollectionDenyList, behavior.Mode, "%s: mode", name) + assert.Equal(t, piiSensitiveTerms, behavior.Terms, "%s: deny-list terms", name) + } +} + +// resetSentryForTest clears the package and registry state Init mutates, both +// before the test runs and after it finishes. +func resetSentryForTest(t *testing.T) { + t.Helper() + reset := func() { + initialized.Store(false) + telemetry.ResetSpanProcessorsForTesting() + spanProcessorMu.Lock() + defer spanProcessorMu.Unlock() + spanProcessor = nil + } + reset() + t.Cleanup(reset) +} diff --git a/pkg/telemetry/config.go b/pkg/telemetry/config.go index e36235e63e..6fa16c135a 100644 --- a/pkg/telemetry/config.go +++ b/pkg/telemetry/config.go @@ -7,6 +7,7 @@ package telemetry import ( "context" "fmt" + "maps" "net/http" "strconv" "strings" @@ -324,7 +325,7 @@ func NewProvider(ctx context.Context, config Config, extraProcessors ...sdktrace providers.WithMetricsEnabled(config.MetricsEnabled), providers.WithSamplingRate(config.GetSamplingRateFloat()), providers.WithEnablePrometheusMetricsPath(config.EnablePrometheusMetricsPath), - providers.WithCustomAttributes(config.CustomAttributes), + providers.WithCustomAttributes(mergeRegisteredResourceAttributes(config.CustomAttributes)), } // Merge globally registered processors (self-registered by integrations such @@ -342,6 +343,21 @@ func NewProvider(ctx context.Context, config Config, extraProcessors ...sdktrace return setGlobalProvidersAndReturn(telemetryProviders, config) } +// mergeRegisteredResourceAttributes overlays the caller's custom attributes on +// top of any registered via RegisterResourceAttributes, so an explicitly +// configured attribute always wins over one a self-registered integration +// supplied as a default. The caller's map is never mutated. +func mergeRegisteredResourceAttributes(configured map[string]string) map[string]string { + registered := RegisteredResourceAttributes() + if len(registered) == 0 { + return configured + } + merged := make(map[string]string, len(registered)+len(configured)) + maps.Copy(merged, registered) + maps.Copy(merged, configured) + return merged +} + // setGlobalProvidersAndReturn sets the global providers for OTEL and returns the providers func setGlobalProvidersAndReturn(telemetryProviders *providers.CompositeProvider, config Config) (*Provider, error) { tracingProvider := telemetryProviders.TracerProvider() diff --git a/pkg/telemetry/registry.go b/pkg/telemetry/registry.go index 5b3873cfcd..de2283da06 100644 --- a/pkg/telemetry/registry.go +++ b/pkg/telemetry/registry.go @@ -4,14 +4,22 @@ package telemetry import ( + "maps" "sync" sdktrace "go.opentelemetry.io/otel/sdk/trace" ) +// DefaultRegisteredSamplingRate is the rate used when a registered integration +// did not specify one, preserving the "sample everything" behaviour that +// processor-only mode had before rates were configurable. +const DefaultRegisteredSamplingRate = 1.0 + var ( - globalProcessors []sdktrace.SpanProcessor - globalProcessorsMu sync.Mutex + globalProcessors []sdktrace.SpanProcessor + globalResourceAttrs map[string]string + globalSamplingRate *float64 + globalProcessorsMu sync.Mutex ) // RegisterSpanProcessor registers an extra OTEL span processor to be included @@ -39,21 +47,89 @@ func RegisterSpanProcessor(p sdktrace.SpanProcessor) { globalProcessors = append(globalProcessors, p) } +// RegisterResourceAttributes merges attributes into the OTEL resource of any +// provider created via NewProvider. Integrations whose exporter bypasses their +// own SDK — such as the Sentry OTLP exporter, which never passes spans through +// the Sentry client — use this to attach the grouping keys their backend needs. +// +// As with RegisterSpanProcessor, registration must happen before NewProvider is +// called. Repeated registrations of the same key overwrite earlier values. +// +// Note that resource attributes apply to the whole provider, so they are also +// exported to any configured OTLP collector, not just to the integration that +// registered them. +func RegisterResourceAttributes(attrs map[string]string) { + if len(attrs) == 0 { + return + } + globalProcessorsMu.Lock() + defer globalProcessorsMu.Unlock() + if globalResourceAttrs == nil { + globalResourceAttrs = make(map[string]string, len(attrs)) + } + maps.Copy(globalResourceAttrs, attrs) +} + +// RegisterSamplingRate records the trace sampling rate an integration wants +// applied to the spans it receives. In processor-only mode (no OTLP endpoint) +// NewServeProvider hands this to the SDK sampler, so unsampled spans are never +// constructed at all. +// +// The SDK sampler is shared by the whole provider, so this rate is not +// per-processor: when an OTLP endpoint is also configured its own sampling rate +// wins and this value is ignored. Repeated registrations overwrite the previous +// value; only one integration is expected to register a rate. +func RegisterSamplingRate(rate float64) { + globalProcessorsMu.Lock() + defer globalProcessorsMu.Unlock() + globalSamplingRate = &rate +} + +// RegisteredSamplingRate returns the rate registered via RegisterSamplingRate, +// or DefaultRegisteredSamplingRate when no integration registered one. +func RegisteredSamplingRate() float64 { + globalProcessorsMu.Lock() + defer globalProcessorsMu.Unlock() + if globalSamplingRate == nil { + return DefaultRegisteredSamplingRate + } + return *globalSamplingRate +} + // HasRegisteredSpanProcessors returns true if any extra span processors have // been registered. Callers can use this to decide whether to initialise an // OTEL provider even when no OTLP endpoint is configured. func HasRegisteredSpanProcessors() bool { + return RegisteredSpanProcessorCount() > 0 +} + +// RegisteredSpanProcessorCount returns how many extra span processors are +// currently registered. +func RegisteredSpanProcessorCount() int { globalProcessorsMu.Lock() defer globalProcessorsMu.Unlock() - return len(globalProcessors) > 0 + return len(globalProcessors) +} + +// RegisteredResourceAttributes returns a copy of every resource attribute +// registered via RegisterResourceAttributes, or nil when there are none. +func RegisteredResourceAttributes() map[string]string { + globalProcessorsMu.Lock() + defer globalProcessorsMu.Unlock() + if len(globalResourceAttrs) == 0 { + return nil + } + return maps.Clone(globalResourceAttrs) } -// ResetSpanProcessorsForTesting clears all registered span processors. -// For use in tests only. +// ResetSpanProcessorsForTesting clears all registered span processors, resource +// attributes and the sampling rate. For use in tests only. func ResetSpanProcessorsForTesting() { globalProcessorsMu.Lock() defer globalProcessorsMu.Unlock() globalProcessors = nil + globalResourceAttrs = nil + globalSamplingRate = nil } // registeredSpanProcessors returns a snapshot of all registered processors. diff --git a/pkg/telemetry/registry_test.go b/pkg/telemetry/registry_test.go index 7f3c0fd59f..e59cd28df8 100644 --- a/pkg/telemetry/registry_test.go +++ b/pkg/telemetry/registry_test.go @@ -10,6 +10,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "go.opentelemetry.io/otel/attribute" sdktrace "go.opentelemetry.io/otel/sdk/trace" "go.opentelemetry.io/otel/sdk/trace/tracetest" ) @@ -97,3 +98,237 @@ func TestNewProvider_PicksUpRegisteredProcessor(t *testing.T) { require.Len(t, spans, 1, "the registered processor should have received OnEnd for the test span") assert.Equal(t, "test-span", spans[0].Name()) } + +// TestNewProvider_PicksUpRegisteredResourceAttributes is an end-to-end test +// that verifies attributes registered via RegisterResourceAttributes actually +// reach the OTEL resource attached to exported spans. Integrations that export +// out of band (e.g. Sentry's OTLP exporter) depend on this for grouping. +// +//nolint:paralleltest // mutates global registry state +func TestNewProvider_PicksUpRegisteredResourceAttributes(t *testing.T) { + ResetSpanProcessorsForTesting() + t.Cleanup(ResetSpanProcessorsForTesting) + + recorder := tracetest.NewSpanRecorder() + RegisterSpanProcessor(recorder) + RegisterResourceAttributes(map[string]string{"sentry.environment": "staging"}) + + ctx := context.Background() + provider, err := NewProvider(ctx, Config{ + ServiceName: "test-svc", + ServiceVersion: "0.0.1", + TracingEnabled: true, + SamplingRate: "1.0", + // No OTLP endpoint — processor-only mode, as in Sentry-only serve. + }) + require.NoError(t, err) + t.Cleanup(func() { _ = provider.Shutdown(context.Background()) }) + + _, span := provider.TracerProvider().Tracer("test-tracer").Start(ctx, "test-span") + span.End() + + spans := recorder.Ended() + require.Len(t, spans, 1) + + attrs := spans[0].Resource().Attributes() + assert.Contains(t, attrs, attribute.String("sentry.environment", "staging"), + "registered resource attributes must be present on the exported span's resource") +} + +// TestRegisterResourceAttributes verifies merge and copy semantics of the +// resource attribute registry. +// +//nolint:paralleltest // mutates global registry state +func TestRegisterResourceAttributes(t *testing.T) { + t.Run("returns nil when nothing is registered", func(t *testing.T) { + ResetSpanProcessorsForTesting() + t.Cleanup(ResetSpanProcessorsForTesting) + + RegisterResourceAttributes(nil) + RegisterResourceAttributes(map[string]string{}) + assert.Nil(t, RegisteredResourceAttributes()) + }) + + t.Run("merges successive registrations and overwrites repeated keys", func(t *testing.T) { + ResetSpanProcessorsForTesting() + t.Cleanup(ResetSpanProcessorsForTesting) + + RegisterResourceAttributes(map[string]string{"a": "1", "b": "2"}) + RegisterResourceAttributes(map[string]string{"b": "overwritten", "c": "3"}) + + assert.Equal(t, map[string]string{"a": "1", "b": "overwritten", "c": "3"}, + RegisteredResourceAttributes()) + }) + + t.Run("does not expose the registry to mutation by callers", func(t *testing.T) { + ResetSpanProcessorsForTesting() + t.Cleanup(ResetSpanProcessorsForTesting) + + input := map[string]string{"a": "1"} + RegisterResourceAttributes(input) + input["a"] = "mutated by caller" + + returned := RegisteredResourceAttributes() + returned["a"] = "mutated by reader" + + assert.Equal(t, map[string]string{"a": "1"}, RegisteredResourceAttributes()) + }) +} + +// TestMergeRegisteredResourceAttributes verifies that explicitly configured +// attributes win over those a self-registered integration supplied. +// +//nolint:paralleltest // mutates global registry state +func TestMergeRegisteredResourceAttributes(t *testing.T) { + tests := []struct { + name string + registered map[string]string + configured map[string]string + want map[string]string + }{ + { + name: "passes configured attributes through when none are registered", + configured: map[string]string{"a": "1"}, + want: map[string]string{"a": "1"}, + }, + { + name: "surfaces registered attributes when none are configured", + registered: map[string]string{"sentry.environment": "staging"}, + want: map[string]string{"sentry.environment": "staging"}, + }, + { + name: "explicit configuration wins over registered defaults", + registered: map[string]string{"sentry.environment": "staging", "a": "1"}, + configured: map[string]string{"sentry.environment": "operator-override"}, + want: map[string]string{"sentry.environment": "operator-override", "a": "1"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ResetSpanProcessorsForTesting() + t.Cleanup(ResetSpanProcessorsForTesting) + RegisterResourceAttributes(tt.registered) + + assert.Equal(t, tt.want, mergeRegisteredResourceAttributes(tt.configured)) + }) + } +} + +// TestRegisterSamplingRate_Overwrites covers the one behaviour +// TestApplyProcessorOnlySampling does not: a repeated registration replaces the +// previous rate. The unset default and an explicit zero are asserted there. +// +//nolint:paralleltest // mutates global registry state +func TestRegisterSamplingRate_Overwrites(t *testing.T) { + ResetSpanProcessorsForTesting() + t.Cleanup(ResetSpanProcessorsForTesting) + + RegisterSamplingRate(0.5) + RegisterSamplingRate(0.01) + assert.InDelta(t, 0.01, RegisteredSamplingRate(), 1e-9) +} + +// TestApplyProcessorOnlySampling verifies that a rate registered by an +// integration replaces the hardcoded 100% sampling that processor-only mode +// used to apply unconditionally. +// +//nolint:paralleltest // mutates global registry state +func TestApplyProcessorOnlySampling(t *testing.T) { + tests := []struct { + name string + register bool + registerRate float64 + wantRate float64 + }{ + { + name: "samples everything when the integration registered no rate", + wantRate: 1.0, + }, + { + name: "honours a low registered rate", + register: true, + registerRate: 0.01, + wantRate: 0.01, + }, + { + name: "honours a registered zero rate", + register: true, + wantRate: 0, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ResetSpanProcessorsForTesting() + t.Cleanup(ResetSpanProcessorsForTesting) + if tt.register { + RegisterSamplingRate(tt.registerRate) + } + + // Start from the 5% default NewServeProvider applies beforehand, to + // prove the registered rate overrides it. + cfg := Config{SamplingRate: "0.05"} + applyProcessorOnlySampling(&cfg) + + assert.True(t, cfg.TracingEnabled, + "registered processors are the only consumers, so tracing must be forced on") + // Assert on the parsed value handed to the sampler, not its string form. + assert.InDelta(t, tt.wantRate, cfg.GetSamplingRateFloat(), 1e-9) + }) + } +} + +// TestIgnoredRegisteredSamplingRate verifies which cases warrant warning the +// operator that a registered rate cannot be honoured. +// +//nolint:paralleltest // mutates global registry state +func TestIgnoredRegisteredSamplingRate(t *testing.T) { + tests := []struct { + name string + hasRegisteredProcessors bool + register bool + registerRate float64 + wantIgnored bool + }{ + { + name: "silent when no integration is registered at all", + wantIgnored: false, + }, + { + name: "silent when the integration wants every trace", + hasRegisteredProcessors: true, + wantIgnored: false, + }, + { + name: "warns when the integration asked for a lower rate", + hasRegisteredProcessors: true, + register: true, + registerRate: 0.01, + wantIgnored: true, + }, + { + name: "warns when the integration asked for no traces", + hasRegisteredProcessors: true, + register: true, + registerRate: 0, + wantIgnored: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ResetSpanProcessorsForTesting() + t.Cleanup(ResetSpanProcessorsForTesting) + if tt.register { + RegisterSamplingRate(tt.registerRate) + } + + rate, ignored := ignoredRegisteredSamplingRate(tt.hasRegisteredProcessors) + assert.Equal(t, tt.wantIgnored, ignored) + if tt.wantIgnored { + assert.InDelta(t, tt.registerRate, rate, 1e-9) + } + }) + } +} diff --git a/pkg/telemetry/serve.go b/pkg/telemetry/serve.go index ace3849658..0725fbb809 100644 --- a/pkg/telemetry/serve.go +++ b/pkg/telemetry/serve.go @@ -52,17 +52,15 @@ func NewServeProvider(ctx context.Context) (provider *Provider, otelEnabled bool telemetryCfg.SamplingRate = "0.05" } - // No OTLP endpoint but registered processors are active (e.g. a Sentry bridge). - // Force tracing on with 100% OTEL sampling so every span reaches the processors. - // Each processor applies its own sampling configuration independently. - // Note: at high RPS with 100% OTEL sampling, the OTEL SDK still constructs - // every span even if the processor's own rate drops most of them. This is an - // acceptable trade-off for Sentry-only mode where an external collector is - // not running. Configure thv config otel set-endpoint to use a real sampler - // when throughput is a concern. if otelCfg.Endpoint == "" && hasRegisteredProcessors { - telemetryCfg.TracingEnabled = true - telemetryCfg.SamplingRate = "1.0" + applyProcessorOnlySampling(&telemetryCfg) + } else if rate, ignored := ignoredRegisteredSamplingRate(hasRegisteredProcessors); ignored { + slog.Warn("integration sampling rate is ignored because an OTLP endpoint is configured; "+ + "the endpoint's sampling rate applies to every backend, so the integration receives "+ + "more traces than it requested", + "ignored_sampling_rate", rate, + "effective_sampling_rate", telemetryCfg.GetSamplingRateFloat(), + "endpoint", otelCfg.Endpoint) } p, err := NewProvider(ctx, telemetryCfg) @@ -78,6 +76,36 @@ func NewServeProvider(ctx context.Context) (provider *Provider, otelEnabled bool return p, true, nil } +// applyProcessorOnlySampling configures tracing for the case where no OTLP +// endpoint is set but registered processors are active (e.g. a Sentry bridge). +// Tracing has to be forced on because the registered processors are the only +// consumers, and the SDK sampler is given their requested rate directly so that +// unsampled spans are never constructed. A processor that registered no rate +// gets DefaultRegisteredSamplingRate. +func applyProcessorOnlySampling(cfg *Config) { + cfg.TracingEnabled = true + cfg.SetSamplingRateFromFloat(RegisteredSamplingRate()) +} + +// ignoredRegisteredSamplingRate returns the rate a registered integration asked +// for, and whether that rate is being ignored. +// +// It is ignored as soon as an OTLP endpoint is configured: the SDK sampler is +// shared by the whole provider, so the endpoint's rate applies to every backend +// and the integration receives everything that sampler passes instead of its own +// share of it. An integration that wants everything (the default rate) is never +// short-changed, so only a rate below the default is reported. +// +// This is a warning rather than an error because exporting more traces than +// requested is not worth refusing to start the server over. +func ignoredRegisteredSamplingRate(hasRegisteredProcessors bool) (float64, bool) { + if !hasRegisteredProcessors { + return 0, false + } + rate := RegisteredSamplingRate() + return rate, rate < DefaultRegisteredSamplingRate +} + // handleUnusedEndpoint enables tracing by default when an OTLP endpoint is // configured but both tracing and metrics are disabled, so the server can start // normally instead of crashing with a fatal validation error.