perf(tracer): stop rebuilding span metadata on every request by aryanmehrotra · Pull Request #3970 · gofr-dev/gofr

aryanmehrotra · 2026-08-18T09:58:16Z

TL;DR — 8 fewer allocations per request. No behaviour change.

The problem

The tracing middleware runs on every request. For each one it rebuilt things that never change for a given route:

the span name ("GET /users/{id}") — via fmt.Sprintf
the attribute slice (http.request.method, http.route)
the trace.WithAttributes option wrapper and the variadic slice holding it
the status attribute — even when the span was non-recording and threw it away
a response-writer wrapper — even when nothing recorded the status
propagation headers — recanonicalised on every lookup

For a fixed route table, all of that produces byte-identical results every single time.

The fix

Build it once per (method, route) and reuse it. Six focused commits, one idea each.

Cache key is the route template (/users/{id}), never the concrete path. Unmatched requests carry a raw, attacker-controlled path and are never cached.

Revised after review. That paragraph used to end "so it is bounded by routes × methods", and it wasn't. Neither half of that product bounds itself in a real GoFr app:

gofr.go registers a PathPrefix("/") catch-all, so mux.CurrentRoute is set for every request and GetPathTemplate() returns "/" even for a path nothing matched. The templated guard was therefore always true — reproduced with method ZZZQUUX on /totally/unknown.

net/http accepts any RFC 7230 token as a method, so a client streaming M00001, M00002, … minted a permanent *spanMeta per request. Reproduced: 5000 distinct methods against the catch-all produced 5000 permanent entries.

That is a remotely-triggerable unbounded-memory DoS, and the pre-cache fmt.Sprintf path retained zero state — so it was introduced here, not inherited. Two things bound it now:

cacheableMethod restricts the key to the nine defined HTTP methods, which turns "methods × routes" back into the fixed quantity the cache was designed around. A made-up method still works — it just rebuilds its span metadata, exactly as every request did before the cache existed.

routeCache caps the whole thing at 4096 entries, because the first bound is an argument about reachability and process memory should rest on a number too.

routeCache and cacheableMethod live in their own file because the metrics caches in #3972 have the same shape on the same key space, and the review asked for all three to be bounded consistently rather than one at a time. #3972 is now stacked on this PR.

The numbers

Two benchmarks ship with this, because a sampling service runs both paths and neither may regress:

	before	after
non-recording provider	21 allocs, 2074 B	11 allocs, 1568 B
recording provider	23 allocs, 2875 B	17 allocs, 2658 B

Allocation counts only — see Measurement below.

The non-recording row was re-measured after review, and it improved: it previously read 13 allocs / 1760 B. BenchmarkTracer documented the non-recording path but did not measure it — TestTracerPropagatesIncomingTraceContext installed a live recording TracerProvider and never restored it, and tests run before benchmarks, so the benchmark ran recording and fed that test's in-memory exporter. It now pins noop.NewTracerProvider() itself, and that test restores both globals it replaces (the provider and the Baggage-less propagator).

Why it matters

Tracing is on the hot path of every request in every GoFr service. This is the single largest per-request allocation saving in the middleware chain.

Baggage regression, found in review and fixed

headerCarrier replaces propagation.HeaderCarrier to avoid canonicalising the lookup key per request — but implemented only Get/Set/Keys. The stdlib type it replaces also satisfies propagation.ValuesGetter, and propagation.Baggage.Extract type-asserts to that interface to combine all values of the Baggage header. The assertion failed, so Extract fell back to the single-value Get and silently dropped every baggage member after the first.

It only surfaces when a request carries more than one Baggage header — legal per W3C, and what proxies and service meshes commonly emit — and GoFr installs propagation.Baggage in its default composite propagator, so the path is live. Reproduced against the stdlib carrier with three Baggage headers:

stdlib HeaderCarrier : members=3  "k2=v2,k3=v3,k1=v1"
gofr   headerCarrier : members=1  "k1=v1"
implements ValuesGetter: gofr=false stdlib=true

Fixed by implementing Values. It deliberately does not use the canonical-key fast path: baggage is not one of the keys that map covers, and a carrier replacing a stdlib one has to be a faithful drop-in first and an optimisation second.

The pre-existing equivalence test compared only Get and Keys, so it could not catch this. The new test compares extracted baggage member by member against the stdlib carrier across four header shapes, and asserts the interface is satisfied.

BenchmarkTracer_WithPropagationHeaders closes the other gap: the existing benchmarks send a bare request, so the propagators find nothing and the canonicalisation this PR avoids never runs — their delta is the per-route cache and the IsRecording gate alone. The new one sends traceparent, tracestate and two Baggage headers, which is what a service behind a mesh actually receives.

What the cache costs

The saving is bought with resident memory: the cache is process-global and entries are never released, because they are pure functions of their key and stay valid for the life of the process.

Measured with runtime.MemStats around a filled cache, stable across three runs:

Shape	Entries	Heap held
small app — 25 routes x GET/POST	50	+22 KB
typical app — 100 routes x GET/POST	200	+83 KB
large app — 500 routes x 4 methods	2000	+825 KB
at the `routeCacheLimit` ceiling	4096	+1735 KB

So a realistic service holds tens of kilobytes, and the worst case any service can reach is ~1.7 MB, permanently. That ceiling is the point of routeCacheLimit — without it the table has no last row.

It is a genuine trade rather than a free win, and it is the one thing in this PR that costs something. Weigh it against ~9 allocations saved on every request.

Safety

No behaviour change. Span names, attributes and propagation are identical.
The IsRecording()-gated non-recording branch — the path this PR optimizes — now has a test; it had none.
buildSpanName's godoc no longer quotes a 44.3 → 22.6 ns/op figure: the fmt.Sprintf baseline it was measured against is gone, so nothing in the tree can reproduce it.
New tests for the bound: an undefined method mints no entry, standard methods still hit the cache, and the cap holds.
Cached data is pure and provider-independent, so it stays correct across TracerProvider replacement.

Measurement

Only allocations and bytes are quoted. Wall-clock on the machine used varied by more than 5× between runs of the same binary, so ns/op there is meaningless. Allocation counts were stable and moved monotonically with each commit in the series.

