perf(metrics): build the measurement option once per route, method and status by aryanmehrotra · Pull Request #3972 · gofr-dev/gofr

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

TL;DR — 4 allocations per observation → 0. No behaviour change.

The problem

Every request records a histogram. Every observation called:

metric.WithAttributes(attrs...)

which sorts and deduplicates the attributes into a new attribute.Set and wraps it in an option.

For a request metric the labels are (route, method, status) — drawn from a small fixed set. So that work produced an identical option over and over.

The fix

Build the option once per (route, method, status) and reuse it, via a new option-based recorder on the metrics manager.

The option is passed as a slice, not a single value — passing one option would allocate a fresh one-element slice for the variadic on every call, undoing the saving.

The numbers

Both benchmarks ship here, so before/after is visible side by side without checking out the old revision:

	allocs/op	B/op	ns/op
`RecordHistogramAttrs` (rebuilds each time)	4	424	~928
`RecordHistogramOpt` (cached option)	0	0	~164

Revised after review. This line used to claim −650 B/request, which no shipped benchmark reproduces. The numbers above are the reproducible ones: the recording call goes 424 B → 0 B, and a cache hit also avoids the attribute build that BenchmarkAttrBuild_HTTP measures at 448 B/op, 3 allocs/op.

Safety

No behaviour change. Same metric, same labels, same values.
The caches are keyed on the route template, never the concrete path.

Revised after review. "Bounded by routes × methods × statuses" was not true, and attrsRecorder.cache had no ceiling at all while its sibling had one.

Only templated routes are 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 that every first-seen legitimate route could never be stored and rebuilt its option per request forever. Memory stayed bounded and correctness was unaffected — the optimization just silently reverted to baseline for production traffic.

The method half is filtered too (cacheableMethod): a bounded route with an unbounded method reopens the same growth.

Both caches now share routeCache with the span cache from perf(tracer): stop rebuilding span metadata on every request #3970, under the same rules. Three caches on one attacker-influenced key space had to be bounded together, so this PR is now stacked on perf(tracer): stop rebuilding span metadata on every request #3970.

Test coverage added after review

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 now fail the suite; verified by making each change.
TestMetricsOptCacheIsBounded drives the guard. It used to assert only that a compile-time constant was positive, so deleting the ceiling kept it green. Deleting it now fails three tests.
attrsRecorder was at 0% 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.

Measurement

ns/op is quoted here only because both benchmarks ran back to back on the same binary; the allocation counts are the reliable signal.

#3970 has merged, so this now targets development directly and carries the shared routeCache/cacheableMethod bound from it. The two were reviewed as one unit at the reviewer's request; this is the half that closes the metrics-side cardinality hole.

PiyushSingh-ZS · 2026-08-18T11:50:02Z

Reviewed against development, built and tested on the branch. The three-strategy split reads well and the layering decision — select once at construction, keep the branching out of the request path — is the right one.

Checks I ran:

Confirmed the runtime type is *metricsManager (container.go:139 / container.go:259), so the optionRecorder path is the one that actually executes in a real app.
RecordHistogramOpt was added to the concrete type only, not to the exported metrics.Manager interface, so existing implementers and generated mocks are unaffected. That's the right call and worth keeping that way.
Emitted label values are unchanged: strconv.Itoa(status) matches the old fmt.Sprintf("%d", status), and status stays a string across all three paths, so OTLP consumers see the same types.
Sharing a built metric.RecordOption across concurrent observations is safe — WithAttributes holds an immutable attribute.Set and Record only reads it.

`attrsRecorder`'s cache has no equivalent of `optCacheLimit`

The rationale given for the cap on the option cache is exactly right:

The path label falls back to the raw request path when no route template is available, and a raw path is caller-controlled.

But that argument applies identically to attrsRecorder.cache, which has no ceiling. An external metrics implementation that satisfies metricsAttrer but not metricsOptRecorder gets an unbounded sync.Map fed by attacker-controlled keys. Low practical impact since in-tree traffic takes the option path, and it's inherited rather than introduced here — but this PR is where the bound is being reasoned about, so it seems like the moment to apply it in both places.

Related, and probably a separate issue

The same fallback (path = r.URL.Path when no route template resolves, metrics.go:73) is an unbounded metric cardinality source, not just a cache-size one — every unmatched path becomes its own time series in the exporter. The cache limit bounds the memory this middleware holds, but not what gets shipped to the backend. Worth a follow-up.

Nit

o.count.Load() < optCacheLimit followed by LoadOrStore can overshoot the limit slightly under concurrency. Harmless at this bound, just noting it's approximate rather than exact.

NitinKumar004

