Skip to content

counter: cache entries for saved positional-label children - #865

Open
SidheshwarSarangal wants to merge 2 commits into
prometheus:mainfrom
SidheshwarSarangal:contributor/cached-counter-child
Open

SidheshwarSarangal wants to merge 2 commits into
prometheus:mainfrom
SidheshwarSarangal:contributor/cached-counter-child

Conversation

@SidheshwarSarangal

Copy link
Copy Markdown

Saved counter children currently validate labels and rebuild the storage key on every increment. Cache the entry for positional string/number labels, invalidating it after reset() or remove().

Object-form labels and exemplar-enabled counters retain their existing paths. Add lifecycle tests and comparison benchmarks.

Relates to #863.

Validation

  • npm test -- --runInBand: 31 suites, 671 tests, and 34 snapshots passed.
  • Node.js v24.22.0, Linux x64, Intel Core 5 210H.
  • Nine alternating benchmark rounds, 1,000,000 increments per round:
Labels Saved child before After Speedup
0 35.11 ns/op 12.06 ns/op 2.91×
1 85.38 ns/op 12.06 ns/op 7.08×
3 312.62 ns/op 11.94 ns/op 26.18×

Direct increments varied by approximately 2% in this run.

AI assistance was used to prepare the implementation, tests, and benchmark helper.

@jdmarshall, could you review whether this scope fits the direction of #863?

Invalidate cached entries after reset or removal. Preserve object-label and exemplar behavior, and add lifecycle tests and benchmarks.

Relates to prometheus#863

Signed-off-by: Sidheshwar Sarangal <sidheshwar.sarangal@gmail.com>
@jdmarshall

jdmarshall commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

While I appreciate the effort, there are several things that make this a “not yet” situation.

From a correctness standpoint there’s a bug where it is missing a bounds check that is in the original code, so suggests that perhaps the three implementations of inc require a rework to eliminate common code. There were two before, your change makes it three and now The Rule of Three should apply.

Second, while I am sure that counter is the low hanging fruit, every time we make one metric behave differently from the others when a different implementation could answer for all four metrics or at least Gauge, Counter and Histogram, that creates a situation where users have to reason separately about each metric type and that’s work we should take on rather than externalizing on the users. At any rate it only invites further feature requests to achieve parity

Lastly, this muddies the waters for #812. #863 should either be a follow-up to #812 or perhaps implemented as a target of opportunity while pursuing #812.

@jdmarshall

jdmarshall commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

There is also a middle ground here where we cache the lookup key instead of the store, which would be particularly appropriate for the labels function.

That avoids the need for the sanity check for wellformed labels and the version number. It would take us a bit back to the tricks of the previous implementation, most of which I removed because this implementation was simpler and faster even without preserving every trick in the original code. This scenario is tightly contained to a single logic path, one that I presume we hope people use in attempt to improve both correctness and performance.

Replace cached entries and version tracking with precomputed lookup keys. Share value validation between direct increments and saved children.

Relates to prometheus#863

Signed-off-by: Sidheshwar Sarangal <sidheshwar.sarangal@gmail.com>
@SidheshwarSarangal

Copy link
Copy Markdown
Author

@jdmarshall I revised this to cache the lookup key instead of the entry, removed version tracking, and extracted shared increment-value validation.

All 691 tests, lint, formatting, and TypeScript checks pass. The revised benchmark shows saved-child speedups of 1.79×, 4.38×, and 16.23× for 0, 1, and 3 labels, with no direct-increment slowdown observed in that run.

Would this narrower approach fit alongside #812?

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants