Bound distribution sample buffer with automatic aggregation (fixes #41, #52) - #77
Open
robacourt wants to merge 1 commit into
Open
Bound distribution sample buffer with automatic aggregation (fixes #41, #52)#77robacourt wants to merge 1 commit into
robacourt wants to merge 1 commit into
Conversation
Distribution (histogram) samples are buffered in a :duplicate_bag and only folded into histogram buckets when Aggregator.aggregate/3 runs, which until now happened solely at scrape time. A reporter whose /metrics endpoint is scraped infrequently -- or never -- therefore accumulates one ETS row per observation without bound (issues beam-telemetry#41 and beam-telemetry#52). The registry now aggregates on its own in addition to on scrape: * size trigger: a lock-free counter in the distribution event handlers enqueues a single flush once :flush_threshold samples have buffered (default 10_000), de-bounced via an atomics flag so a burst over the threshold doesn't flood the registry mailbox; * time fallback: at most every :max_flush_interval_ms (default 60_000) for low-volume distributions, keeping exported histograms fresh. Set either option to :infinity to restore the previous scrape-only behaviour. Aggregation now runs inside the registry process, so the read-modify-write of the cumulative aggregates table is serialized with the flushes and with concurrent scrapes -- also removing a pre-existing race when scrapes overlapped. Fixes beam-telemetry#41, beam-telemetry#52. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced Jun 29, 2026
robacourt
added a commit
to electric-sql/electric
that referenced
this pull request
Jun 29, 2026
When ELECTRIC_PROMETHEUS_PORT is set but the /metrics endpoint is scraped infrequently or never (e.g. OTel-only deployments), telemetry_metrics_prometheus_core buffers one ETS row per distribution observation and only drains on scrape, so the dist table grows without bound (the per-transaction receive_lag metric dominates). An 8GB+ ETS table and eventual OOM was observed in the field. Pin telemetry_metrics_prometheus_core to a fork that bounds the buffer by aggregating automatically on a size threshold (default 10k samples) and a time fallback (default 60s), in addition to on scrape. Only affects the MIX_TARGET=application build (the standalone sync-service / Docker image); the telemetry deps are target-gated out of the Hex package, so this git dep does not affect publishing of `electric`. Upstream PR: beam-telemetry/telemetry_metrics_prometheus_core#77 Revert to the Hex release once it lands. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YW7Njz5ZpBDaoGviW1eVR8
robacourt
added a commit
to electric-sql/electric
that referenced
this pull request
Jun 30, 2026
## Summary If `ELECTRIC_PROMETHEUS_PORT` is set but nothing ever scrapes the `/metrics` endpoint, Electric's memory grows without bound and can eventually OOM-kill the service. This came out of debugging a customer whose `prometheus_metrics_dist` ETS table had grown to ~8 GB on an OTel-only deployment that had the Prometheus port enabled but unscraped. ### Why it happens `telemetry_metrics_prometheus_core` stores **distribution (histogram)** metrics in a `:duplicate_bag` and appends one row per observation (`distribution.ex` `:ets.insert`), only aggregating into buckets — and draining the bag via `:ets.take` — **at scrape time** (documented in the library's moduledoc: *"aggregations for distributions (histogram) only occur at scrape time"*). So if the endpoint is never scraped, the bag is never drained. Electric's default Prometheus metric set includes `electric.postgres.replication.transaction_received.receive_lag`, a distribution emitted roughly once per replication transaction, so the table accretes continuously under load (~0.55 GB/day in the customer's case). Counters/sums/last-values are unaffected — they use bounded `:set` storage. This is purely a docs change to make the "must be scraped" requirement explicit everywhere we mention Prometheus. ## Changes - **Telemetry reference** (`website/docs/sync/reference/telemetry.md`) — warning callout in the Metrics section. - **Config reference** (`website/docs/sync/api/config.md`) — note on `ELECTRIC_PROMETHEUS_PORT`. - **Deployment guide** (`website/docs/sync/guides/deployment.md`) — warning in the Observability section. - **`prometheus_port` docstring** (`packages/sync-service/lib/electric.ex`). ## Follow-up (not in this PR) Worth considering a code-level guard so an enabled-but-unscraped endpoint can't OOM the service (e.g. move high-frequency distributions like `receive_lag` out of the default Prometheus set, or run an internal periodic drain). I have raised a PR with the library maintainer for a potential fix: beam-telemetry/telemetry_metrics_prometheus_core#77 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@robacourt was the close intentional? |
Author
No! Thank you :) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Distribution (histogram) samples are buffered in the
*_dist:duplicate_bagtable and only folded into their histogram buckets whenAggregator.aggregate/3runs — which today happens only at scrape time (:ets.takeinaggregator.ex). A reporter whose/metricsendpoint is scraped infrequently, or never, therefore accumulates one ETS row per observation, without bound.This bites anyone who starts the reporter but doesn't actually scrape it (e.g. metrics are shipped via another path, or the scraper is misconfigured). It's reported in:
In #52 the suggested fix was "a GenServer cron job that regularly aggregates the ETS data" — this PR builds that in.
Approach
The registry now aggregates on its own, in addition to on scrape, with two triggers:
:atomicscounter incremented in the distribution event handlers enqueues a single flush once:flush_thresholdsamples have buffered (default10_000). A second:atomicsflag (compare_exchange) de-bounces it so a burst over the threshold enqueues one flush, not one per event. The hot path adds only an atomic increment + compare.:max_flush_interval_ms(default60_000) so low-volume distributions still drain and exported histograms stay fresh.Both default on, so the reporter is safe out of the box. Set either to
:infinityto restore the previous scrape-only behaviour.Race-freedom
Aggregation is a read-modify-write of the cumulative aggregates table (
get → merge → putinaggregator.ex). Todayscrape/1runs it in the caller process, so two overlapping scrapes can already lose updates. This PR moves aggregation into the registry process (scrape/1now callsRegistry.aggregate/1), so all aggregation — scrape-triggered, size-triggered, and time-triggered — is serialized through one mailbox. The read-only export stays in the caller. This also fixes the pre-existing overlapping-scrape race.Compatibility
:infinity/:infinityrestores exact prior behaviour.Distribution.register/4gains an optional flush-context arg defaulting to a disabled context, so existingregister/3callers keep working.Tests
test/flush_test.exs: size-triggered flush (no scrape), time-fallback flush incl. the periodic chain re-arming across intervals, and the:infinity/:infinityopt-out (drains on scrape only).mix format --check-formattedclean.Docs (
core.exmoduledoc +start_link/1options) andCHANGELOG.mdupdated.