Skip to content

perf(counter): reuse pending increment entries - #28

Merged
AlinsRan merged 1 commit into
mainfrom
perf/counter-in-place
Nov 3, 2026
Merged

AlinsRan merged 1 commit into
mainfrom
perf/counter-in-place

Conversation

@AlinsRan

@AlinsRan AlinsRan commented Nov 3, 2026 •

Copy link
Copy Markdown

Repeated incr() calls currently allocate a replacement {v, t} table even when the metric already has a pending increment. Histogram observations multiply this allocation across the count, sum and buckets.

Update the existing worker-local entry in place until sync() clears the buffer. The latest call still supplies the expiry, including nil and 0; neither increment nor synchronization yields.

Added tests cover expiry replacement, preserving an existing shared-dictionary TTL when the next batch omits it, default/zero/fractional increments, and counter/histogram values across synchronization cycles.

Validation:

  • Lua 5.2: all 93 unit tests passed.
  • Both CI luacheck commands passed.
  • Existing four-worker integration test: 47,596 requests with nine concurrent clients; all assertions passed.

A warmed LuaJIT microbenchmark on CPU 0, using 64 rotating keys and seven interleaved rounds of five million increments, measured:

Median time per increment Allocation per increment
main (a4217eb) 30.5 ns 112 B
This change 2.4 ns 0 B

Timing includes normal GC. Allocation was measured separately with GC stopped over 100,000 increments after warmup; accumulated values were checked. This measures the buffered increment hot path, not end-to-end throughput.

Summary by CodeRabbit

  • Bug Fixes
    • Buffered counter increments now update existing cached values and expiry information correctly. When no expiry is specified, the existing shared-dictionary TTL is preserved; an explicit zero expiry clears it.
    • Counter and histogram totals remain consistent across repeated syncs, with subsequent observations reflected in stored totals and buckets.

@coderabbitai

coderabbitai Bot commented Nov 3, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review.

Walkthrough

The counter increment method now updates an existing cached entry’s value and expiry in place. Two tests cover buffered counter expiry and counter and histogram accumulation across syncs.

Changes: Buffered counter updates

Layer / File(s) Summary
Counter cache update and sync validation
prometheus_resty_counter.lua, prometheus_test.lua
Existing cached counter entries are updated in place. Tests check how nil, positive, and zero expiry values affect the shared-dictionary TTL. They also check that repeated syncs preserve counter totals and histogram buckets without duplicating observations.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: juststillthinking

Merge Risk: Minimal · up to 2f894

No actionable merge-blocking risk was identified.

Pre-merge checks: Passed 6
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title "perf(counter): reuse pending increment entries" matches the main change. The raw summary and PR objectives say _M:incr now updates existing pending counter entries in place instead of all…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
E2e Test Quality Review Passed The PR reuses the existing pending entry in place and preserves the latest expiry, including nil and 0; Lua treats numeric 0 as truthy, so sync correctly calls expire(k, 0). The added tests …
Security Check Passed No security issue was introduced. The diff only changes worker-local counter aggregation in prometheus_resty_counter.lua:96-105; it updates numeric value and expiry in place, or creates the same pen…
Full details: Title check

Explanation

The title "perf(counter): reuse pending increment entries" matches the main change. The raw summary and PR objectives say _M:incr now updates existing pending counter entries in place instead of allocating a new {v, t} table for each repeated increment. The title names the performance goal and the scope (counter) accurately. It is concise, single-sentence, and specific enough for a teammate scanning history. The title does not mention the added tests, but that detail is not required.

Full details: E2e Test Quality Review

Explanation

The PR reuses the existing pending entry in place and preserves the latest expiry, including nil and 0; Lua treats numeric 0 as truthy, so sync correctly calls expire(k, 0). The added tests cover accumulation, repeated syncs, histogram buckets, nil/zero/fractional expiry cases, and readable assertions. Existing integration tests exercise the public API through nginx and the real shared dictionary, including counters and histograms. No changed-code error-handling, concurrency, scope, or hidden-order issue is evident.

Full details: Security Check

Explanation

No security issue was introduced. The diff only changes worker-local counter aggregation in prometheus_resty_counter.lua:96-105; it updates numeric value and expiry in place, or creates the same pending entry. The call path passes metric keys, numeric values, and expiry only (prometheus.lua:515-524, 606-635). No issues found in Category 1 (sensitive exposure), 2 (unencrypted secrets), 3 (authorization), 4 (ownership), 5 (TLS), 6 (shared-resource isolation), or 7 (secret reference resolution). Existing error logging is unchanged.

Finishing Touches
Generate docstrings
  • Commit to this branch
  • Create a new PR
Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

CodeRabbit Verify

  • Run CodeRabbit Verify to test the changes in this pull request.
Recent review info
Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Essentials
  • Run ID: 04605f1e-f0fb-412a-b921-23563bfad4f7
Commits

Reviewing files that changed from the base of the PR and between a4217eb and 2f89412.

Files selected for processing: 2
  • prometheus_resty_counter.lua
  • prometheus_test.lua

Included review availability: This review used your included allowance. 0 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour.

@AlinsRan
AlinsRan merged commit 869e6e0 into main Nov 3, 2026
5 checks passed
@AlinsRan
AlinsRan deleted the perf/counter-in-place branch November 3, 2026 06:08
@AlinsRan AlinsRan self-assigned this Nov 3, 2026
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