Production code is correct; requesting changes for test coverage of the new fast path, cache behavior under catch-all traffic, and cross-PR cache coordination.

Medium

The optionRecorder fast path — the one all real traffic now uses — has no label assertions. metricsManager implements RecordHistogramOpt, so production flows through optionRecorder, yet the only test touching it (TestMetricsOptRecorderIsUsedAndStillRecords) drops the opts and just counts hits. Nothing asserts the path is the route template /users/{id} (not /users/7), that method/status keys are present, or that status is attribute.String (matching the slow path so OTLP types agree). Change String→Int or emit the concrete path and every test stays green while dashboards break and cardinality explodes.

Low

TestMetricsOptCacheIsBounded only asserts require.Positive(optCacheLimit) — a compile-time constant. It never drives >limit distinct keys through record() nor inspects count/cache size, so deleting the if o.count.Load() < optCacheLimit guard (re-introducing unbounded growth) still passes. The advertised bound has no executable coverage.
Shared 4096-slot cache lets catch-all raw-path traffic starve real routes. The PathPrefix("/") catch-all always takes the raw r.URL.Path fallback; 4096+ unique unmatched paths saturate the cache, after which every first-seen legitimate combo can never be stored and record() rebuilds metric.WithAttributes per request forever — the optimization silently reverts to baseline for production traffic (memory stays bounded; correctness unaffected). Cache only when RouteTemplate(r) != "", or give the raw-path fallback a separate budget.
attrsRecorder is dead + untested in-tree, and its cache is unbounded. No in-tree type selects it (*metricsManager implements both), so it runs at 0% coverage, and its sync.Map has no optCacheLimit ceiling — an external RecordHistogramAttrs-only backend grows it without bound under catch-all traffic.
The headline "-650 B/request" is not reproducible from the shipped benchmarks (they yield ~424 B/op).

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

These metrics caches + #3970's tracerSpanCache are three caches on the same attacker-influenced key space with inconsistent bounding (attrsRecorder.cache unbounded). One malicious request stream inflates several. Bound all three consistently and review as one unit.

Requested changes: assert label/type/template on the opt path; make the cache-bound test actually drive the guard; cache only templated routes (or a separate raw-path budget); bound attrsRecorder.cache; align the perf-table numbers with the shipped benchmarks; coordinate with #3970.

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.

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.

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.

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.

aryanmehrotra · 2026-08-27T09:01:48Z

All six items are addressed. Re-requesting review — the CHANGES_REQUESTED verdict sits on 7a41fd973 and the head is now 5d30c466b.

The fast path now has label assertions

You were right that this was the gap that mattered: metricsManager implements RecordHistogramOpt, so production flows through optionRecorder, and nothing pinned what it emitted. TestMetricsOptRecorderLabels asserts the route template, the method, and that status is attribute.String.

I also added a test that compares all three strategies against each other on the same requests, because "the opt path is right" is weaker than "the three cannot disagree":

GET /users/42     slow="method=STRING:GET path=STRING:/users/{id} status=STRING:201"
                  attr="method=STRING:GET path=STRING:/users/{id} status=STRING:201"
                  opt ="method=STRING:GET path=STRING:/users/{id} status=STRING:201"

Identical across six request shapes — same keys, same values, same attribute types. Both mutations you named now fail the suite: String→Int on status, and emitting the concrete path instead of the template.

Catch-all traffic can no longer starve real routes

This was the subtlest of your findings and it needed a design change, not just a ceiling. Every branch of the path label that falls back to r.URL.Path — unmatched route, static extension, /static, and the "/" template GoFr's catch-all produces — yields a caller-controlled string. Those are no longer cached at all:

after 6000 catch-all requests:        entries=0
real route can still be cached:       entries=1
6000 undefined methods on one route:  entries=0
attrsRecorder at ceiling:             entries=4096 (limit 4096)

attrsRecorder had no ceiling at all while its sibling had one — exactly the inconsistency that makes a shared key space dangerous. Both go through routeCache now, and it has direct tests for its labels, its reuse and its bound; it was previously at 0% coverage because no in-tree type selects it.

`TestMetricsOptCacheIsBounded` drives the guard

It asserted only that a compile-time constant was positive, so deleting the ceiling kept it green. It now pushes past the limit in distinct keys and inspects the cache; deleting the ceiling fails it along with two others.

The `−650 B/request` claim is retracted

No shipped benchmark reproduces it. The PR body now quotes the reproducible pair — BenchmarkRecordHistogramAttrs at 424 B/op, 4 allocs/op against BenchmarkRecordHistogramOpt at 0 B/op, 0 allocs/op, with BenchmarkAttrBuild_HTTP at 448 B/op for the attribute build a cache hit also avoids.