Independent of the other perf/* PRs — touches only tracer.go and its test. Merge in any order.

PiyushSingh-ZS · 2026-08-18T11:49:59Z

Reviewed against development, built and ran the middleware tests on the branch. The caching logic is correct, and the two things I most expected to be wrong here turn out to be fine:

Sharing startOpts / attrs across concurrent spans is safe. attributeOption.applySpan appends into the span config's slice, which starts nil, so the append allocates rather than writing into the shared array. The SDK likewise appends into the new span's own (empty) attribute slice. Nothing mutates the cached slice.
The cache key is sound. Keying only on resolved route templates, with templated gating entry into the map, is what keeps this bounded — and the !templated early return means an unmatched, attacker-controlled path never enters it. The bool return from routeTemplate is the right shape for that.
headerCarrier. Behaviour matches propagation.HeaderCarrier for the three W3C keys (Go canonicalizes incoming header keys at parse time, so Traceparent is what's actually in the map), and unrecognized keys correctly fall through to Header.Get, so B3/Jaeger-style custom propagators keep working. Keys() matches otel's implementation exactly.

`spanMeta.attrs` is dead

It's assigned in newSpanMeta and never read — startOpts is the only consumer. Either drop the field or, if it's kept for a future SetAttributes path, say so, because right now it reads as if something still uses it.

The `IsRecording()` gate changes what downstream sees

The reasoning is right — Tracer is outermost, so the assertion always failed and every request allocated a wrapper that a NeverSample deployment never read. I confirmed the chain order (http_server.go:66: Tracer → Logging → CORS → Metrics), and that dropping the wrap is harmless there: Logging wraps immediately after, and Metrics reuses Logging's wrapper.

The part worth documenting is that the identity of the ResponseWriter seen downstream now depends on sampling. Any user middleware inserted between Tracer and Logging that type-asserts *StatusResponseWriter will behave differently on a sampled vs unsampled request, which is a genuinely hard bug to find. One line in the comment saying so would pay for itself.

Minor

tracerSpanCache is package-level with no eviction. Bounded by methods × routes in production, so fine — but it also persists across App instances within a single test binary, which is worth knowing if anyone later asserts on cache contents in a test.

NitinKumar004

The caching direction is reasonable, but as written this introduces a remotely-triggerable unbounded-memory DoS. Blocking on that.

🔴 Blocker — unbounded span cache → remote OOM

tracerSpanCache is a package-global sync.Map keyed on (method, route-template) with no eviction. The safety claim is that only bounded route templates are cached and raw/unmatched paths never are — but that guard is dead in a real GoFr app:

GoFr registers a catch-all router.PathPrefix("/"), so mux.CurrentRoute(r) is set for every request and GetPathTemplate() returns ("/", nil) even for unknown paths (verified with method ZZZQUUX, path /totally/unknown). So templated == true always, and everything is cached under {method, "/"}.
method = strings.ToUpper(r.Method) is not restricted — Go's server accepts any RFC-7230 method token. An unauthenticated client streaming distinct methods (M00001, M00002, …) mints a permanent *spanMeta each → resident memory grows without bound → OOM.

The pre-PR fmt.Sprintf path retained zero state, so this DoS vector is newly introduced, and the "no behaviour change / deliberately never cached" claims don't hold. Fix: don't cache the / catch-all (or any prefix catch-all); and/or cap+evict; and/or only cache routes registered via explicit Methods().Path().

Medium

BenchmarkTracer measures the recording path, not the documented non-recording path. A prior test (TestTracerPropagatesIncomingTraceContext) installs a live recording TracerProvider and never restores noop; tests run before benchmarks, so BenchmarkTracer runs with IsRecording()==true and feeds the leaked in-memory exporter. The "non-recording: 13 allocs" headline only reproduces in isolation. Install noop.NewTracerProvider() with b.Cleanup, like BenchmarkTracer_Recording.

Low

The IsRecording()-gated non-recording branch (the path this PR optimizes) has no test.
TestTracerPropagatesIncomingTraceContext also leaks the global TextMapPropagator (Baggage-less) with no Cleanup, making the suite order-dependent.
buildSpanName godoc + test enshrine a 44.3 → 22.6 ns/op figure that no shipped benchmark can reproduce (the fmt.Sprintf baseline was removed).

⚠️ Cross-PR (HIGH, coordinate with #3972)

This is not the only cache with this shape. #3972 adds optionRecorder.cache (bounded 4096) and attrsRecorder.cache (unbounded) on the same (path, method, status) key space. The same malicious stream inflates all three. Fixing this cache in isolation is insufficient — bound all three consistently, reviewed as one unit. Also overlaps #3770 in the StatusResponseWriter block.

Requested changes: eliminate the unbounded-cache DoS (blocker); fix the benchmark + propagator leaks; add a non-recording test; and coordinate cache-bounding with #3972.

Addresses the blocker on #3970. tracerSpanCache was a package-global sync.Map with no eviction, and its safety argument - "only bounded route templates are cached" - did not survive contact with a real GoFr app. gofr.go registers a PathPrefix("/") catch-all, so mux.CurrentRoute is set for every request and GetPathTemplate() returns "/" even for a path nothing matched. The templated guard is therefore true always, and the route half of the key is not the bound it looked like. Reproduced: method "ZZZQUUX" on "/totally/unknown" resolves templated=true, route="/". The method half is worse. net/http accepts any RFC 7230 token, so an unauthenticated client streaming M00001, M00002, ... minted a permanent *spanMeta per request. Reproduced: 5000 distinct methods against the catch-all produced 5000 permanent entries. The pre-cache fmt.Sprintf path retained no state, so this was introduced by the cache rather than inherited. Two things now bound it. cacheableMethod restricts the key to the nine defined HTTP methods, which turns "methods x routes" back into the fixed quantity the cache was designed around; a request with a made-up method still works, it just rebuilds its span metadata as every request did before the cache existed. routeCache then caps the whole thing at 4096 entries, because the first bound is an argument about reachability and process memory should rest on a number too. routeCache and cacheableMethod live in their own file because the metrics caches in #3972 have the same shape on the same attacker-influenced key space, and the reviewer asked for all three to be bounded consistently rather than one at a time. BenchmarkTracer documented the non-recording path but did not measure it: TestTracerPropagatesIncomingTraceContext installs a live recording provider and never restored it, and tests run before benchmarks, so the benchmark ran recording and fed that test's in-memory exporter. Both globals it replaces are restored now, and the benchmark pins the noop provider itself. It reports 11 allocs/op on the path it describes. The non-recording branch this PR optimizes had no test; it has one now. buildSpanName's godoc no longer quotes a 44.3 -> 22.6 ns/op figure that nothing in the tree can reproduce, since the fmt.Sprintf baseline it was measured against is gone.

Addresses review on #3972. Both caches now go through the routeCache introduced for the span cache in #3970, under the same rules, because all three sit on the same attacker-influenced key space and bounding them one at a time does not help - one malicious request stream inflates whichever is left unbounded. attrsRecorder.cache had no ceiling at all while its sibling had one. Only a templated route may be cached now. Every branch of the path label that falls back to r.URL.Path yields a caller-controlled string - an unmatched route, a static-asset extension, /static, and the "/" template GoFr's PathPrefix("/") catch-all produces for every unmatched request. Those used to be cached, so a stream of unique unmatched paths filled the cache to its ceiling, after which every first-seen legitimate route could never be stored and rebuilt its measurement option per request forever. Memory stayed bounded and correctness was unaffected, but the optimization silently reverted to baseline for production traffic. The method half is filtered too, for the reason given in cacheableMethod: a bounded route with an unbounded method reopens the same growth. The fast path all production traffic uses now has label assertions. metricsManager implements RecordHistogramOpt, so requests flow through optionRecorder, yet nothing pinned what it emitted. Changing status from attribute.String to attribute.Int, or emitting the concrete path instead of the route template, left every test green while dashboards broke and cardinality exploded. Both are now caught - verified by making each change and watching the test fail. TestMetricsOptCacheIsBounded asserted that a compile-time constant was positive, so deleting the ceiling kept it green. It drives more than the limit in distinct keys through record() and inspects the cache size now; deleting the ceiling fails it, TestRouteCacheStopsAtItsLimit and TestAttrsRecorderIsCorrectAndBounded together. attrsRecorder was at zero coverage - no in-tree type selects it, since *metricsManager implements both optional interfaces and optionRecorder wins - and now has direct tests for its labels, its reuse and its bound. The "-650 B/request" figure is dropped. The shipped benchmarks give BenchmarkRecordHistogramAttrs at 424 B/op and 4 allocs/op against BenchmarkRecordHistogramOpt at 0 B/op and 0 allocs/op, with BenchmarkAttrBuild_HTTP at 448 B/op for the attribute build a cache hit also avoids. Those are the numbers the PR body now quotes.

Umang01-hash

Strong, careful perf PR — I verified the risky parts hold: the shared per-route spanMeta cache is concurrency-safe (trace.WithAttributes only reads the cached slice; otel applies it via append(cfg.attributes, o...), and -race passes), the IsRecording gate is correct (http_server.go registers Tracer before Logging, so Tracer is outermost and the old StatusResponseWriter assertion always failed — the stale comment was wrong), and the tracer cache is genuinely DoS-bounded (cacheableMethod + routeCacheLimit). gofmt/vet/-race/benchmarks all green. One regression to fix before merge, plus two notes.

Baggage-loss regression (introduced here) — please fix

headerCarrier replaces propagation.HeaderCarrier but implements only Get/Set/Keys, not Values(string) []string. In otel v1.45.0, propagation.Baggage.Extract type-asserts the carrier to ValuesGetter — the stdlib HeaderCarrier satisfies it and combines all Baggage header values; headerCarrier fails the assertion, so Extract falls back to single-value Get("baggage") and silently drops every baggage member after the first when a request carries multiple Baggage headers (legal per W3C; commonly emitted by proxies/meshes). GoFr installs propagation.Baggage in the default composite propagator, so this is live.

Reproduced locally: stdlib carrier → 2 members, headerCarrier → 1. TestHeaderCarrierMatchesHeaderCarrier only compares Get/Keys, so it doesn't catch it. Fix is one line:

func (c headerCarrier) Values(key string) []string { return http.Header(c).Values(key) }

plus a multi-Baggage equivalence assertion. A custom carrier replacing a stdlib one needs to be a faithful drop-in.

Note — the metrics sibling DoS (`metrics.go` `routeAttrs`) is still open

Not introduced here, and your commit message already calls it out for #3972 (the reusable routeCache is built for exactly this). Just flagging that it's the same live unbounded-key DoS — an unauthenticated client streaming distinct RFC-7230 method tokens grows routeAttrs without bound (repro'd ~61MB/200k requests). Landing this without #3972 leaves that hole open, so it'd be good to land them together or back-to-back.

Nit — benchmark doesn't exercise `headerCarrier`

BenchmarkTracer/_Recording set no propagation headers, so they never hit the canonicalization optimization — the measured alloc delta is from the cache + IsRecording only. The headerCarrier win is real by inspection but unproven by the bench; a header-bearing variant would close that.

No breaking API change (all new symbols unexported). Nice work overall — the DoS analysis in the doc comments is excellent.

… not lost Addresses review on #3970. headerCarrier replaces propagation.HeaderCarrier to avoid canonicalizing the lookup key on every request, but implemented only Get/Set/Keys. The stdlib type it replaces also satisfies propagation.ValuesGetter, and propagation.Baggage.Extract type-asserts to that interface to combine ALL values of the Baggage header. The assertion failed, so Extract fell back to the single-value Get and silently dropped every baggage member after the first. It only shows up when a request carries more than one Baggage header, which is legal per W3C and is what proxies and service meshes commonly emit, and GoFr installs propagation.Baggage in its default composite propagator - so the path is live. Reproduced against the stdlib carrier with three Baggage headers: stdlib extracted 3 members, headerCarrier extracted 1. The pre-existing equivalence test compared only Get and Keys, so it could not catch this. The new test compares extracted baggage member by member against the stdlib carrier across four header shapes, and asserts the interface is satisfied. Values deliberately does not use the canonical-key fast path: baggage is not one of the keys that map covers, and a carrier replacing a stdlib one has to be a faithful drop-in first and an optimization second. BenchmarkTracer and BenchmarkTracer_Recording send a bare request, so the propagators find nothing and the canonicalization this PR avoids never runs - their delta is the per-route cache and the IsRecording gate alone. BenchmarkTracer_WithPropagationHeaders sends traceparent, tracestate and two Baggage headers, which is what a service behind a mesh actually receives and the only shape where the carrier's saving is visible. The propagator lookup keys are constants now rather than three repeated string literals.