Add OTEL distributed tracing and optional Sentry integration to the API server

[samuv]

Summary

Adds optional distributed tracing and error reporting to `thv serve` using an OTEL-first approach with Sentry as an optional backend.

• `otelhttp` middleware extracts W3C `traceparent` headers and creates per-request spans. A post-routing `chiRouteSpanNamer` middleware renames spans after chi resolves the route, producing parameterised names like `GET /api/v1beta/workloads/{name}` instead of raw URL paths
• `thv config otel set-endpoint` configures the OTLP backend — no new CLI flags for tracing; `thv serve` reuses the same OTEL config as `thv run`
• Span processor registry (`telemetry.RegisterSpanProcessor`) allows integrations to self-register during their own `Init` — `pkg/telemetry/NewServeProvider` has no provider-specific knowledge
• Sentry bridge (`sentry-go/otel`): when a DSN is provided, the Sentry span processor self-registers so spans export to both the OTLP backend and Sentry simultaneously
• Sentry-only mode: when no OTLP endpoint is configured but a DSN is provided, a minimal OTEL SDK is created — no external collector required
• Panic and 5xx error recording: recovery middleware and error handler call both `span.RecordError` + `span.SetStatus(codes.Error)` (vendor-agnostic OTEL) and `sentrypkg.RecoverPanic` / `sentrypkg.CaptureException` (explicit Sentry Issues). The Sentry span processor only creates transactions; explicit hub calls are required for errors to appear in the Sentry Issues tab
• `custom.instance_id` tag: every Sentry event is tagged with the anonymous instance ID for correlation with ToolHive Studio
• ToolHive Studio compatibility: enable `propagateTraceparent: true` in Studio's Sentry JS SDK to send W3C `traceparent` headers, giving end-to-end distributed traces across frontend and backend

When no DSN and no OTEL endpoint are configured, all operations are no-ops with zero overhead.

Type of change

• New feature

Does this introduce a user-facing change?

Yes. `thv serve` now accepts optional `--sentry-dsn`, `--sentry-environment` and `--sentry-traces-sample-rate` flags (also readable from `SENTRY_DSN` / `SENTRY_ENVIRONMENT` env vars). Distributed tracing is configured via the existing `thv config otel set-endpoint` command.

Special notes for reviewers

The `telemetry.RegisterSpanProcessor` registry makes the OTEL provider setup fully provider-agnostic. Sentry self-registers during `sentrypkg.Init` — adding a future provider (e.g. Datadog) requires no changes to `server.go`.

Shutdown ordering is deterministic via LIFO defer: the OTEL provider (which flushes the Sentry span processor) shuts down before `sentrypkg.Close()`.

Large PR Justification

This PR introduces a new cross-cutting observability capability that spans multiple layers of the stack and cannot be meaningfully split without leaving the codebase in a broken or misleading intermediate state:

• Dependency chain is atomic — the OTEL provider layer (pkg/telemetry), the span processor registry, the Sentry bridge (pkg/sentry), and the API server wiring (pkg/api, cmd/thv/app) must all land together. Merging any subset would either leave dead code, broken imports, or a non-functional tracing pipeline
• Middleware ordering is a correctness constraint — otelhttp must be outermost, chiRouteSpanNamer must be inner, and recovery must be innermost. Any partial merge that changes this order would silently break panic capture on active spans
• Review feedback addressed in-place — roughly half the diff is direct responses to review comments (security fixes, design improvements, test additions). These are inseparable from the original feature commits since they correct the same code paths

[codecov]

Codecov Report

❌ Patch coverage is 65.83851% with 55 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.65%. Comparing base (c63b9eb) to head (d8b3bcc).
⚠️ Report is 10 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/telemetry/serve.go	0.00%	30 Missing ⚠️
pkg/api/server.go	4.76%	20 Missing ⚠️
pkg/sentry/sentry.go	89.79%	4 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4365      +/-   ##
==========================================
+ Coverage   69.60%   69.65%   +0.04%     
==========================================
  Files         490      497       +7     
  Lines       50269    50591     +322     
==========================================
+ Hits        34988    35237     +249     
- Misses      12595    12660      +65     
- Partials     2686     2694       +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[ChrisJBurns]

[Nit] The tag name custom.user_id is misleading — this is an anonymous instance ID, not a user ID. Anyone with Sentry project access could misinterpret it as PII. Combined with SendDefaultPII: true, this tag alongside IP addresses and request headers could de-anonymize the installation.

Also, Sentry's built-in PII detection heuristics may flag fields named user_id.