Measured through the middleware against this PR's base:

base  1456 B/op  11 allocs/op
3972  1232 B/op  10 allocs/op

Worth recording that the saving depends on which optional interface the backend implements: a backend offering only RecordHistogramAttrs sees −32 B, because that path was already cheap. The −224 B is the RecordHistogramOpt path, which is what metricsManager selects.

Coordinated with #3970, as you asked

routeCache and cacheableMethod live in their own file for exactly this reason, and this PR is stacked on #3970 so the three caches are bounded by one mechanism under one set of rules rather than three ad-hoc ceilings.

No behaviour change

14 label cases diffed against the base — matched templates, unmatched paths, undefined methods, root, /static prefix, static extensions including uppercase and with a query string, trailing-slash trimming, the /graphql skip, and /graphql appearing in the query rather than the path. All identical.

-race clean on the http and metrics trees, golangci-lint --new-from-rev reports 0 issues, 0 behind development.

Sprintf walks a format string and boxes two arguments that are already strings, to produce a value that concatenation produces directly. Measured by BenchmarkBuildSpanName at the real call site: before 44.33 ns/op 16 B/op 1 allocs/op after 22.61 ns/op 16 B/op 1 allocs/op This is a CPU saving only, not an allocation saving. Both forms pay the one unavoidable allocation for the result string, because the span name escapes into the span; runtime.concatstrings only uses its 32-byte stack buffer when escape analysis proves the result stays in the frame. The span name and http.route attribute are unchanged, and are now pinned by tests rather than left implicit. (cherry picked from commit 9b03ddfc79c1c2bbe7cf7d72d753a9bebbbcc600)

A non-recording span discards SetAttributes by contract, but the call still builds the variadic []attribute.KeyValue to hand it. GoFr installs an SDK provider with NeverSample when no TRACE_EXPORTER is configured, so on the default deployment that slice was allocated and thrown away on every single request. Measured by a Tracer middleware benchmark under NeverSample: before 2072 B/op 19 allocs/op after 2008 B/op 18 allocs/op Recording spans are unaffected: the status attribute is still set, and is now pinned by TestTracerStatusAttributeStillRecorded. The implicit-200 normalization established by #3431 is pinned by TestTracerImplicit200StillRecorded. (cherry picked from commit aba7cbfffee78119a254c4d223bb24f438696a27)

The span name and the http.route attribute set are pure functions of (method, route template), yet both were rebuilt on every request: one string allocation for the name and one slice allocation for the attributes handed to trace.WithAttributes. They are now built once per (method, route) and reused, following the routeAttrs cache already used by metrics.go. Measured by a Tracer middleware benchmark under NeverSample: before 2008 B/op 18 allocs/op after 1864 B/op 16 allocs/op Cumulative across this stack: 2072 B / 19 allocs -> 1864 B / 16 allocs. Two safety properties, both tested: * The cache is keyed on the route TEMPLATE, so 500 concrete paths through /users/{id} produce one entry. An unmatched request carries a raw, attacker-controlled path and is deliberately never cached, which is what keeps the cache bounded by routes x methods. * The attributes are still passed to Start rather than applied after it. A sampler can read attributes when deciding, so applying them later would silently change sampling for anyone sampling on http.route. The cache is package level because the middleware chain is rebuilt around the matched handler on every request, so a closure-owned cache would be discarded each time. Its contents are pure data and provider-independent, so it stays correct across TracerProvider replacement. (cherry picked from commit c680c5544438476d60e9b44bc4d37678eecaf60e)

Caching the attribute slice still left two allocations per request: trace.WithAttributes builds an option wrapper around the slice, and passing it to Start builds the variadic []trace.SpanStartOption. Both are immutable once constructed and both are pure functions of (method, route), so they belong in the same per-route cache. Measured by a Tracer middleware benchmark under NeverSample: before 1864 B/op 16 allocs/op after 1824 B/op 14 allocs/op Cumulative across this file: 2072 B / 19 allocs -> 1824 B / 14 allocs. The attributes are still supplied at Start rather than applied afterwards, so sampler behaviour is unchanged for anyone sampling on http.route. (cherry picked from commit 397a7fb510a3f9ebe780cf5d8e3836045fd2d1f1)

