fix: aggregate metrics bucket counters on insert

[rjmackay]

Description

Fixes quadratic growth in metrics cache writes, in the existing format and without changing any contract. MetricsBucket holds one MetricsBucketToggle per evaluation and is read-modify-written in a shared cache on every update, so bytes written per interval are N²·b/2 for N updates — the bucket grows with every write, and every write rewrites all of it.

DefaultMetricsBucketSerializer now writes each distinct entry once with a count, instead of repeating it, and expands the counts back on read. MetricsBucket, MetricsBucketToggle, DefaultMetricsHandler and CacheKey are untouched, as is every existing public API — the serializer gains one optional constructor argument, and it had no constructor before.

Fixes #258

Builds on #198, doesn't revisit it. #198 fixed cost per byte ("a simple (and small) string which can be reconstructed to the metrics bucket object is serialized instead") and left record count alone, so the growth survived with a smaller constant. This is the remaining half, in the format #198 introduced.

Effect — one interval, 100 distinct flags (script in #258). Cache value is what the bucket occupies at the end of the interval; written over the interval sums every one of those writes, since each update rewrites the whole bucket:

updates in interval	cache value: before → after	written over the interval: before → after
1,000	14.9 KB → 3.4 KB	7.5 MB → 2.7 MB
5,000	74.5 KB → 3.5 KB	186.3 MB → 16.7 MB
10,000	149.0 KB → 3.6 KB	745.1 MB → 34.4 MB
20,000	298.0 KB → 3.7 KB	2,980.3 MB → 71.2 MB

The first column is why the second one happens. Today the cache value grows by ~15 bytes per evaluation, so summing it across N writes comes to payload × N/2 — 298 KB × 10,000 = 2,980 MB, which is the measurement, and N²·b/2 confirmed. With a flat payload that sum is ~3.5 KB × N instead: 20× the updates costs 397× the bytes before and 26× after.

The number of writes doesn't change — one per update either way; this changes how big each is. Real cache traffic is roughly double the figures above, because every update reads the value before rewriting it, and both sides improve by the same factor.

What changed

One file. serialize() groups the records by feature, outcome and variant, and appends the number of times each combination occurred:

before:  1700000000;checkout-v2:1:blue,checkout-v2:0:~,search-ranking:1:~,checkout-v2:1:blue;~
after:   1700000000;checkout-v2:1:blue:2,checkout-v2:0:~:1,search-ranking:1:~:1;~

There's one entry per feature, outcome and variant combination, so a feature evaluated both ways takes two entries and each variant adds one — bounded by how the features are configured, not by how often they're evaluated, which is the property that fixes the bug. deserialize() reads the count and rebuilds that many records, so the bucket keeps exactly the shape it has today. The stub DefaultFeature is the same one the current implementation rebuilds; it's now allocated once per entry rather than once per record, as is the DefaultVariant.

The count is an extension of the format, not a new one

An entry with no count means one evaluation — which is precisely what every earlier version wrote. The two formats are therefore the same format, and both directions of a rolling deploy are safe. Verified against 2.10.1, not just asserted:

• 2.10.1 payload → this code: read in full. 1700000000;checkout-v2:1:blue,checkout-v2:0:~,search-ranking:1:~,checkout-v2:1:blue;~ → {"checkout-v2":{"yes":2,"no":1,"variants":{"blue":2}},"search-ranking":{"yes":1,"no":0}}. Nothing is lost on upgrade.
• This code's payload → 2.10.1: read, ignoring the counts. It takes the first three fields of each entry and drops the fourth, so it sees one evaluation per entry and under-counts the single interval it flushes. No exception, no warning.

That second direction is why the count is a fourth field inside the entry rather than a version marker or a new top-level field. 2.10.1's deserialize() destructures on ; unguarded, and anything it doesn't recognise throws TypeError: DefaultVariant::__construct(): Argument #1 ($name) must be of type string, null given — uncaught, from inside isEnabled(). Any format it can't parse 500s every process still running it for the length of the deploy. This shape can't trigger that.

In the other direction deserialize() never throws, and a payload it can't fully make sense of costs as little as it can:

• Unreadable interval boundaries → a fresh empty bucket. If the interval itself can't be read there's nothing in the payload worth keeping.
• A suspect entry → that entry is skipped, the interval survives. This matters more than it looks. Callers decide whether an interval has elapsed from the bucket's start date, so handing back a bucket started now leaves the interval permanently incomplete and metrics silently never send — not delayed, never. A feature name containing a comma is enough to trigger it, since serialize() writes it and deserialize() then can't read it back: measured through DefaultMetricsHandler, 0 flushes in 2.4s against a 0.5s interval, against 5 once the entry is skipped instead.
• An entry over the expansion budget → skipped, not clamped. A count can claim billions of evaluations where the repeated-entry format couldn't, so expansion is bounded per bucket. Skipped rather than clamped to what's left, because a count that far above a plausible interval is likelier corrupt than honest and clamping would fabricate records from it. The entries either side of it are kept.

The budget is a constructor argument, defaulting to DEFAULT_MAX_EVALUATIONS = 500_000, because only the consumer knows what its memory_limit and per-request budget can absorb — a web tier at 128M and a queue worker at 1G in the same deployment want different answers. Measured on 8.4: ~113 bytes per expanded record, so the default is ~54 MB, and expansion costs ~0.2ms per 1,000 records. Reaching it honestly needs more than 8,000 evaluations a second, fleet-wide, against a 60 second interval.

Feature names containing ;, , or : still don't round trip, exactly as before — on main such a name makes 2.10.1's own reader throw the TypeError above; here it fails soft. I did try percent-encoding the names, and dropped it: it would have made a payload written now unreadable to older versions, which isn't worth fixing a case that has never worked.

What this deliberately does not fix

CPU is still O(evaluations) per update, because MetricsBucket still holds one record per evaluation — deserialize() has to rebuild them, just as it does today. Same loop, but reading the bucket back each time the way DefaultMetricsHandler does:

updates in interval	wall before	wall after	peak memory before	after
1,000	0.24 s	0.19 s	4 MB	2 MB
5,000	6.34 s	3.68 s	6 MB	4 MB
10,000	25.78 s	14.08 s	10 MB	6 MB
20,000	115.94 s	52.84 s	18 MB	8 MB

Faster and lighter than today at every size, but the same shape of curve. This PR fixes the cache-traffic half of the bug — the half that hits a shared Redis and that #197 was reported about.

Aggregating inside MetricsBucket would make that O(1) too, and I have it working — it needs add()/getCounts() on the bucket and either deprecates or replaces getToggles(). I left it out because MetricsBucketSerializer is public and a third-party implementation may call getToggles(), so this way the PR asks nothing of anyone. Happy to push it here or as a follow-up.

Also out of scope, as separate concerns: the per-call cache I/O in DefaultMetricsHandler (what a custom handler solves, per #197), and the lost-update race inherent to read-modify-write over PSR-16 — concurrent processes can still overwrite each other's counts exactly as they can today, and fixing it needs atomic increments PSR-16 can't express.

Type of change

• Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

• Unit tests
• Spec Tests
• Integration tests / Manual Tests

All green locally on 8.4: composer phpunit (279 tests, incl. the client-specification submodule), composer phpstan (level=max), composer fixer. Rector's 7.2 downgrade was also run over src and the suite re-run against the transpiled output (239 tests — UnleashBuilderTest and AbstractNumberOperatorValidatorTest need their *Impl80 helpers, which can't load against transpiled src on 8.4; unrelated to this change). Both compatibility directions were checked by running the real 2.10.1 code against payloads from this branch and vice versa, as quoted above.

New tests, all in DefaultMetricsBucketSerializerTest:

• The entry fan-out: a feature with mixed outcomes and two variants serializes to exactly flag:1:blue:300,flag:1:green:100,flag:0:~:100, with the counts the server receives asserted alongside it.
• The regression guard: 10,000 evaluations of one flag serialize to exactly 3 bytes more than 10 evaluations — the count gaining 3 digits. Without it this fix can be silently undone later.
• Reading the previous format: four payloads without counts, including an empty bucket and a variant case, each producing the counts they represent.
• Writing something the previous format can read: the exact serialized string is asserted, so the entry keeps its name:outcome:variant prefix and the count stays last.
• Round trip: jsonSerialize() identical after a round trip for counts, variants, interval start, null and non-null end date, plus the record count coming back as expected.
• Fail-soft, split by what's actually wrong: eight payloads whose interval is unreadable — garbage, JSON, wrong field counts, non-numeric timestamps — each yielding a fresh bucket; and three whose entries are unusable, each keeping its real start date and losing only those entries.
• The expansion budget: the entries either side of an oversized one survive, the allowance applies across the bucket rather than per entry, and the limit is configurable.
• A name containing a delimiter produces nonsense rather than an exception, as it did before.

The existing round-trip data provider is unchanged, so the buckets it already covered still have to survive the new format.

Checklist:

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes
• Any dependent changes have been merged and published in downstream modules

Docs are unchanged because none of them mention MetricsBucket, getToggles() or the stored format — the README's Metrics section documents builder options only. Happy to write up the cache format if you'd like it somewhere.

[rjmackay]

FYI we've just started running this fix on our own service and this is the difference in network throughput when it deployed