Suggestion: Rename to custom.instance_id.

[samuv]

Fixed, renamed to custom.instance_id. Added a code comment noting that ToolHive Studio currently uses custom.user_id for the same value and should be aligned in a follow-up.

It’s not a problem right now, but you’re right, it should represent that consistently. The only downside is that I’d need to refactor all the dashboards. I’ll take care of that in the future; for now, it’s okay to have this drift.

[ChrisJBurns]

[Bug - High] The span name formatter is called at span-start time by otelhttp, which is before chi's routing has populated RouteContext.RoutePattern(). At this point in the middleware chain, chi has created a RouteContext (so the nil check passes), but RoutePattern() returns "" because route matching hasn't happened yet.

otelhttp v0.65.0+ has a post-handler rename that checks r.Pattern, but that's a Go 1.22+ net/http.ServeMux feature — chi does not set r.Pattern.

Result: Every span will be named with the raw URL path (e.g., GET /api/v1beta/workloads/my-server) instead of the parameterized route (GET /api/v1beta/workloads/{name}). This causes unbounded cardinality in OTEL/Sentry backends, defeating route-based grouping.

Fix: Add an inner middleware (registered after chi routing) that renames the span:

func chiRouteTagMiddleware(next http.Handler) http.Handler {
    return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
        next.ServeHTTP(w, r)
        if rctx := chi.RouteContext(r.Context()); rctx != nil && rctx.RoutePattern() != "" {
            trace.SpanFromContext(r.Context()).SetName(r.Method + " " + rctx.RoutePattern())
        }
    })
}

[samuv]

Fixed, removed WithSpanNameFormatter entirely. Added chiRouteSpanNamer, a post-routing middleware that calls next.ServeHTTP first and then renames the span once RouteContext.RoutePattern() is populated. Spans now correctly show as GET /api/v1beta/workloads/{name}.

[samuv]

Thinking more about this, I also added URL path parameters as span attributes in the same middleware. This way, the concrete MCP name (and any other route params) are still visible at the trace level without increasing cardinality:

• Span name → GET /api/v1beta/workloads/{name} (low cardinality, used for grouping)
• Span attribute → url.path_param.name = "my-server" (high cardinality, for per-trace drill-down)

This is implemented generically in chiRouteSpanNamer by iterating over rctx.URLParams.Keys/Values, so all parameterized routes automatically record their path params, no per-route code needed.

Addressed here

[ChrisJBurns]

[Security - Low] With otelhttp as the outermost middleware, it reads W3C traceparent and tracestate headers from incoming requests before authentication. An untrusted client can:

1. Inject arbitrary trace IDs to correlate their requests across your telemetry backend
2. Set sampled=1 in the traceparent to force 100% sampling of their requests, increasing telemetry costs
3. Pollute trace context with large tracestate headers

This is partially mitigated if you switch to a ParentBased sampler that ignores remote parent sampling decisions (see the comment on otlp/tracing.go).

[samuv]

Acknowledged. Moving OTEL after auth would break the span lifetime guarantee for the recovery middleware (the span must be active when a panic is caught). Partially mitigated by switching to the ParentBased sampler, see the sampler comment below. Added a code comment documenting the trade-off.

[ChrisJBurns]

[Design - Must Fix] Adding otelEnabled bool as a positional parameter before the variadic middlewares breaks the public API of this exported function. The ServerBuilder already has a fluent With* pattern (WithDebugMode, WithDocs, WithOIDCConfig, etc.).

This approach also makes the call site progressively harder to read with 7+ positional booleans/strings.

Fix: The otelEnabled config should only be added as a field on ServerBuilder via the existing WithOtelEnabled(bool) method, and the Serve convenience function signature should remain unchanged. The caller can pass the OTEL middleware through the existing middlewares variadic, or the Serve function can accept a config struct.

[samuv]

Fixed — removed otelEnabled from Serve(). The cmd layer now uses ServerBuilder directly and calls WithOtelEnabled(otelEnabled), keeping Serve()'s public signature clean.

[ChrisJBurns]

[Bug - Medium] Bare TraceIDRatioBased is a root-only sampler. When a remote parent exists with sampled=true (via the W3C traceparent header from ToolHive Studio), this sampler may still drop the span if the trace ID doesn't fall within the ratio. This breaks distributed tracing correlation.

The correct pattern is:

sdktrace.WithSampler(sdktrace.ParentBased(sdktrace.TraceIDRatioBased(config.SamplingRate)))