…request The W3C propagators look their headers up by the lowercase names from the specs -- "traceparent", "tracestate", "baggage" -- and propagation.HeaderCarrier forwards those to http.Header.Get, which canonicalizes whatever key it is handed. None of those names is already canonical, so every request allocated the canonical string, whether or not the header was present at all. The canonical spellings are constants, so they are resolved once and the carrier reads the header map directly. A key the carrier does not recognize falls through to the ordinary lookup, so a propagator using its own header names -- B3, Jaeger, anything user-installed -- keeps working. TestHeaderCarrierMatchesHeaderCarrier pins that it resolves exactly what propagation.HeaderCarrier resolves, including for an unrecognized key and an absent one. TestTracerPropagatesIncomingTraceContext pins the behaviour that matters: an incoming traceparent is still adopted as the parent span. (cherry picked from commit 76a9accceb90c61d0c9d63f82ba3d839f2aaad1d)

The status wrapper existed solely to put http.response.status_code on the span, but it was created on every request regardless of whether any span would keep it. Two things made that worse than it looks. Tracer is the outermost middleware, so the StatusResponseWriter that Logging installs does not exist yet and the type assertion always failed -- meaning the fallback allocated a fresh wrapper every request rather than the "uncommon" case the old comment described. And GoFr installs an SDK provider with NeverSample when no TRACE_EXPORTER is configured, so on the default deployment nothing ever read it. The wrapper and its deferred attribute are now inside the IsRecording check, which is known immediately after Start. A recording span still gets the wrapper and the attribute, including the implicit-200 normalization, pinned by TestTracerStatusAttributeStillRecorded and TestTracerImplicit200StillRecorded. (cherry picked from commit 73b87213c044ce6889340df3a81e14865cc390ba)

The change caches the span name, attributes and start option per route so they are not rebuilt on every request. Nothing measured that, and one benchmark alone would have been misleading: a service that samples runs both a recording and a non-recording provider, and the middleware must not be quietly worse in either. Two benchmarks, so both paths are covered: BenchmarkTracer non-recording 21 -> 13 allocs/op, 2074 -> 1760 B/op BenchmarkTracer_Recording recording 23 -> 17 allocs/op, 2875 -> 2657 B/op Only allocation counts are quoted. Wall-clock on the machine these were taken on varied by more than 5x between runs of the same binary, so ns/op there says nothing; the allocation counts were stable and moved monotonically with each commit in the series. Also repairs the comment on the cache, which had been scrambled so that a nolint directive split the sentence explaining why the cache is package level.

buildSpanName was inserted directly above methodKV, so methodKV's entire doc -- the semconv rationale and the escape-analysis note -- ended up documenting buildSpanName instead, and methodKV was left with none. Moves buildSpanName below methodKV. No code change.

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.

… 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.

…an design Rebasing onto #3770 replays this branch's tracer.go and tracer_test.go over #3770's, which drops #3770's twelve tracer characterization tests wholesale. All twelve are restored here unchanged - they exercise the middleware through its exported surface and are indifferent to whether the span metadata is cached. Test_TracerContract_ReusesStatusResponseWriter is the one that matters most: it pins the StatusResponseWriter block this branch wraps in the IsRecording gate, which is the overlap the review flagged as the place a careless merge resolution would silently drop the gate. It passes against the gated code, and the gate is verified present.

…d status RecordHistogramAttrs called metric.WithAttributes on every observation, which builds an attribute.Set -- sorting and deduplicating the attributes -- and an option wrapper. For a request metric the attribute combinations come from a small fixed set, so that work produced the same option over and over. RecordHistogramOpt accepts an already-built option, and the middleware caches one per (path, method, status). The cache carries a ceiling: the path label falls back to the raw request path when no route template is available, and a raw path is caller-controlled, so past the limit the option is built per request exactly as before. The recording strategy is now selected once, at middleware construction, from what the backend supports -- prebuilt option, attribute slice, or plain labels -- and each strategy is its own type. The request path makes a single call and carries none of that branching, which also clears the cyclomatic-complexity warning Metrics carried before this change. RecordHistogramOpt is additive: RecordHistogramAttrs and RecordHistogram are unchanged, and an external implementation providing neither is served by the same varargs path it used before. The status attribute remains a string, so OTLP queries expecting a string label keep working across all three paths. (cherry picked from commit e76395aa4eb1ec6c1b8d15b3c4bd61841a5ce8da)

RecordHistogramOpt took a single measurement option and passed it to histogram.Record, whose options parameter is variadic. Handing a lone option to a variadic allocates a fresh one-element slice on every observation, which made this the largest single allocation in the request path once the option itself was cached. The option slice is now cached alongside the rest of the per-(path, method, status) data and passed through with opts..., which reuses the existing slice rather than building one. The signature takes metric.RecordOption rather than MeasurementOption for the same reason: it is what histogram.Record accepts, so nothing has to be converted or re-wrapped at the call site. (cherry picked from commit ebd679a2530f3f9bb78764d2c121cb3257827abc)

