feat(router): make SpanNameFormatter a configurable variable#2742
feat(router): make SpanNameFormatter a configurable variable#2742SophieDeBenedetto wants to merge 5 commits intowundergraph:mainfrom
Conversation
Convert SpanNameFormatter from a named function to a package-level
variable, allowing custom modules to wrap it with additional processing.
This enables use cases like normalizing high-cardinality operation names
(e.g., stripping hex memory addresses) before they become span names,
without requiring modifications to the core router code.
Usage from a custom module:
func init() {
original := core.SpanNameFormatter
core.SpanNameFormatter = func(op string, r *http.Request) string {
return normalize(original(op, r))
}
}
The default behavior is unchanged — existing code that passes
SpanNameFormatter as a function value continues to work identically.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
SophieDeBenedetto
requested review from
Noroth,
SkArchon,
StarpTech,
devsergiy,
endigma and
jensneuse
as code owners
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
Not stale, needs review. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
Walkthrough
ChangesSpanNameFormatter change + tests
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.1)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
router/core/transport.go (1)
297-308:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftConcurrent reads of mutable
SpanNameFormattercreate a data race if assigned outsideinit().Line 358 (
spanName := SpanNameFormatter("", reqCtx.request)) reads this package-level variable inside goroutines handling live requests. Any write toSpanNameFormatterafter the server starts (e.g., from a module that doesn't follow theinit()-only contract) races with those reads and will be caught by Go's race detector.The PR description relies on the
init()convention as a safety guarantee, but the public API doesn't enforce or document it. Without a mutex oratomic.Pointer, there is no mechanism preventing callers from writing toSpanNameFormatterat any point.Two options:
Option 1 (low-overhead): Document the contract clearly and add a setter with a note.
At minimum, add a doc comment warning that assignment must complete before the router starts accepting requests (i.e., only safe frominit()or beforerouter.Start()).Option 2 (safer): Use
atomic.Pointerfor lock-free concurrent safety.🔒 Proposed atomic approach
+import "sync/atomic" -// SpanNameFormatter formats the span name based on the http request. -// It is a package-level variable so that custom modules can wrap it with -// additional processing (e.g., normalizing high-cardinality operation names). -var SpanNameFormatter = func(_ string, r *http.Request) string { - requestContext := getRequestContext(r.Context()) - - if requestContext != nil && requestContext.operation != nil { - return GetSpanName(requestContext.operation.Name(), requestContext.operation.Type()) - } - - return fmt.Sprintf("%s %s", r.Method, r.URL.Path) -} +// spanNameFormatterDefault is the built-in implementation. +func spanNameFormatterDefault(_ string, r *http.Request) string { + requestContext := getRequestContext(r.Context()) + if requestContext != nil && requestContext.operation != nil { + return GetSpanName(requestContext.operation.Name(), requestContext.operation.Type()) + } + return fmt.Sprintf("%s %s", r.Method, r.URL.Path) +} + +// spanNameFormatterVar holds the active formatter. Use SetSpanNameFormatter/GetSpanNameFormatter +// for concurrent-safe access. +var spanNameFormatterVar atomic.Pointer[func(string, *http.Request) string] + +func init() { + f := func(string, *http.Request) string { return "" } // placeholder + _ = f + SetSpanNameFormatter(spanNameFormatterDefault) +} + +// SetSpanNameFormatter replaces the active span name formatter. +// Safe to call concurrently. Intended to be called from module init() or before +// the router starts serving requests. +func SetSpanNameFormatter(f func(string, *http.Request) string) { + spanNameFormatterVar.Store(&f) +} + +// SpanNameFormatter is the active formatter, safe for concurrent use. +var SpanNameFormatter = func(op string, r *http.Request) string { + return (*spanNameFormatterVar.Load())(op, r) +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@router/core/transport.go` around lines 297 - 308, The package-level variable SpanNameFormatter is read concurrently by request goroutines (see spanName := SpanNameFormatter("", reqCtx.request)) and can race if reassigned after startup; either document the requirement that SpanNameFormatter must only be set before the router starts (e.g., in init() or before router.Start()) and add a setter that states this contract, or make it concurrency-safe by replacing the mutable variable with an atomic.Pointer-based accessor and setter so reads in request handlers use atomic.Load and writes use atomic.Store to avoid data races.
🧹 Nitpick comments (1)
router/core/transport.go (1)
247-251: ⚡ Quick win
SpanNameFormatteris snapshotted here but read live at line 358 — these will diverge if the variable is reassigned.At line 248, the current value of
SpanNameFormatteris passed by value intootelhttp.WithSpanNameFormatter(...). This means the HTTP tracing transport permanently holds the formatter that existed atRoundTripper()call time.At line 358 (
spanName := SpanNameFormatter("", reqCtx.request)), the variable is dereferenced on every gRPC invocation, so it always sees the live value.If a module wraps
SpanNameFormatterat any point afterRoundTripper()is called, HTTP transport spans and gRPC spans will name spans differently.Wrap line 248 in a closure to match the live-read semantics of line 358:
♻️ Proposed fix for consistent live-read behavior
otelHttpOptions := []otelhttp.Option{ - otelhttp.WithSpanNameFormatter(SpanNameFormatter), + otelhttp.WithSpanNameFormatter(func(op string, r *http.Request) string { + return SpanNameFormatter(op, r) + }), otelhttp.WithSpanOptions(otrace.WithAttributes(otel.EngineTransportAttribute)), otelhttp.WithTracerProvider(t.tracerProvider), }The same snapshot pattern exists in
graph_server.goat line 344 (SpanNameFormatter: SpanNameFormatter), which should be made consistent as well.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@router/core/transport.go` around lines 247 - 251, The HTTP transport currently captures the value of SpanNameFormatter when building otelHttpOptions via otelhttp.WithSpanNameFormatter, causing divergence from code that calls SpanNameFormatter live (e.g. spanName := SpanNameFormatter("", reqCtx.request)); change the otelhttp.WithSpanNameFormatter argument to a closure that calls the global SpanNameFormatter at runtime (so the formatter is looked up live) and make the same change in graph_server.go where SpanNameFormatter is assigned (replace the snapshot assignment with a function/closure that delegates to the live SpanNameFormatter) to ensure HTTP and gRPC span names remain consistent if SpanNameFormatter is reassigned.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@router/core/transport.go`:
- Around line 297-308: The package-level variable SpanNameFormatter is read
concurrently by request goroutines (see spanName := SpanNameFormatter("",
reqCtx.request)) and can race if reassigned after startup; either document the
requirement that SpanNameFormatter must only be set before the router starts
(e.g., in init() or before router.Start()) and add a setter that states this
contract, or make it concurrency-safe by replacing the mutable variable with an
atomic.Pointer-based accessor and setter so reads in request handlers use
atomic.Load and writes use atomic.Store to avoid data races.
---
Nitpick comments:
In `@router/core/transport.go`:
- Around line 247-251: The HTTP transport currently captures the value of
SpanNameFormatter when building otelHttpOptions via
otelhttp.WithSpanNameFormatter, causing divergence from code that calls
SpanNameFormatter live (e.g. spanName := SpanNameFormatter("", reqCtx.request));
change the otelhttp.WithSpanNameFormatter argument to a closure that calls the
global SpanNameFormatter at runtime (so the formatter is looked up live) and
make the same change in graph_server.go where SpanNameFormatter is assigned
(replace the snapshot assignment with a function/closure that delegates to the
live SpanNameFormatter) to ensure HTTP and gRPC span names remain consistent if
SpanNameFormatter is reassigned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e7b45608-7b47-4a47-b317-4bcd613eea9b
📒 Files selected for processing (1)
router/core/transport.go
|
Hi @SophieDeBenedetto, would you mind adding a test? Check telemetry tests in |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2742 +/- ##
==========================================
+ Coverage 1.50% 58.94% +57.44%
==========================================
Files 315 238 -77
Lines 44099 25946 -18153
Branches 451 0 -451
==========================================
+ Hits 663 15295 +14632
+ Misses 43132 9127 -34005
- Partials 304 1524 +1220
🚀 New features to boost your workflow:
|
Add unit tests covering: - Default behavior with operation context (named and unnamed operations) - Fallback to HTTP method + path when no request context - Variable can be overridden by custom modules (wrapping original) - GetSpanName helper function Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
added an integration and a unit test 😄 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@router/core/transport_test.go`:
- Around line 13-56: TestSpanNameFormatter_DefaultBehavior's parallelization
causes a data race with TestSpanNameFormatter_Configurable because subtests read
the package-level variable SpanNameFormatter while the configurable test writes
it; fix by removing t.Parallel() from the top-level
TestSpanNameFormatter_DefaultBehavior and from each of its three subtests so
they run sequentially and do not overlap with
TestSpanNameFormatter_Configurable's reassignment of SpanNameFormatter.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 54607844-fc1c-4a7b-8245-3c20b4a75201
📒 Files selected for processing (1)
router/core/transport_test.go
| func TestSpanNameFormatter_DefaultBehavior(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| t.Run("returns operation type and name when request context has operation", func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| req := httptest.NewRequest(http.MethodPost, "/graphql", nil) | ||
| rc := buildRequestContext(requestContextOptions{r: req}) | ||
| rc.operation = &operationContext{ | ||
| name: "GetUser", | ||
| opType: "query", | ||
| } | ||
| ctx := context.WithValue(req.Context(), rcontext.RequestContextKey, rc) | ||
| req = req.WithContext(ctx) | ||
|
|
||
| result := SpanNameFormatter("", req) | ||
| assert.Equal(t, "query GetUser", result) | ||
| }) | ||
|
|
||
| t.Run("returns operation type and unnamed when operation name is empty", func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| req := httptest.NewRequest(http.MethodPost, "/graphql", nil) | ||
| rc := buildRequestContext(requestContextOptions{r: req}) | ||
| rc.operation = &operationContext{ | ||
| name: "", | ||
| opType: "mutation", | ||
| } | ||
| ctx := context.WithValue(req.Context(), rcontext.RequestContextKey, rc) | ||
| req = req.WithContext(ctx) | ||
|
|
||
| result := SpanNameFormatter("", req) | ||
| assert.Equal(t, "mutation unnamed", result) | ||
| }) | ||
|
|
||
| t.Run("returns method and path when no request context", func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| req := httptest.NewRequest(http.MethodGet, "/health", nil) | ||
|
|
||
| result := SpanNameFormatter("", req) | ||
| assert.Equal(t, "GET /health", result) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Data race: parallel subtests concurrently read SpanNameFormatter while TestSpanNameFormatter_Configurable writes it.
In Go's test runner, top-level tests without t.Parallel() run sequentially, but parallel subtests are only unblocked when their parent function returns — after which they execute concurrently with whatever the runner starts next, including TestSpanNameFormatter_Configurable. Because these subtests call SpanNameFormatter(...) (a read) while the configurable test reassigns SpanNameFormatter (a write), running with -race will flag this.
The fix is to drop t.Parallel() from the three subtests inside TestSpanNameFormatter_DefaultBehavior (they're cheap HTTP-context manipulations with no I/O, so parallelism adds no value):
🔒 Proposed fix: remove t.Parallel() from subtests that read the package-level variable
func TestSpanNameFormatter_DefaultBehavior(t *testing.T) {
t.Parallel()
t.Run("returns operation type and name when request context has operation", func(t *testing.T) {
- t.Parallel()
req := httptest.NewRequest(http.MethodPost, "/graphql", nil)
// ...
})
t.Run("returns operation type and unnamed when operation name is empty", func(t *testing.T) {
- t.Parallel()
req := httptest.NewRequest(http.MethodPost, "/graphql", nil)
// ...
})
t.Run("returns method and path when no request context", func(t *testing.T) {
- t.Parallel()
req := httptest.NewRequest(http.MethodGet, "/health", nil)
// ...
})
}Also remove t.Parallel() from the top-level TestSpanNameFormatter_DefaultBehavior call itself so that it is fully sequenced relative to TestSpanNameFormatter_Configurable.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@router/core/transport_test.go` around lines 13 - 56,
TestSpanNameFormatter_DefaultBehavior's parallelization causes a data race with
TestSpanNameFormatter_Configurable because subtests read the package-level
variable SpanNameFormatter while the configurable test writes it; fix by
removing t.Parallel() from the top-level TestSpanNameFormatter_DefaultBehavior
and from each of its three subtests so they run sequentially and do not overlap
with TestSpanNameFormatter_Configurable's reassignment of SpanNameFormatter.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
The latest commit should address this issue!
Add telemetry integration tests that verify: - Default formatter produces correct root span names for named and anonymous operations - Custom SpanNameFormatter override is applied to transport (client) spans Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
3 participants
Hello from GitHub! We recently an encountered an issue using the distributed tracing instrumentation built into this package that resulted in very high-cardinality span names which our tracing backend cannot support. More info below.
Summary
Convert
SpanNameFormatterfrom a named function to a package-level variable, allowing custom modules to wrap it with additional processing without modifying core router code.Motivation
When using custom modules with the Cosmo Router, there's currently no way to customize span naming without forking the router. A common need is normalizing high-cardinality operation names (e.g., stripping hex memory addresses from auto-generated names) before they become span/resource names in APM backends like Datadog.
Since
SpanNameFormatteris a named function, modules can't wrap or replace it. Converting it to avarmakes it externally configurable while preserving the exact same default behavior.Change
One-line change in
router/core/transport.go:Usage
Custom modules can wrap the formatter in their
init():Backward Compatibility
This is fully backward compatible:
SpanNameFormatteras a function value continue to work identicallySummary by CodeRabbit