From d2bd86a61c4cffb8493da50453f329576fcaeb6c Mon Sep 17 00:00:00 2001 From: "renovate[bot]" <29139614+renovate[bot]@users.noreply.github.com> Date: Thu, 10 Sep 2026 08:14:09 +0000 Subject: [PATCH 1/8] Update module github.com/getsentry/sentry-go/otel to v0.49.0 --- go.mod | 2 +- go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/go.mod b/go.mod index 6117eb190f..c09a4d0c02 100644 --- a/go.mod +++ b/go.mod @@ -23,7 +23,7 @@ require ( github.com/coreos/go-oidc/v3 v3.21.0 github.com/evanphx/json-patch/v5 v5.9.11 github.com/getsentry/sentry-go v0.49.0 - github.com/getsentry/sentry-go/otel v0.44.1 + github.com/getsentry/sentry-go/otel v0.49.0 github.com/github/smimesign v0.2.0 github.com/go-chi/chi/v5 v5.3.2 github.com/go-git/go-billy/v5 v5.9.1 diff --git a/go.sum b/go.sum index 0ad744ca18..d43302218c 100644 --- a/go.sum +++ b/go.sum @@ -216,8 +216,8 @@ github.com/fxamacker/cbor/v2 v2.9.0 h1:NpKPmjDBgUfBms6tr6JZkTHtfFGcMKsw3eGcmD/sa github.com/fxamacker/cbor/v2 v2.9.0/go.mod h1:vM4b+DJCtHn+zz7h3FFp/hDAI9WNWCsZj23V5ytsSxQ= 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.44.1 h1:RV2zUHEvGHJmCCpMaJ52tZZAlcbMgvtasQn/g3CcKKc= -github.com/getsentry/sentry-go/otel v0.44.1/go.mod h1:CfzTxocQJ6JX4SLFvnBrGULBAARFAd1fHmbJCTQlOP4= +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/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= From 3b5ccb7530a8ae29e783d64da796d00432a5bae1 Mon Sep 17 00:00:00 2001 From: Radoslav Dimitrov Date: Thu, 10 Sep 2026 11:35:46 +0300 Subject: [PATCH 2/8] Use Sentry OTEL linking integration --- pkg/sentry/sentry.go | 16 +++++++--------- pkg/sentry/sentry_test.go | 25 ------------------------- 2 files changed, 7 insertions(+), 34 deletions(-) diff --git a/pkg/sentry/sentry.go b/pkg/sentry/sentry.go index 608774fda0..8c6c9eb84e 100644 --- a/pkg/sentry/sentry.go +++ b/pkg/sentry/sentry.go @@ -14,7 +14,6 @@ import ( "github.com/getsentry/sentry-go" sentryotel "github.com/getsentry/sentry-go/otel" - "github.com/stacklok/toolhive/pkg/telemetry" "github.com/stacklok/toolhive/pkg/updates" "github.com/stacklok/toolhive/pkg/versions" ) @@ -55,6 +54,10 @@ func Init(cfg Config) error { Debug: cfg.Debug, EnableTracing: true, AttachStacktrace: true, + SendDefaultPII: false, + Integrations: func(integrations []sentry.Integration) []sentry.Integration { + return append(integrations, sentryotel.NewOtelIntegration()) + }, }) if err != nil { return fmt.Errorf("sentry init: %w", err) @@ -75,12 +78,6 @@ func Init(cfg Config) error { 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") - return nil } @@ -106,8 +103,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 diff --git a/pkg/sentry/sentry_test.go b/pkg/sentry/sentry_test.go index de804058fe..cadef74e99 100644 --- a/pkg/sentry/sentry_test.go +++ b/pkg/sentry/sentry_test.go @@ -87,31 +87,6 @@ func TestClose(t *testing.T) { }) } -//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) { From c50f0b6b777e70dc6bc33e75432179df23592b40 Mon Sep 17 00:00:00 2001 From: Radoslav Dimitrov Date: Thu, 10 Sep 2026 11:56:29 +0300 Subject: [PATCH 3/8] Restore Sentry OTEL trace export --- cmd/thv/app/server.go | 15 +++++------ go.mod | 1 + go.sum | 2 ++ pkg/api/errors/handler.go | 3 +-- pkg/sentry/sentry.go | 20 ++++++++++++++- pkg/sentry/sentry_test.go | 52 +++++++++++++++++++++++++++++---------- 6 files changed, 68 insertions(+), 25 deletions(-) diff --git a/cmd/thv/app/server.go b/cmd/thv/app/server.go index 71760f7774..7372c819a3 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 c09a4d0c02..cb67892e51 100644 --- a/go.mod +++ b/go.mod @@ -24,6 +24,7 @@ require ( github.com/evanphx/json-patch/v5 v5.9.11 github.com/getsentry/sentry-go v0.49.0 github.com/getsentry/sentry-go/otel v0.49.0 + github.com/getsentry/sentry-go/otel/otlp v0.49.0 github.com/github/smimesign v0.2.0 github.com/go-chi/chi/v5 v5.3.2 github.com/go-git/go-billy/v5 v5.9.1 diff --git a/go.sum b/go.sum index d43302218c..6b4257930d 100644 --- a/go.sum +++ b/go.sum @@ -218,6 +218,8 @@ github.com/getsentry/sentry-go v0.49.0 h1:Ehejknu1l023Ub7QoRBVLAI7g3Jnhqku4oWx4B 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 8c6c9eb84e..c8dcc424ac 100644 --- a/pkg/sentry/sentry.go +++ b/pkg/sentry/sentry.go @@ -5,6 +5,7 @@ package sentry import ( + "context" "fmt" "log/slog" "net/http" @@ -13,7 +14,10 @@ import ( "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" "github.com/stacklok/toolhive/pkg/versions" ) @@ -63,8 +67,14 @@ func Init(cfg Config) error { return fmt.Errorf("sentry init: %w", err) } + exporter, err := sentryotlp.NewTraceExporter(context.Background(), cfg.DSN) + if err != nil { + return fmt.Errorf("create Sentry trace exporter: %w", err) + } + telemetry.RegisterSpanProcessor(sdktrace.NewBatchSpanProcessor(exporter)) initialized.Store(true) slog.Debug("sentry initialized", "environment", cfg.Environment) + slog.Debug("sentry trace exporter registered with OTEL registry") // Tag every event and transaction with the anonymous instance ID so that // Sentry events from the API server can be correlated with those from @@ -114,7 +124,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 cadef74e99..0bee03dfa1 100644 --- a/pkg/sentry/sentry_test.go +++ b/pkg/sentry/sentry_test.go @@ -4,14 +4,17 @@ 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" ) @@ -19,7 +22,7 @@ import ( // 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 +36,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", @@ -53,7 +56,11 @@ func TestInit(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { initialized.Store(false) - defer initialized.Store(false) + telemetry.ResetSpanProcessorsForTesting() + t.Cleanup(func() { + initialized.Store(false) + telemetry.ResetSpanProcessorsForTesting() + }) err := Init(tt.cfg) if tt.wantErr { @@ -62,11 +69,13 @@ 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 TestClose(t *testing.T) { t.Run("no-op when not initialized", func(_ *testing.T) { initialized.Store(false) @@ -75,13 +84,17 @@ func TestClose(t *testing.T) { t.Run("flushes when initialized", func(t *testing.T) { initialized.Store(false) + telemetry.ResetSpanProcessorsForTesting() + t.Cleanup(func() { + initialized.Store(false) + telemetry.ResetSpanProcessorsForTesting() + }) 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() }) @@ -102,28 +115,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"]) }) } From c982a8af7039d83413f0f1bf282e7d1ae51eafc2 Mon Sep 17 00:00:00 2001 From: Radoslav Dimitrov Date: Thu, 10 Sep 2026 12:39:11 +0300 Subject: [PATCH 4/8] Restore Sentry behavior lost in otel migration Moving from sentryotel.NewSentrySpanProcessor (removed upstream in v0.47.0) to sentryotlp.NewTraceExporter turned Sentry from an in-process span processor into a plain OTLP exporter. The old processor funnelled every span through sentry.StartTransaction, so it inherited the client's sampler, scope tags, environment and release for free. The exporter never touches the client, so all of that was silently dropped. Apply the Sentry sample rate in the processor itself. A bare BatchSpanProcessor does no sampling, and NewServeProvider forces OTEL sampling to 1.0 in Sentry-only mode, so --sentry-traces-sample-rate had become a no-op and every span was shipped to Sentry regardless of the configured rate. Sampling is derived from the trace ID via ParentBased so whole traces are kept or dropped together and a trace already sampled upstream is preserved. This restores the per-processor sampling invariant NewServeProvider had already documented. Carry environment, release and instance ID as OTEL resource attributes. Without them Issues and Traces disagreed on all three, and --sentry-environment no longer segregated traces in the Sentry UI. They are registered through the same self-registration seam as the span processor, so the OTEL provider setup stays free of Sentry specifics. Create the span processor once per process. The registry deduplicates by pointer identity and NewBatchSpanProcessor allocates a fresh pointer on every call, so a second Init registered a duplicate processor, double-exported every span and leaked the first exporter's goroutine. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/sentry/processor.go | 79 ++++++++++++++++++++ pkg/sentry/processor_test.go | 132 +++++++++++++++++++++++++++++++++ pkg/sentry/sentry.go | 96 ++++++++++++++++++++++-- pkg/sentry/sentry_test.go | 110 ++++++++++++++++++++++++--- pkg/telemetry/config.go | 18 ++++- pkg/telemetry/registry.go | 53 +++++++++++-- pkg/telemetry/registry_test.go | 117 +++++++++++++++++++++++++++++ pkg/telemetry/serve.go | 6 +- 8 files changed, 585 insertions(+), 26 deletions(-) create mode 100644 pkg/sentry/processor.go create mode 100644 pkg/sentry/processor_test.go diff --git a/pkg/sentry/processor.go b/pkg/sentry/processor.go new file mode 100644 index 0000000000..1bc2685ec8 --- /dev/null +++ b/pkg/sentry/processor.go @@ -0,0 +1,79 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package sentry + +import ( + "context" + + sdktrace "go.opentelemetry.io/otel/sdk/trace" + "go.opentelemetry.io/otel/trace" +) + +// samplingSpanProcessor applies Sentry's own trace sample rate before handing +// spans to the wrapped processor. +// +// The OTEL SDK installs a single global sampler shared by every configured +// backend, so it cannot express "export every span to the OTLP collector but +// only 1% to Sentry". telemetry.NewServeProvider therefore runs the SDK sampler +// at 1.0 whenever a registered processor is active and relies on each processor +// to enforce its own rate here. Without this, --sentry-traces-sample-rate would +// be silently ignored and every span would be shipped to Sentry. +type samplingSpanProcessor struct { + sdktrace.SpanProcessor + sampler sdktrace.Sampler +} + +// newSamplingSpanProcessor wraps next so that only spans within rate reach it. +// A rate of 1.0 or above needs no filtering at all, so next is returned +// unwrapped; a rate of 0 or below disables Sentry trace export entirely. +func newSamplingSpanProcessor(next sdktrace.SpanProcessor, rate float64) sdktrace.SpanProcessor { + switch { + case rate >= 1.0: + return next + case rate <= 0: + return &samplingSpanProcessor{SpanProcessor: next, sampler: sdktrace.NeverSample()} + default: + // ParentBased mirrors the SDK sampler in toolhive-core: a trace already + // sampled by an upstream service (e.g. ToolHive Studio) is kept even + // when the local ratio would drop it, so distributed traces are not + // truncated half way through. + return &samplingSpanProcessor{ + SpanProcessor: next, + sampler: sdktrace.ParentBased(sdktrace.TraceIDRatioBased(rate)), + } + } +} + +func (p *samplingSpanProcessor) OnStart(parent context.Context, s sdktrace.ReadWriteSpan) { + if !p.sample(s.SpanContext().TraceID(), s.Parent(), s.Name(), s.SpanKind()) { + return + } + p.SpanProcessor.OnStart(parent, s) +} + +func (p *samplingSpanProcessor) OnEnd(s sdktrace.ReadOnlySpan) { + if !p.sample(s.SpanContext().TraceID(), s.Parent(), s.Name(), s.SpanKind()) { + return + } + p.SpanProcessor.OnEnd(s) +} + +// sample derives the decision from the trace ID alone, which keeps it +// deterministic: OnStart and OnEnd always agree, and every span belonging to a +// trace is kept or dropped together rather than leaving partial traces in +// Sentry. +func (p *samplingSpanProcessor) sample( + traceID trace.TraceID, + parent trace.SpanContext, + name string, + kind trace.SpanKind, +) bool { + result := p.sampler.ShouldSample(sdktrace.SamplingParameters{ + ParentContext: trace.ContextWithSpanContext(context.Background(), parent), + TraceID: traceID, + Name: name, + Kind: kind, + }) + return result.Decision == sdktrace.RecordAndSample +} diff --git a/pkg/sentry/processor_test.go b/pkg/sentry/processor_test.go new file mode 100644 index 0000000000..896084ebc8 --- /dev/null +++ b/pkg/sentry/processor_test.go @@ -0,0 +1,132 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package sentry + +import ( + "context" + "sync" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + sdktrace "go.opentelemetry.io/otel/sdk/trace" + "go.opentelemetry.io/otel/trace" +) + +// countingProcessor records how many spans made it past the sampling wrapper. +type countingProcessor struct { + mu sync.Mutex + starts int + ends int +} + +func (p *countingProcessor) OnStart(_ context.Context, _ sdktrace.ReadWriteSpan) { + p.mu.Lock() + defer p.mu.Unlock() + p.starts++ +} + +func (p *countingProcessor) OnEnd(_ sdktrace.ReadOnlySpan) { + p.mu.Lock() + defer p.mu.Unlock() + p.ends++ +} + +func (*countingProcessor) Shutdown(context.Context) error { return nil } +func (*countingProcessor) ForceFlush(context.Context) error { return nil } + +func (p *countingProcessor) counts() (int, int) { + p.mu.Lock() + defer p.mu.Unlock() + return p.starts, p.ends +} + +// recordSpans drives count root spans through a provider wired with proc. +func recordSpans(t *testing.T, proc sdktrace.SpanProcessor, count int) { + t.Helper() + provider := sdktrace.NewTracerProvider( + sdktrace.WithSampler(sdktrace.AlwaysSample()), + sdktrace.WithSpanProcessor(proc), + ) + t.Cleanup(func() { + require.NoError(t, provider.Shutdown(context.Background())) + }) + tracer := provider.Tracer("test-tracer") + for range count { + _, span := tracer.Start(context.Background(), "test-span") + span.End() + } +} + +func TestNewSamplingSpanProcessor(t *testing.T) { + t.Parallel() + + t.Run("returns the processor unwrapped at full sample rate", func(t *testing.T) { + t.Parallel() + next := &countingProcessor{} + assert.Same(t, sdktrace.SpanProcessor(next), newSamplingSpanProcessor(next, 1.0), + "a rate of 1.0 needs no filtering and should add no wrapper overhead") + }) + + t.Run("drops every span at a zero sample rate", func(t *testing.T) { + t.Parallel() + next := &countingProcessor{} + recordSpans(t, newSamplingSpanProcessor(next, 0), 50) + + starts, ends := next.counts() + assert.Zero(t, starts) + assert.Zero(t, ends) + }) + + t.Run("drops most spans at a low sample rate", func(t *testing.T) { + t.Parallel() + // Regression test for --sentry-traces-sample-rate being ignored: a bare + // BatchSpanProcessor exported all 200 spans regardless of the rate. + const total = 200 + next := &countingProcessor{} + recordSpans(t, newSamplingSpanProcessor(next, 0.05), total) + + _, ends := next.counts() + assert.Less(t, ends, total/2, + "a 5%% sample rate must drop the large majority of %d spans, got %d", total, ends) + }) + + t.Run("keeps OnStart and OnEnd paired for the same trace", func(t *testing.T) { + t.Parallel() + next := &countingProcessor{} + recordSpans(t, newSamplingSpanProcessor(next, 0.5), 200) + + starts, ends := next.counts() + assert.Equal(t, starts, ends, + "the decision must be deterministic per trace ID so no span is started without being ended") + }) + + t.Run("keeps spans whose remote parent was already sampled", func(t *testing.T) { + t.Parallel() + // A trace sampled upstream (e.g. by ToolHive Studio) must survive the + // local ratio, otherwise distributed traces are truncated mid-way. + next := &countingProcessor{} + proc := newSamplingSpanProcessor(next, 0.0001) + provider := sdktrace.NewTracerProvider( + sdktrace.WithSampler(sdktrace.AlwaysSample()), + sdktrace.WithSpanProcessor(proc), + ) + t.Cleanup(func() { + require.NoError(t, provider.Shutdown(context.Background())) + }) + + remoteParent := trace.NewSpanContext(trace.SpanContextConfig{ + TraceID: trace.TraceID{0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f, 0x10}, + SpanID: trace.SpanID{0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08}, + TraceFlags: trace.FlagsSampled, + Remote: true, + }) + ctx := trace.ContextWithSpanContext(context.Background(), remoteParent) + _, span := provider.Tracer("test-tracer").Start(ctx, "child-span") + span.End() + + _, ends := next.counts() + assert.Equal(t, 1, ends) + }) +} diff --git a/pkg/sentry/sentry.go b/pkg/sentry/sentry.go index c8dcc424ac..8c096eb818 100644 --- a/pkg/sentry/sentry.go +++ b/pkg/sentry/sentry.go @@ -9,6 +9,7 @@ import ( "fmt" "log/slog" "net/http" + "sync" "sync/atomic" "time" @@ -24,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. @@ -49,11 +71,14 @@ 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, @@ -67,30 +92,87 @@ func Init(cfg Config) error { return fmt.Errorf("sentry init: %w", err) } - exporter, err := sentryotlp.NewTraceExporter(context.Background(), cfg.DSN) - if err != nil { - return fmt.Errorf("create Sentry trace exporter: %w", err) + if err := registerTraceExporter(cfg); err != nil { + return err } - telemetry.RegisterSpanProcessor(sdktrace.NewBatchSpanProcessor(exporter)) initialized.Store(true) slog.Debug("sentry initialized", "environment", cfg.Environment) - slog.Debug("sentry trace exporter registered with OTEL registry") // Tag every event and transaction with the anonymous instance ID so that // Sentry events from the API server can be correlated with those from // 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) } + // 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 and sample rate. thv +// serve calls Init exactly once per process, and the registry already only +// feeds processors to providers created after registration, so re-initialising +// with different values 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 = newSamplingSpanProcessor( + sdktrace.NewBatchSpanProcessor(exporter), + cfg.TracesSampleRate, + ) + } + + telemetry.RegisterSpanProcessor(spanProcessor) + slog.Debug("sentry trace exporter registered with OTEL registry", + "traces_sample_rate", cfg.TracesSampleRate) return nil } +// 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() { diff --git a/pkg/sentry/sentry_test.go b/pkg/sentry/sentry_test.go index 0bee03dfa1..760c4997a0 100644 --- a/pkg/sentry/sentry_test.go +++ b/pkg/sentry/sentry_test.go @@ -17,6 +17,7 @@ import ( 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 @@ -55,12 +56,7 @@ func TestInit(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - initialized.Store(false) - telemetry.ResetSpanProcessorsForTesting() - t.Cleanup(func() { - initialized.Store(false) - telemetry.ResetSpanProcessorsForTesting() - }) + resetSentryForTest(t) err := Init(tt.cfg) if tt.wantErr { @@ -75,6 +71,86 @@ func TestInit(t *testing.T) { } } +//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_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) { @@ -83,12 +159,7 @@ func TestClose(t *testing.T) { }) t.Run("flushes when initialized", func(t *testing.T) { - initialized.Store(false) - telemetry.ResetSpanProcessorsForTesting() - t.Cleanup(func() { - initialized.Store(false) - telemetry.ResetSpanProcessorsForTesting() - }) + resetSentryForTest(t) err := Init(Config{ DSN: "https://examplePublicKey@o0.ingest.sentry.io/0", Environment: "test", @@ -202,3 +273,18 @@ func TestEnabled(t *testing.T) { assert.True(t, Enabled()) initialized.Store(false) } + +// 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..78adfbc46e 100644 --- a/pkg/telemetry/registry.go +++ b/pkg/telemetry/registry.go @@ -4,14 +4,16 @@ package telemetry import ( + "maps" "sync" sdktrace "go.opentelemetry.io/otel/sdk/trace" ) var ( - globalProcessors []sdktrace.SpanProcessor - globalProcessorsMu sync.Mutex + globalProcessors []sdktrace.SpanProcessor + globalResourceAttrs map[string]string + globalProcessorsMu sync.Mutex ) // RegisterSpanProcessor registers an extra OTEL span processor to be included @@ -39,21 +41,62 @@ 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) +} + // 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) +} + +// 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() - return len(globalProcessors) > 0 + 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 and +// resource attributes. For use in tests only. func ResetSpanProcessorsForTesting() { globalProcessorsMu.Lock() defer globalProcessorsMu.Unlock() globalProcessors = nil + globalResourceAttrs = 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..b60fccfa37 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,119 @@ 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)) + }) + } +} diff --git a/pkg/telemetry/serve.go b/pkg/telemetry/serve.go index ace3849658..8dd9bb3235 100644 --- a/pkg/telemetry/serve.go +++ b/pkg/telemetry/serve.go @@ -54,7 +54,11 @@ func NewServeProvider(ctx context.Context) (provider *Provider, otelEnabled bool // 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. + // Because the SDK sampler is global and shared by every backend, it cannot + // express a per-backend rate; each registered processor is therefore + // responsible for applying its own sampling before exporting (see + // pkg/sentry.newSamplingSpanProcessor). A processor that does not will + // receive — and export — every span. // 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 From 646f92f9d8c38c2ca387025f3b401ffa6f30458b Mon Sep 17 00:00:00 2001 From: Radoslav Dimitrov Date: Thu, 10 Sep 2026 12:53:44 +0300 Subject: [PATCH 5/8] Sample Sentry traces in the SDK sampler Replace the custom sampling span processor with a sampling rate handed to the OTEL SDK sampler. Both approaches make --sentry-traces-sample-rate effective again, but the SDK sampler runs before span creation, so unsampled spans are never constructed rather than being built at 100% and discarded on export. That removes the throughput caveat processor-only mode had to document, and drops a SpanProcessor implementation in favour of a config value. The tradeoff is that the SDK sampler is shared by the whole provider, so Sentry no longer gets a rate independent of a configured OTLP collector; when an endpoint is set, its rate wins. Only processor-only mode, which is how thv serve runs Sentry, honours the registered rate. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/sentry/processor.go | 79 -------------------- pkg/sentry/processor_test.go | 132 --------------------------------- pkg/sentry/sentry.go | 17 +++-- pkg/sentry/sentry_test.go | 18 +++++ pkg/telemetry/registry.go | 37 ++++++++- pkg/telemetry/registry_test.go | 64 ++++++++++++++++ pkg/telemetry/serve.go | 26 +++---- 7 files changed, 138 insertions(+), 235 deletions(-) delete mode 100644 pkg/sentry/processor.go delete mode 100644 pkg/sentry/processor_test.go diff --git a/pkg/sentry/processor.go b/pkg/sentry/processor.go deleted file mode 100644 index 1bc2685ec8..0000000000 --- a/pkg/sentry/processor.go +++ /dev/null @@ -1,79 +0,0 @@ -// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. -// SPDX-License-Identifier: Apache-2.0 - -package sentry - -import ( - "context" - - sdktrace "go.opentelemetry.io/otel/sdk/trace" - "go.opentelemetry.io/otel/trace" -) - -// samplingSpanProcessor applies Sentry's own trace sample rate before handing -// spans to the wrapped processor. -// -// The OTEL SDK installs a single global sampler shared by every configured -// backend, so it cannot express "export every span to the OTLP collector but -// only 1% to Sentry". telemetry.NewServeProvider therefore runs the SDK sampler -// at 1.0 whenever a registered processor is active and relies on each processor -// to enforce its own rate here. Without this, --sentry-traces-sample-rate would -// be silently ignored and every span would be shipped to Sentry. -type samplingSpanProcessor struct { - sdktrace.SpanProcessor - sampler sdktrace.Sampler -} - -// newSamplingSpanProcessor wraps next so that only spans within rate reach it. -// A rate of 1.0 or above needs no filtering at all, so next is returned -// unwrapped; a rate of 0 or below disables Sentry trace export entirely. -func newSamplingSpanProcessor(next sdktrace.SpanProcessor, rate float64) sdktrace.SpanProcessor { - switch { - case rate >= 1.0: - return next - case rate <= 0: - return &samplingSpanProcessor{SpanProcessor: next, sampler: sdktrace.NeverSample()} - default: - // ParentBased mirrors the SDK sampler in toolhive-core: a trace already - // sampled by an upstream service (e.g. ToolHive Studio) is kept even - // when the local ratio would drop it, so distributed traces are not - // truncated half way through. - return &samplingSpanProcessor{ - SpanProcessor: next, - sampler: sdktrace.ParentBased(sdktrace.TraceIDRatioBased(rate)), - } - } -} - -func (p *samplingSpanProcessor) OnStart(parent context.Context, s sdktrace.ReadWriteSpan) { - if !p.sample(s.SpanContext().TraceID(), s.Parent(), s.Name(), s.SpanKind()) { - return - } - p.SpanProcessor.OnStart(parent, s) -} - -func (p *samplingSpanProcessor) OnEnd(s sdktrace.ReadOnlySpan) { - if !p.sample(s.SpanContext().TraceID(), s.Parent(), s.Name(), s.SpanKind()) { - return - } - p.SpanProcessor.OnEnd(s) -} - -// sample derives the decision from the trace ID alone, which keeps it -// deterministic: OnStart and OnEnd always agree, and every span belonging to a -// trace is kept or dropped together rather than leaving partial traces in -// Sentry. -func (p *samplingSpanProcessor) sample( - traceID trace.TraceID, - parent trace.SpanContext, - name string, - kind trace.SpanKind, -) bool { - result := p.sampler.ShouldSample(sdktrace.SamplingParameters{ - ParentContext: trace.ContextWithSpanContext(context.Background(), parent), - TraceID: traceID, - Name: name, - Kind: kind, - }) - return result.Decision == sdktrace.RecordAndSample -} diff --git a/pkg/sentry/processor_test.go b/pkg/sentry/processor_test.go deleted file mode 100644 index 896084ebc8..0000000000 --- a/pkg/sentry/processor_test.go +++ /dev/null @@ -1,132 +0,0 @@ -// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. -// SPDX-License-Identifier: Apache-2.0 - -package sentry - -import ( - "context" - "sync" - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - sdktrace "go.opentelemetry.io/otel/sdk/trace" - "go.opentelemetry.io/otel/trace" -) - -// countingProcessor records how many spans made it past the sampling wrapper. -type countingProcessor struct { - mu sync.Mutex - starts int - ends int -} - -func (p *countingProcessor) OnStart(_ context.Context, _ sdktrace.ReadWriteSpan) { - p.mu.Lock() - defer p.mu.Unlock() - p.starts++ -} - -func (p *countingProcessor) OnEnd(_ sdktrace.ReadOnlySpan) { - p.mu.Lock() - defer p.mu.Unlock() - p.ends++ -} - -func (*countingProcessor) Shutdown(context.Context) error { return nil } -func (*countingProcessor) ForceFlush(context.Context) error { return nil } - -func (p *countingProcessor) counts() (int, int) { - p.mu.Lock() - defer p.mu.Unlock() - return p.starts, p.ends -} - -// recordSpans drives count root spans through a provider wired with proc. -func recordSpans(t *testing.T, proc sdktrace.SpanProcessor, count int) { - t.Helper() - provider := sdktrace.NewTracerProvider( - sdktrace.WithSampler(sdktrace.AlwaysSample()), - sdktrace.WithSpanProcessor(proc), - ) - t.Cleanup(func() { - require.NoError(t, provider.Shutdown(context.Background())) - }) - tracer := provider.Tracer("test-tracer") - for range count { - _, span := tracer.Start(context.Background(), "test-span") - span.End() - } -} - -func TestNewSamplingSpanProcessor(t *testing.T) { - t.Parallel() - - t.Run("returns the processor unwrapped at full sample rate", func(t *testing.T) { - t.Parallel() - next := &countingProcessor{} - assert.Same(t, sdktrace.SpanProcessor(next), newSamplingSpanProcessor(next, 1.0), - "a rate of 1.0 needs no filtering and should add no wrapper overhead") - }) - - t.Run("drops every span at a zero sample rate", func(t *testing.T) { - t.Parallel() - next := &countingProcessor{} - recordSpans(t, newSamplingSpanProcessor(next, 0), 50) - - starts, ends := next.counts() - assert.Zero(t, starts) - assert.Zero(t, ends) - }) - - t.Run("drops most spans at a low sample rate", func(t *testing.T) { - t.Parallel() - // Regression test for --sentry-traces-sample-rate being ignored: a bare - // BatchSpanProcessor exported all 200 spans regardless of the rate. - const total = 200 - next := &countingProcessor{} - recordSpans(t, newSamplingSpanProcessor(next, 0.05), total) - - _, ends := next.counts() - assert.Less(t, ends, total/2, - "a 5%% sample rate must drop the large majority of %d spans, got %d", total, ends) - }) - - t.Run("keeps OnStart and OnEnd paired for the same trace", func(t *testing.T) { - t.Parallel() - next := &countingProcessor{} - recordSpans(t, newSamplingSpanProcessor(next, 0.5), 200) - - starts, ends := next.counts() - assert.Equal(t, starts, ends, - "the decision must be deterministic per trace ID so no span is started without being ended") - }) - - t.Run("keeps spans whose remote parent was already sampled", func(t *testing.T) { - t.Parallel() - // A trace sampled upstream (e.g. by ToolHive Studio) must survive the - // local ratio, otherwise distributed traces are truncated mid-way. - next := &countingProcessor{} - proc := newSamplingSpanProcessor(next, 0.0001) - provider := sdktrace.NewTracerProvider( - sdktrace.WithSampler(sdktrace.AlwaysSample()), - sdktrace.WithSpanProcessor(proc), - ) - t.Cleanup(func() { - require.NoError(t, provider.Shutdown(context.Background())) - }) - - remoteParent := trace.NewSpanContext(trace.SpanContextConfig{ - TraceID: trace.TraceID{0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f, 0x10}, - SpanID: trace.SpanID{0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08}, - TraceFlags: trace.FlagsSampled, - Remote: true, - }) - ctx := trace.ContextWithSpanContext(context.Background(), remoteParent) - _, span := provider.Tracer("test-tracer").Start(ctx, "child-span") - span.End() - - _, ends := next.counts() - assert.Equal(t, 1, ends) - }) -} diff --git a/pkg/sentry/sentry.go b/pkg/sentry/sentry.go index 8c096eb818..7ae8f88232 100644 --- a/pkg/sentry/sentry.go +++ b/pkg/sentry/sentry.go @@ -131,10 +131,10 @@ func Init(cfg Config) error { // 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 and sample rate. thv -// serve calls Init exactly once per process, and the registry already only -// feeds processors to providers created after registration, so re-initialising -// with different values is not supported either way. +// 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() @@ -144,13 +144,14 @@ func registerTraceExporter(cfg Config) error { if err != nil { return fmt.Errorf("create Sentry trace exporter: %w", err) } - spanProcessor = newSamplingSpanProcessor( - sdktrace.NewBatchSpanProcessor(exporter), - cfg.TracesSampleRate, - ) + 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 diff --git a/pkg/sentry/sentry_test.go b/pkg/sentry/sentry_test.go index 760c4997a0..d0142d9e2d 100644 --- a/pkg/sentry/sentry_test.go +++ b/pkg/sentry/sentry_test.go @@ -90,6 +90,24 @@ func TestInit_RegistersExactlyOneSpanProcessor(t *testing.T) { "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 diff --git a/pkg/telemetry/registry.go b/pkg/telemetry/registry.go index 78adfbc46e..de2283da06 100644 --- a/pkg/telemetry/registry.go +++ b/pkg/telemetry/registry.go @@ -10,9 +10,15 @@ import ( 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 globalResourceAttrs map[string]string + globalSamplingRate *float64 globalProcessorsMu sync.Mutex ) @@ -64,6 +70,32 @@ func RegisterResourceAttributes(attrs map[string]string) { 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. @@ -90,13 +122,14 @@ func RegisteredResourceAttributes() map[string]string { return maps.Clone(globalResourceAttrs) } -// ResetSpanProcessorsForTesting clears all registered span processors and -// resource attributes. 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 b60fccfa37..09218418c4 100644 --- a/pkg/telemetry/registry_test.go +++ b/pkg/telemetry/registry_test.go @@ -214,3 +214,67 @@ func TestMergeRegisteredResourceAttributes(t *testing.T) { }) } } + +// 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) + }) + } +} diff --git a/pkg/telemetry/serve.go b/pkg/telemetry/serve.go index 8dd9bb3235..c575c7508b 100644 --- a/pkg/telemetry/serve.go +++ b/pkg/telemetry/serve.go @@ -52,21 +52,8 @@ 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. - // Because the SDK sampler is global and shared by every backend, it cannot - // express a per-backend rate; each registered processor is therefore - // responsible for applying its own sampling before exporting (see - // pkg/sentry.newSamplingSpanProcessor). A processor that does not will - // receive — and export — every span. - // 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) } p, err := NewProvider(ctx, telemetryCfg) @@ -82,6 +69,17 @@ 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()) +} + // 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. From 8761d798fa636278956bc501fe81b58f25dd656e Mon Sep 17 00:00:00 2001 From: Radoslav Dimitrov Date: Thu, 10 Sep 2026 13:10:06 +0300 Subject: [PATCH 6/8] Warn when a registered sampling rate is ignored Sampling now happens in the shared SDK sampler, so once an OTLP endpoint is configured the endpoint's rate applies to every backend and an integration's own rate cannot be honoured. Sentry then receives everything that sampler passes rather than its own share of it, which is more traces than --sentry-traces-sample-rate asked for. Restoring independent per-backend rates needs a sampling span processor, which is more machinery than the case warrants. Log the conflict at startup instead so the behaviour is visible rather than silent, and keep serving: exporting more traces than requested is not worth refusing to start over. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/telemetry/registry_test.go | 54 ++++++++++++++++++++++++++++++++++ pkg/telemetry/serve.go | 26 ++++++++++++++++ 2 files changed, 80 insertions(+) diff --git a/pkg/telemetry/registry_test.go b/pkg/telemetry/registry_test.go index 09218418c4..e59cd28df8 100644 --- a/pkg/telemetry/registry_test.go +++ b/pkg/telemetry/registry_test.go @@ -278,3 +278,57 @@ func TestApplyProcessorOnlySampling(t *testing.T) { }) } } + +// 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 c575c7508b..0725fbb809 100644 --- a/pkg/telemetry/serve.go +++ b/pkg/telemetry/serve.go @@ -54,6 +54,13 @@ func NewServeProvider(ctx context.Context) (provider *Provider, otelEnabled bool if otelCfg.Endpoint == "" && hasRegisteredProcessors { 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) @@ -80,6 +87,25 @@ func applyProcessorOnlySampling(cfg *Config) { 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. From c1b1fb66ea6be43babe39ebe2be755b42cc36842 Mon Sep 17 00:00:00 2001 From: Radoslav Dimitrov Date: Thu, 10 Sep 2026 13:26:50 +0300 Subject: [PATCH 7/8] Replace deprecated SendDefaultPII with DataCollection The v0.49.0 bump deprecated ClientOptions.SendDefaultPII, failing the lint job. Migrating is not a straight swap: SendDefaultPII=false also installs an extended deny-list covering forwarding headers, remote addresses and user identifiers, and sentry-go keeps that list in an unexported field the DataCollection API cannot reach. Setting DataCollection naively would have quietly stopped scrubbing client IPs from an API server that usually sits behind a proxy. Reproduce the list as per-behaviour Terms instead. In CollectionDenyList mode the SDK ORs a behaviour's Terms with its built-in terms, so filtering is unchanged; verified by comparing the resolved DataCollection of both configurations field by field. A test pins the result so the PII posture cannot be loosened without a failing assertion. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/sentry/sentry.go | 46 ++++++++++++++++++++++++++++++++++++++- pkg/sentry/sentry_test.go | 25 +++++++++++++++++++++ 2 files changed, 70 insertions(+), 1 deletion(-) diff --git a/pkg/sentry/sentry.go b/pkg/sentry/sentry.go index 7ae8f88232..59057d59fa 100644 --- a/pkg/sentry/sentry.go +++ b/pkg/sentry/sentry.go @@ -83,7 +83,7 @@ func Init(cfg Config) error { Debug: cfg.Debug, EnableTracing: true, AttachStacktrace: true, - SendDefaultPII: false, + DataCollection: noPIIDataCollection(), Integrations: func(integrations []sentry.Integration) []sentry.Integration { return append(integrations, sentryotel.NewOtelIntegration()) }, @@ -157,6 +157,50 @@ func registerTraceExporter(cfg Config) error { 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. diff --git a/pkg/sentry/sentry_test.go b/pkg/sentry/sentry_test.go index d0142d9e2d..48a43f0b72 100644 --- a/pkg/sentry/sentry_test.go +++ b/pkg/sentry/sentry_test.go @@ -292,6 +292,31 @@ func TestEnabled(t *testing.T) { 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) { From 235e2fc419a5379635d62faecaab5cc2072ed7f2 Mon Sep 17 00:00:00 2001 From: Radoslav Dimitrov Date: Mon, 14 Sep 2026 21:59:38 +0300 Subject: [PATCH 8/8] Flush Sentry traces before closing client --- cmd/thv/app/server.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/cmd/thv/app/server.go b/cmd/thv/app/server.go index 7372c819a3..e0c7fd34fb 100644 --- a/cmd/thv/app/server.go +++ b/cmd/thv/app/server.go @@ -69,6 +69,7 @@ var serveCmd = &cobra.Command{ if err := sentrypkg.Init(sentryCfg); err != nil { return fmt.Errorf("failed to initialize sentry: %w", err) } + defer sentrypkg.Close() // Initialize OTEL provider from global config (thv config otel set-endpoint). // When Sentry is initialized, its trace exporter is added as a span processor, @@ -91,8 +92,6 @@ var serveCmd = &cobra.Command{ } }() } - defer sentrypkg.Close() - // If socket path is provided, use it; otherwise use host:port address := fmt.Sprintf("%s:%d", host, port) isUnixSocket := false