The change adds an option-based recorder so a caller whose label combinations come from a small fixed set can build the measurement once and reuse it. Nothing measured what that saves. Two benchmarks, both shipped here, so the before and after are visible side by side rather than needing a checkout of the previous revision: RecordHistogramAttrs 4 allocs/op 424 B/op ~928 ns/op RecordHistogramOpt 0 allocs/op 0 B/op ~164 ns/op The cost being removed is metric.WithAttributes on every observation, which sorts and deduplicates the attributes into a new attribute.Set and wraps it -- work that produces an identical option every time for a request metric keyed by route, method and status.

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.

…ption design Rebasing onto the #3770 stack replays this branch's metrics_test.go over #3770's, dropping its twenty metrics characterization tests. All twenty are restored here unchanged. They are worth more on this branch than they were on #3770. The path label resolution they pin - template vs raw path, static extensions forcing the raw path, the /static prefix, trailing-slash trimming, the GraphQL skip - is exactly the logic this branch refactored into metricsPath in order to report whether the label is a bounded route template. They pass against the refactor, which is independent evidence that threading `templated` out did not disturb what gets labelled. Test_MetricsContractFastAndSlowPathsAgree also pins the two recorder strategies emitting identical labels, which is the property this branch's new optionRecorder assertions depend on.

…locs

…trics-allocs

…llocs # Conflicts: # pkg/gofr/http/middleware/routecache.go # pkg/gofr/http/middleware/tracer.go # pkg/gofr/http/middleware/tracer_test.go

The cached-option branch discarded the type assertion's ok: opts, _ := v.([]metric.RecordOption) o.rec.RecordHistogramOpt(ctx, histogramName, seconds, opts...) A failed assertion leaves opts nil, and recording with no options emits the measurement with NO attributes -- an unlabelled time series that looks like a working metric and silently corrupts whatever queries it. That is worse than the nil dereference the same shape produced in the tracer cache, because nothing fails loudly. Nothing else stores into this cache, so it cannot fail today. It now falls through to the rebuild the miss path already performs, matching what spanMetaFor does since #3970. attrsRecorder already handled this correctly -- its assertion is followed by an `if b == nil` rebuild -- so only the option path changed. No behaviour or allocation change: BenchmarkRecordHistogramOpt stays at 0 B/op, 0 allocs/op. pkg/gofr/http/middleware and pkg/gofr/metrics pass under -race.

aryanmehrotra · 2026-09-01T07:35:18Z

Up to date, and a self-review

Merged development after #3970 landed. That merge conflicted in three files, and the resolution is worth stating because a careless one would have been silent: this branch carried an older snapshot of tracer.go / tracer_test.go from when it was stacked, so keeping "ours" would have reverted three fixes that went into #3970 after the stack was cut. Development's versions were taken for both, and routecache.go was rebuilt from development's copy with this PR's own change — generalising the cache key from routeKey to any — re-applied on top. Verified afterwards that the dead spanMeta.attrs stayed removed, and that the nil-assertion fix and the sampling comment survived.

0 behind development, CI green.

Requested changes, and where they landed

Request	Where
Medium — the `optionRecorder` fast path has no label assertions	`TestMetricsOptRecorderLabels` asserts the exact map: `path=STRING:/users/{id}` (template, not `/users/7`), `method=STRING:GET`, `status=STRING:201` — including that status is a String, so the two recorders agree for OTLP
Bound test asserted only a compile-time constant	`TestMetricsOptCacheIsBounded` drives `routeCacheLimit + 1000` distinct keys through `record()` and asserts `cache.len() == routeCacheLimit`
Catch-all raw-path traffic can starve real routes out of the cache	`cacheable := templated && cacheableMethod(method)` on both recorders — an untemplated path never enters the cache, so it cannot consume the budget. `TestMetricsCachesRejectUntemplatedPaths`
`attrsRecorder` untested, and its cache unbounded	now on the same `routeCache` under the same rules; `TestAttrsRecorderIsCorrectAndBounded`
"−650 B/request" not reproducible	description revised; the numbers below are the shipped ones
⚠️ Cross-PR — bound all three caches consistently	done by construction: `routeCache` is one type with one ceiling, shared by the tracer cache and both metrics recorders. That is what the `any` key exists for

The numbers, re-run just now

-benchtime 20000x -count 4:

benchmark	B/op	allocs/op
`BenchmarkRecordHistogramAttrs` (rebuilds each time)	424	4
`BenchmarkRecordHistogramOpt` (cached option)	0	0
`BenchmarkAttrBuild_HTTP` (the attribute build a hit also avoids)	448	3

