feat(tracing): OTel improvements from go-fan review - #8841
Conversation
- Add span.IsRecording() guards around AddEvent calls and attribute slice construction in unified.go and proxy/handler.go to eliminate allocations in the noop (common) case. - Add RecordSpanErrorSafe helper that records a scrubbed public error on the span instead of the internal error, preventing internal details from leaking to trace backends. Migrate the ad-hoc fmt.Errorf pattern in unified.go to use the new helper. - Centralise all semconv/v1.41.0 imports into internal/tracing/semconv.go. The new file is the single location for semconv re-exports (attribute keys, helper functions, SchemaURL). All other tracing package files and the proxy handler now reference these re-exports, so a future semconv version bump only requires editing one file. - Tune BatchSpanProcessor options explicitly: MaxExportBatchSize=256 and BatchTimeout=2s for better gateway throughput characteristics. - Improve fanoutExporter.Shutdown context propagation: each exporter goroutine now gets a fresh per-exporter context derived from the parent's remaining deadline, preventing a slow exporter from starving others of their time budget. Closes #8834
…o clarity
- Fix TestRecordSpanErrorSafe_RecordsPublicMsgNotInternalError: use a
distinctive sentinel string ("secret-token-12345") in the internal
error that is also checked in the NotContains assertion, so the test
unambiguously verifies the internal error detail is not leaked.
- Refactor fanoutExporter.Shutdown context propagation: use an explicit
early-return path (remaining <= 0 → use parent ctx) and a clear
non-conditional context.WithTimeout + defer cancel() path for the
deadline case, eliminating the previously asymmetric conditional block.
There was a problem hiding this comment.
Pull request overview
This PR improves the gateway’s OpenTelemetry tracing implementation by reducing allocations when tracing is disabled, centralizing semantic convention usage, making trace error recording explicitly “safe” for security-sensitive paths, tuning batch export behavior, and adjusting fanout shutdown behavior.
Changes:
- Added
span.IsRecording()guards aroundAddEventand attribute slice construction to avoid allocations in noop tracing. - Introduced
RecordSpanErrorSafeand updated an error path to record only a scrubbed/public message in traces. - Centralized
semconv/v1.41.0usage behindinternal/tracing/semconv.gore-exports and tunedBatchSpanProcessorparameters; updated fanout exporter shutdown behavior.
Show a summary per file
| File | Description |
|---|---|
| internal/tracing/span_helpers.go | Adds RecordSpanErrorSafe and migrates span helpers to semconv re-exports. |
| internal/tracing/span_helpers_test.go | Updates semconv usage and adds tests for the new safe error-recording helper. |
| internal/tracing/semconv.go | New central semconv re-export module for tracing package callers. |
| internal/tracing/provider.go | Uses semconv re-exports for resource attrs and tunes SDK batch span processor options. |
| internal/tracing/http.go | Switches HTTP span attributes to semconv re-exports. |
| internal/tracing/genai_attrs.go | Clarifies split between semconv-derived GenAI keys (in semconv.go) and custom keys. |
| internal/tracing/fanout.go | Changes shutdown to use per-exporter timeout contexts based on remaining deadline. |
| internal/server/unified.go | Adds IsRecording() guards and uses RecordSpanErrorSafe for transport/tool execution errors. |
| internal/proxy/handler.go | Adds IsRecording() guards and switches HTTP status key usage to semconv re-exports. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 9/9 changed files
- Comments generated: 3
- Review effort level: Low
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
@copilot fix this failing ci check https://github.com/github/gh-aw-mcpg/actions/runs/28868492598/job/85624699407?pr=8841 |
Fixed in the latest commit. The CI failure was caused by a missing closing brace |
Implements the High/Medium recommendations from the go-fan OTel review: allocation guards for noop spans, explicit error-scrubbing helper, centralised semconv imports, batch processor tuning, and safer fanout shutdown.
Changes
span.IsRecording()guards —unified.go,proxy/handler.goWrap
AddEventcalls and attribute slice construction withif span.IsRecording()to skip allocations entirely in noop mode (the common case when tracing is disabled).RecordSpanErrorSafehelper —internal/tracing/span_helpers.goNew helper that records only a scrubbed public message on the span instead of the internal error, making the security intent explicit and preventing accidental leakage of transport/credential details to trace backends:
Centralised semconv imports —
internal/tracing/semconv.go(new)Single file that imports
go.opentelemetry.io/otel/semconv/v1.41.0and re-exports all attribute keys and helper functions used across the package. All other non-test files (includingproxy/handler.go) now reference these re-exports — a future version bump touches one file instead of five.BatchSpanProcessortuning —internal/tracing/provider.goExplicit
MaxExportBatchSize=256+BatchTimeout=2s(SDK defaults are 512 / 5 s) to reduce export tail latency under gateway throughput patterns.fanoutExporter.Shutdowncontext propagation —internal/tracing/fanout.goEach exporter goroutine now receives a fresh
context.WithTimeoutderived from the parent's remaining deadline at the momentShutdownis called. This prevents a slow exporter from consuming the entire budget and leaving subsequent exporters with an already-expired context.