Repository navigation
Fix histogram bucket consistency during concurrent collection - #179
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new histogram layout hashing uses table.concat(metric.buckets, ",") on numeric bucket values, which will throw at runtime and break histogram registration.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR revises histogram storage/collection to avoid inconsistent cumulative bucket exports under concurrent worker flush + collection interleavings by storing disjoint per-range counters and deriving cumulative buckets during collection, with accompanying documentation and test coverage updates.
Changes:
- Switch histogram observations to increment exactly one disjoint bucket counter plus
_sum, then build cumulative buckets (and_count) from a single read pass during collection. - Add a separate histogram key index / namespace to isolate the new format (and bucket layouts) from legacy cumulative histogram cells.
- Update Lua unit tests and documentation/changelog to reflect the new behavior (including exporting zero-valued buckets).
File summaries
| File | Description |
|---|---|
| README.md | Updates histogram concurrency/consistency documentation. |
| prometheus.lua | Implements disjoint histogram storage, new histogram key index, and cumulative reconstruction at collection time. |
| prometheus_test.lua | Refactors histogram assertions to read public exposition and adds deterministic concurrency/layout isolation regressions. |
| CHANGELOG.md | Documents the new histogram storage model and upgrade/reload implications. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
knyar
left a comment
knyar
left a comment
There was a problem hiding this comment.
Thank you, this looks good. Improving consistency/correctness is great, but this also allows us to clean up some of the older hacks added for histogram support.
I left a few minor comments but I am also happy to merge this as is and fix those minor issues myself as a follow up - please let me know if you prefer that.
| Each observation increments one disjoint bucket counter and the sum. Collection | ||
| reads each bucket counter once and derives the cumulative buckets and `_count` | ||
| from those reads. Concurrent worker flushes cannot create observations in an | ||
| empty range, and the `+Inf` bucket always equals `_count`. | ||
|
|
||
| Observations remain eventually consistent across workers. The bucket reads and | ||
| `_sum` do not form a single atomic snapshot, so an observation may become visible | ||
| in the sum and count in different scrapes. |
There was a problem hiding this comment.
This is user-facing documentation for histogram:observe(), so I don't think we need to describe implementation details (disjoint counters) here. How about a shorter note documenting the impact for the user? E.g. "Note that bucket counters and the _sum counter are incremented independently, so an observation may become visible in the sum and count in different scrapes."
There was a problem hiding this comment.
| luaunit.assertEquals(self.dict:get("metric1"), nil) | ||
| luaunit.assertEquals(self.dict:get("l1_count"), nil) | ||
| luaunit.assertEquals(sample(self.p, "l1_count"), nil) |
There was a problem hiding this comment.
Any thoughts on replacing other metric reads that use self.dict:get with the new sample() helper? That would make the tests more readable, making it explicit whether we're testing metric output vs internal state (self.dict:get will need to stay for checking internal keys)
There was a problem hiding this comment.
Yep, metric-output checks now use sample(). Kept direct reads for internal keys and injected-failure checks, where sample() would trigger another scrape.
| if metric.help then | ||
| table_insert_tail(output, string.format("# HELP %s%s %s\n", | ||
| self.prefix, metric.name, metric.help)) | ||
| end | ||
| table_insert_tail(output, string.format("# TYPE %s%s histogram\n", | ||
| self.prefix, metric.name)) | ||
| seen_metrics[metric.name] = true |
There was a problem hiding this comment.
This is almost identical to the existing comment output code in metric_data. Maybe move it to a method on metric so that this becomes just:
if not seen_metrics[metric.name] then
metric:write_comments(output)
seen_metrics[metric.name] = true
end
There was a problem hiding this comment.
Done: shared method, histogram caller, and scalar caller. seen_metrics stays in the callers.
| @@ -743,7 +701,10 @@ function Prometheus.init(dict_name, options_or_prefix) | |||
| if m and m.typ == TYPE_HISTOGRAM then | |||
There was a problem hiding this comment.
I think we no longer need special handling of histogram keys here, or do we?
There was a problem hiding this comment.
Yep, no longer needed. Reset now uses the base metric name directly; the _sum special case is gone.
There was a problem hiding this comment.
We could keep sorting keys just for aesthetical reasons, but this comment is no longer accurate.
There was a problem hiding this comment.
| -- Ignore cumulative histogram keys left in the old index after a reload. | ||
| local name = key:match("^[^{]+") | ||
| local metric = self.registry[name] or self.registry[ | ||
| name:match("^(.*)_bucket$") or name:match("^(.*)_count$") or | ||
| name:match("^(.*)_sum$")] | ||
| local value, err | ||
| if not metric or metric.typ ~= TYPE_HISTOGRAM then | ||
| value, err = self.dict:get(key) | ||
| end |
There was a problem hiding this comment.
Sould we make a helper for this to make it a bit more readable? E.g.
if not should_ignore_key(key, self.registry) then
value, err = self.dict:get(key)
end
There was a problem hiding this comment.
Done: should_ignore_key helper and the caller.
| value, err = self.dict:get(key) | ||
| end | ||
| if value then | ||
| local short_name = short_metric_name(key) |
There was a problem hiding this comment.
We no longer need short_metric_name, I believe - or maybe it could just return key:match("^[^{]+")?
There was a problem hiding this comment.
Yep, removed short_metric_name. The caller now uses key:match("^[^{]+") directly.
| local function reset_lookup(metric_key) | ||
| -- When another worker calls reset or del on a metric, reset that | ||
| -- metric's local lookup table. | ||
| local metric_name = ngx_re_gsub(metric_key, "{.*", "", "jo") |
There was a problem hiding this comment.
Should we replace this gsub with your new key:match("^[^{]+") approach?
There was a problem hiding this comment.
| @@ -872,6 +834,14 @@ local function register(self, name, help, label_names, buckets, typ) | |||
| metric.buckets = buckets or DEFAULT_BUCKETS | |||
| metric.bucket_count = #metric.buckets | |||
| metric.bucket_format = construct_bucket_format(metric.buckets) | |||
There was a problem hiding this comment.
We no longer need construct_bucket_format, fix_histogram_bucket_labels and METRICS_KEY_REGEX I believe.
There was a problem hiding this comment.
Yep, removed all three in d516af0. Bounds now use tostring directly, with numeric validation preserved.
Co-authored-by: Anton Tolchanov <1687799+knyar@users.noreply.github.com>
|
Feel free to clean it up more as you wish before merging. |
Quick follow-up to the recent histogram changes in #179.
Quick follow-up to the recent histogram changes in #179.
|
I just created a release 0.20260912 which includes this change. Thank you for your contribution @creationix! |
Concurrent histogram updates and collection can report observations in ranges
that have never received a value. Store each observation in one disjoint range
and construct the cumulative buckets from those range counters during collection.
This also makes
+Infand_countidentical in every exported label set.Related to #161, which partially mitigated the race by changing increment order.
The per-worker counter buffer is flushed with
pairs(), and collection can alsooverlap an otherwise complete flush, so increment order alone cannot prevent it.
Concrete failure
Suppose a histogram has bounds
{1, 10, 100}, and all ten observations are0.5:le="1"le="10"le="100"le="+Inf"The collector reads
le="1"before another worker flushes one more0.5observation, then reads the other buckets afterward. The middle scrape claims
one observation in
(1, 10], although every observation was below 1. Otherinterleavings can expose decreasing cumulative buckets or disagreeing
+Infand
_countvalues.Consumers that convert cumulative buckets into per-range observations can turn
this discrepancy into phantom slow samples, inflating tail percentiles and
histogram-derived maxima. A later consistent scrape does not necessarily undo
observations already ingested into a downstream distribution.
Changes
_sumper observation. Collection readseach bucket once and builds cumulative counts from those same reads.
including zeros, so a newly observed label set has a complete schema.
bucket layouts. Serialize boundaries with 17 significant digits so distinct
floating-point layouts cannot share counters. Preserve escaped labels, output
prefixes, and reset behavior.
generation. Bucket labels retain sub-microsecond and longer decimal bounds
instead of rounding them to six decimal places.
existing Lua unit-test harness. Public metric assertions read exported samples.
This provides bucket/count consistency, not a globally atomic snapshot:
observations remain eventually consistent, and
_sumis sampled independently.Histogram history is not migrated from the old storage format. Upgrading or
changing bucket boundaries requires a full nginx restart or fresh shared
dictionary; graceful reloads can retain old cells and overlap incompatible worker
generations. The changelog documents this upgrade requirement. Counter and gauge
storage remains unchanged.
Validation
luacheckcommands from the existing CI workflow pass.
97.05% comparison baseline), with 95.12% coverage of
prometheus.lua.Tests exercise every newly added runtime line, including histogram read/reset
errors and timer-driven worker index synchronization.
expected zero observations in
(1, 10], got one. The zero-bucket regressionalso fails upstream because the empty lower bucket is absent.
{0.3, 1}and{0.1 + 0.2, 1},whose distinct boundaries both become
0.3with default number formatting.the previous implementation and pass with the cleanup.
and concurrent reset checks.