Matches the description exactly. One correction: the description's ~928 ns/op for the attrs path measures ~280 ns/op here. ns/op is machine noise and the description already says the allocation counts are the reliable signal, but the figure should not be read as reproducible.

What I found reviewing my own diff

Fixed in 4af7ab8e. The cached-option branch discarded the assertion's ok:

opts, _ := v.([]metric.RecordOption)
o.rec.RecordHistogramOpt(ctx, histogramName, seconds, opts...)

A failed assertion leaves opts nil, and recording with no options emits the measurement with no attributes — an unlabelled time series that looks like a working metric while corrupting any query over it. Worse than the nil dereference the same shape caused in the tracer cache, because nothing fails loudly. It now falls through to the rebuild, matching spanMetaFor. attrsRecorder was already correct: its assertion is followed by an if b == nil rebuild.

Observation, not a change. newStatusAttrCache is an unbounded sync.Map keyed on the status code. Unlike path and method, the status is chosen by the application rather than the caller, so it is bounded by the app's own code paths — but it is the one cache here without a ceiling, and worth knowing if a handler ever writes a caller-supplied status.

Out of scope, pre-existing. Both recorders pass context.Background() rather than the request context, so a recorded measurement carries no exemplar linking it to the trace. That is unchanged from development (it does the same in both places) and is not something this PR should alter, but it is the reason metrics here cannot be correlated to a span.

Why this matters beyond the allocation win

Measured against development today, as a running service:

metric series before:                     21
after 5,000 requests with distinct methods: 42,000
app_http_response_count{method="M000000",path="/totally/unknown",status="404"} 1

An unauthenticated client inflates metric cardinality by varying the HTTP method alone. #3970 closed the tracer half; this is the metrics half, and it is live on development until this lands.

aryanmehrotra · 2026-09-01T07:42:02Z

Ran it as a service — and I need to correct something I wrote above

I said this PR "closes the metrics-side cardinality hole". That is wrong, and running it is what showed it. Two binaries, development and this branch, same app, 5,000 and 20,000 requests each carrying a distinct RFC-7230 method token.

Metric cardinality is unchanged, and was never GoFr's to bound

	development	this branch
`app_http_*` series after 5,000 distinct methods	21 → 42,000	21 → 42,000
distinct `method` label values exported	1,998	1,998
`otel_metric_overflow` markers	21	21

Identical. The cap at ~2,000 is the OTel SDK's own default cardinality limit, spilling the rest into the overflow bucket — it is doing that on development today and it does the same here. The caches this PR bounds are GoFr's internal maps; they never controlled how many series the SDK creates, because the attributes still carry the caller's method either way.

So the attacker-influenced cardinality question is still open, on both, and it is a separate change: it needs the label itself constrained (an allow-list of methods, or bucketing unknown ones), not a cache ceiling. Worth its own issue rather than being folded in here.

What the bound does buy, measured

requests with distinct methods	development	this branch
5,000	+31.4 MB RSS	+25.4 MB
20,000	+35.5 MB RSS	+26.4 MB

About 9 MB less resident, and flatter as the flood grows — that is the cache ceiling doing its job. The remainder is the SDK's own series, which neither branch changes.

Behaviour is identical on real traffic

Same app, same requests, exported series compared byte for byte:

app_http_response_count{method="GET",  path="/users/{id}", status="200"} 2
app_http_response_count{method="POST", path="/users",      status="201"} 1
app_http_response_count{method="GET",  path="/.well-known/alive", status="200"} 1

Identical on both. Two different concrete paths (/users/7, /users/99) collapse to the one /users/{id} template, the method is the raw request method, and the status is a string — the exact contract TestMetricsOptRecorderLabels pins.

Summary of what this PR actually delivers

the allocation win on the record path: 424 B → 0 B, 4 allocs → 0
a ceiling on both metrics caches, shared with the tracer's: ~9 MB less resident under a hostile method stream, where before attrsRecorder had no ceiling at all
no change to metric names, labels, types or values

It does not bound metric cardinality, and the description should not be read as claiming it does.

aryanmehrotra · 2026-09-01T07:44:52Z

Retracting the cardinality framing entirely

I called the metric cardinality an open hole needing its own fix. It is not — it is a configured SDK limit behaving as designed, and I should have checked before writing that.

sdk/metric@v1.45.0/config.go sets defaultCardinalityLimit = 2000 and exposes it through WithCardinalityLimit and OTEL_GO_X_CARDINALITY_LIMIT. It tracks the setting exactly — 4,000 requests with distinct method tokens:

`OTEL_GO_X_CARDINALITY_LIMIT`	distinct `method` labels exported
unset (default 2000)	1,998
100	98
500	498