This is pre-existing, but now materially impactful since this PR adds otelhttp traceparent extraction. Without ParentBased, the advertised Studio end-to-end tracing won't work reliably at sampling rates below 1.0.

[samuv]

Fixed, wrapped with sdktrace.ParentBased(sdktrace.TraceIDRatioBased(...)). Incoming traceparent headers with sampled=true from ToolHive Studio are now always honoured regardless of the local sampling ratio.

[ChrisJBurns]

[Design - Low] Two concerns with the global registry:

1. No dedup guard: If sentry.Init() is called twice (e.g., config reload, tests), RegisterSpanProcessor appends the same sentryotel.NewSentrySpanProcessor() singleton pointer twice. This causes OnStart/OnEnd to fire twice per span.

2. Init ordering dependency: sentry.Init() must be called before telemetry.NewProvider() for the processor to be picked up. This ordering is correct in the current code but is implicit and fragile. Consider documenting the ordering requirement, or adding a dedup check:

func RegisterSpanProcessor(p sdktrace.SpanProcessor) {
    if p == nil { return }
    globalProcessorsMu.Lock()
    defer globalProcessorsMu.Unlock()
    for _, existing := range globalProcessors {
        if existing == p { return } // already registered
    }
    globalProcessors = append(globalProcessors, p)
}

[samuv]

Fixed,added a pointer-equality check in RegisterSpanProcessor so duplicate registrations are silently ignored. Added a doc comment documenting the ordering requirement (processors must register before NewProvider is called).

[ChrisJBurns]

[Testing] The new extraProcessors ...sdktrace.SpanProcessor parameter and the registeredSpanProcessors() merge on the lines below are never tested end-to-end. No test calls RegisterSpanProcessor(p) followed by NewProvider(ctx, config) and then verifies the processor ends up in the SDK tracer.

The strategy tests set ExtraSpanProcessors directly on the Config struct, and TestInit_RegistersSpanProcessor only checks HasRegisteredSpanProcessors() — the complete pipeline is untested.

[samuv]

Added TestNewProvider_PicksUpRegisteredProcessor in pkg/telemetry/registry_test.go, registers a tracetest.SpanRecorder, calls NewProvider, creates a span, and asserts the recorder received the OnEnd callback. Also added TestRegisterSpanProcessor_Dedup to cover the new dedup guard.

[ChrisJBurns]

[Testing - Medium] Only the guard clauses are tested here (not-initialized, nil-error). The actual hub.CaptureException(err) call — the primary error-reporting path and the whole point of the Sentry integration — is never reached in any test.

Suggestion: Add a happy-path test using sentry-go's test transport:

t.Run("captures exception when initialized", func(t *testing.T) {
    initialized.Store(false)
    transport := &sentry.TransportMock{}
    err := sentry.Init(sentry.ClientOptions{
        Dsn:       "https://key@o0.ingest.sentry.io/0",
        Transport: transport,
    })
    require.NoError(t, err)
    initialized.Store(true)
    defer initialized.Store(false)
    
    req := httptest.NewRequest(http.MethodGet, "/", nil)
    CaptureException(req, errors.New("test error"))
    
    require.Equal(t, 1, len(transport.Events()))
})

[samuv]

Added captures exception when initialized sub-test using gosentry.MockTransport, verifies a call to CaptureException results in exactly one event on the transport after Flush.

[ChrisJBurns]

[Testing - Medium] Same gap as CaptureException above — RecoverPanic happy path (initialized=true, non-nil recovered value) is never tested. The hub.RecoverWithContext() and hub.Flush() calls are completely unreached.

Also note: the hub fallback path (GetHubFromContext returns nil, clone CurrentHub()) — which is the only production path since the PR doesn't use sentryhttp middleware — is never exercised.

[samuv]

Added recovers panic and creates Sentry event sub-test using gosentry.MockTransport, verifies the hub.RecoverWithContext + hub.Flush path creates an event, including the hub fallback path (clone CurrentHub()) which is the only production path without sentryhttp

[github-actions]

Large PR Detected

This PR exceeds 1000 lines of changes and requires justification before it can be reviewed.

How to unblock this PR:

Add a section to your PR description with the following format:

## Large PR Justification

[Explain why this PR must be large, such as:]
- Generated code that cannot be split
- Large refactoring that must be atomic
- Multiple related changes that would break if separated
- Migration or data transformation

Alternative:

Consider splitting this PR into smaller, focused changes (< 1000 lines each) for easier review and reduced risk.

See our Contributing Guidelines for more details.

---

This review will be automatically dismissed once you add the justification section.