So the ~2,000 ceiling and the otel_metric_overflow bucket are the SDK doing its job, tunable per deployment. There is nothing for GoFr to fix and no issue worth filing. Sorry for the noise in the two comments above.

What this PR does, stated plainly

424 B → 0 B, 4 allocs → 0 on the record path
a ceiling on both metrics caches, shared with the tracer's, where attrsRecorder previously had none: ~9 MB less resident under a hostile method stream, and flatter as it grows
no change to metric names, labels, types or values — verified by running two binaries side by side and comparing the exported series

That is the whole claim, and it holds.

The comment added in 4af7ab8 used en-GB spellings, which both spelling gates reject: typos: `unlabelled` should be `unlabeled` metrics.go:207 misspell: `honoured` is a misspelling of `honored` metrics.go:205 Comment only; no code change.

Umang01-hash

LGTM. Ran it locally at this head — booted examples/http-server and scraped /metrics: app_http_response emits correctly through the real metricsManager→optionRecorder path (path/method/status all String). Benchmarks confirm the win: Opt = 0 allocs/110ns vs Attrs = 4 allocs/424B/284ns. gofmt/vet/build/-race tests/golangci-lint --new-from-rev all clean; 100% cover on every new func. Nice catch bounding the previously-unbounded attrs cache under routeCache + cacheableMethod + the templated gate — that closes a real memory-growth vector, not just a perf tidy.

All comments below are nits, nothing blocking.

Umang01-hash · 2026-09-01T09:41:00Z

+	o.rec.RecordHistogramOpt(context.Background(), histogramName, seconds, opts...)
+}
+
+// attrsRecorder caches the (path, method) attribute pair and copies it into a


nit: attrsRecorder is unreachable in prod — *metricsManager implements RecordHistogramOpt and newHistogramRecorder checks metricsOptRecorder first, so optionRecorder always wins. The tier only ever runs from its own test. Fine to keep as a migration path for external Attrs-only Managers, but if that's not a supported surface this could collapse to option+labels.

Umang01-hash · 2026-09-01T09:41:00Z

+		// silently unlabeled time series rather than a visible failure. Falling
+		// through rebuilds instead.
+		if v, ok := o.cache.load(key); ok {
+			if opts, ok := v.([]metric.RecordOption); ok {


nit: this inner type-assertion can't fail — the cache is single-consumer and only ever holds []metric.RecordOption, which is also why the fall-through has no coverage. opts := v.([]metric.RecordOption) would read as honestly single-typed.

Umang01-hash · 2026-09-01T09:41:00Z

+// its ceiling, after which every first-seen LEGITIMATE route could never be
+// stored and rebuilt its measurement option per request forever. Memory stayed
+// bounded; the optimization silently reverted to baseline for real traffic.
+func metricsPath(r *http.Request) (path string, templated bool) {


nit: the new templated return is only pinned indirectly via the cache-growth tests; a direct metricsPath() unit test over static-ext / "/" / /static / unmatched would assert it cleanly. Non-blocking.

aryanmehrotra force-pushed the perf/metrics-allocs branch from ecf0dc7 to 7a41fd9 Compare August 18, 2026 10:07

PiyushSingh-ZS mentioned this pull request Aug 18, 2026

fix(http): characterize the request path and fix eleven defects it exposed #3770

Merged

NitinKumar004 mentioned this pull request Aug 18, 2026

perf(tracer): stop rebuilding span metadata on every request #3970

Merged

NitinKumar004 suggested changes Aug 18, 2026

View reviewed changes

aryanmehrotra changed the base branch from development to perf/tracer-allocs August 20, 2026 06:54

aryanmehrotra force-pushed the perf/tracer-allocs branch from 7c3d4ab to 4eed1cc Compare August 20, 2026 07:03

aryanmehrotra force-pushed the perf/metrics-allocs branch from 7a41fd9 to cd1bf5d Compare August 20, 2026 07:03

aryanmehrotra requested a review from NitinKumar004 August 20, 2026 10:04

aryanmehrotra force-pushed the perf/metrics-allocs branch from cd1bf5d to 3434a4b Compare August 21, 2026 07:09

aryanmehrotra force-pushed the perf/tracer-allocs branch from f4512ad to a09f4f6 Compare August 21, 2026 07:47

aryanmehrotra force-pushed the perf/metrics-allocs branch from 3434a4b to d648833 Compare August 21, 2026 07:49

aryanmehrotra force-pushed the perf/tracer-allocs branch from a09f4f6 to 9e20256 Compare August 27, 2026 05:59

aryanmehrotra force-pushed the perf/metrics-allocs branch from d648833 to ac9f800 Compare August 27, 2026 06:00

aryanmehrotra force-pushed the perf/tracer-allocs branch from 9e20256 to 16ee698 Compare August 27, 2026 06:28

aryanmehrotra force-pushed the perf/metrics-allocs branch from ac9f800 to 5d30c46 Compare August 27, 2026 06:28

aryanmehrotra requested review from NitinKumar004 and removed request for NitinKumar004 August 27, 2026 09:01

aryanmehrotra added 2 commits August 28, 2026 12:40

aryanmehrotra added 14 commits August 28, 2026 12:40

aryanmehrotra force-pushed the perf/tracer-allocs branch from 16ee698 to 9aee0c9 Compare August 28, 2026 07:18

aryanmehrotra force-pushed the perf/metrics-allocs branch from 5d30c46 to 589fd80 Compare August 28, 2026 07:21

aryanmehrotra added 2 commits August 31, 2026 18:18

Merge remote-tracking branch 'origin/development' into perf/tracer-al…

f4bae8f

…locs

Merge remote-tracking branch 'origin/perf/tracer-allocs' into perf/me…

e40f35c

…trics-allocs

Base automatically changed from perf/tracer-allocs to development September 1, 2026 07:11

An error occurred while trying to automatically change base from perf/tracer-allocs to development September 1, 2026 07:11

aryanmehrotra added 2 commits September 1, 2026 12:43

Merge remote-tracking branch 'origin/development' into perf/metrics-a…

31076b9

…llocs # Conflicts: # pkg/gofr/http/middleware/routecache.go # pkg/gofr/http/middleware/tracer.go # pkg/gofr/http/middleware/tracer_test.go

Umang01-hash approved these changes Sep 1, 2026

View reviewed changes

Navigation Menu

Uh oh!

perf(metrics): build the measurement option once per route, method and status - #3972

perf(metrics): build the measurement option once per route, method and status#3972
aryanmehrotra wants to merge 21 commits into
developmentfrom
perf/metrics-allocs

aryanmehrotra commented Aug 18, 2026 •

edited

Loading

Uh oh!

PiyushSingh-ZS commented Aug 18, 2026

Uh oh!

NitinKumar004 left a comment

Uh oh!

aryanmehrotra commented Aug 27, 2026

Uh oh!

aryanmehrotra commented Sep 1, 2026

Uh oh!

aryanmehrotra commented Sep 1, 2026

Uh oh!

aryanmehrotra commented Sep 1, 2026

Uh oh!

Umang01-hash left a comment

Uh oh!

Umang01-hash Sep 1, 2026

Uh oh!

Umang01-hash Sep 1, 2026

Uh oh!

Umang01-hash Sep 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

aryanmehrotra commented Aug 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The problem

The fix

The numbers

Safety

Test coverage added after review

Measurement

Uh oh!

PiyushSingh-ZS commented Aug 18, 2026

attrsRecorder's cache has no equivalent of optCacheLimit

Related, and probably a separate issue

Nit

Uh oh!

NitinKumar004 left a comment

Choose a reason for hiding this comment

Medium

Low

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

Uh oh!

aryanmehrotra commented Aug 27, 2026

The fast path now has label assertions

Catch-all traffic can no longer starve real routes

TestMetricsOptCacheIsBounded drives the guard

The −650 B/request claim is retracted

Coordinated with #3970, as you asked

No behaviour change

Uh oh!

aryanmehrotra commented Sep 1, 2026

Up to date, and a self-review

Requested changes, and where they landed

The numbers, re-run just now

What I found reviewing my own diff

Why this matters beyond the allocation win

Uh oh!

aryanmehrotra commented Sep 1, 2026

Ran it as a service — and I need to correct something I wrote above

Metric cardinality is unchanged, and was never GoFr's to bound

What the bound does buy, measured

Behaviour is identical on real traffic

Summary of what this PR actually delivers

Uh oh!

aryanmehrotra commented Sep 1, 2026

Retracting the cardinality framing entirely

What this PR does, stated plainly

Uh oh!

Umang01-hash left a comment

Choose a reason for hiding this comment

Uh oh!

Umang01-hash Sep 1, 2026

Choose a reason for hiding this comment

Uh oh!

Umang01-hash Sep 1, 2026

Choose a reason for hiding this comment

Uh oh!

Umang01-hash Sep 1, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

aryanmehrotra commented Aug 18, 2026 •

edited

Loading

`attrsRecorder`'s cache has no equivalent of `optCacheLimit`

`TestMetricsOptCacheIsBounded` drives the guard

The `−650 B/request` claim is retracted