refactor(usage): extract buffering into a standalone Accumulator#15
Conversation
Move collect()/flush() and the in-memory buffer out of Usage into a dedicated Accumulator that consumers instantiate against an adapter. Replace the shouldFlush()/threshold policy with raw signals — count() and elapsedSeconds() — so callers compose their own flush decision. Usage is now a pure adapter facade. Buffer behaviour is covered by a new DB-free AccumulatorTest using a recording fake adapter; the buffer tests are removed from UsageBase. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Greptile SummaryThis PR moves metric buffering out of
Confidence Score: 5/5The refactor is narrowly scoped and covered by updated unit and adapter tests. No code issues were identified in the reviewed changes, and the API split is consistently reflected across implementation, tests, benchmarks, and documentation.
What T-Rex did
Reviews (9): Last reviewed commit: "fix(clickhouse): reject empty tenant on ..." | Re-trigger Greptile |
Thread tenant through the API instead of carrying it as mutable adapter state, so a single Usage/adapter is safe across tenants and coroutines. - All read/query methods take `string $tenant` as the first parameter; addBatch carries `tenant` per metric row (a batch may span tenants). - Remove setTenant from Usage and both adapters. ClickHouse injects the tenant as a synthetic query filter (shared-tables only) via scopeToTenant(); Database scopes the underlying db per call. - Accumulator.collect() takes tenant first and keys the buffer by tenant. - Add Tenant decorator: binds a tenant once and forwards to Usage with it pre-filled (stamping addBatch rows) for single-tenant callers. Tests: inline adapter construction (drop makeAdapter helper), pass tenant explicitly, add a dedicated TenantTest, update README. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- ClickHouse: extract sumDailyScoped() so routedSum() stops calling the now-tenant-first sumDaily() with stale args (caught by phpstan). - ClickHouse purge: keep the tenant out of the daily-forwarding decision and apply it only as a scope on the resulting delete, so a purge by a daily-incompatible/raw-only filter no longer wipes the tenant's whole rollup. - Database: tenant key is required per the addBatch shape, drop dead guard. - Tests: stamp tenant on remaining multiline addBatch rows, tighten RecordingAdapter types, pint formatting. Full suite green (236 tests) against mariadb + clickhouse; phpstan + pint clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Make tenant scoping unavoidable instead of a step callers must remember to compose. parseQueries() — the single chokepoint every read/delete WHERE is built from — now takes the tenant as a required first argument and prepends the tenant filter itself (shared-tables mode). There is no longer any way to produce a WHERE clause that isn't tenant-scoped, and PHP/phpstan flag any caller that omits it. scopeToTenant() is removed; the tenant is threaded down to parseQueries through the private read helpers. purge keeps reasoning about the caller's own (tenant-free) filters for the daily-rollup forwarding decision. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Flush buffered metrics through the Usage facade instead of holding the adapter directly — the accumulator is a higher-level buffer over Usage. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… tenant guards - Accumulator: hash the full (tenant, metric, type, sorted-tags) identity so ambiguous splits like tenant "a"/metric "b:c" vs "a:b"/"c" can't collide, and tag insertion order no longer splits entries. Reject empty tenant. - Accumulator: only restart the flush timer when the buffer fully drains, so a partial-failure retry stays overdue instead of waiting a fresh interval. - ClickHouse: reject a shared-tables batch row without a tenant — it would be invisible to every tenant-scoped read. - Database: document that the adapter owns the injected db's tenant (no set/restore dance; hand it a db dedicated to usage). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
empty() treats the string "0" as empty, so a tenant or metric literally named "0" was wrongly rejected. Compare against '' instead. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| public function addBatch(array $metrics, string $type, int $batchSize = 1000): bool | ||
| { | ||
| foreach ($metrics as &$metric) { | ||
| $metric['tenant'] = $this->tenant; | ||
| } | ||
| unset($metric); | ||
|
|
||
| return $this->usage->addBatch($metrics, $type, $batchSize); | ||
| } |
There was a problem hiding this comment.
This wrapper overwrites any existing tenant on each metric row. If a caller accidentally passes a mixed-tenant batch into a bound tenant view, the rows are all written under the bound tenant with no signal, which can misattribute usage. Rejecting rows that already carry a conflicting tenant would keep this decorator from hiding a bad producer.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
new Tenant($usage, '') would silently scope every read/write to the empty tenant in shared-tables mode. Reject '' at construction, matching the accumulator/write-side rule. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| public function getTimeSeries(string $tenant, array $metrics, string $interval, string $startDate, string $endDate, array $queries = [], bool $zeroFill = true, ?string $type = null): array | ||
| { | ||
| return $this->adapter->getTimeSeries($metrics, $interval, $startDate, $endDate, $queries, $zeroFill, $type); | ||
| return $this->adapter->getTimeSeries($tenant, $metrics, $interval, $startDate, $endDate, $queries, $zeroFill, $type); |
There was a problem hiding this comment.
The direct Usage methods now take a tenant, but this path forwards an empty string unchanged. In shared-table ClickHouse mode that becomes a tenant = '' filter, so a caller with a missing tenant can silently read an empty scope instead of failing like Accumulator and Tenant do. The same guard should be applied before forwarding tenant-scoped reads and mutations.
| // tuples never collide on the key — a raw `:`-join would let | ||
| // e.g. tenant "a"/metric "b:c" and tenant "a:b"/metric "c" share one | ||
| // entry. Tags are sorted first so key order doesn't matter. |
There was a problem hiding this comment.
ksort() only normalizes the top-level tag keys before hashing. If a tag value is itself an array, the same logical tags with nested keys in a different insertion order still produce different hashes, so event values split into multiple buffered rows and gauges can keep more than one pending value for the same metric identity. Recursively sorting array tag values before json_encode() would keep the buffer identity stable.
An empty tenant compiled to `tenant = ''` and silently read an empty scope. parseQueries() is the single chokepoint every read funnels through, so guard there — matching the write-side and the Accumulator/Tenant rules. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
What
Extracts the in-memory metric buffering out of
Usageinto a dedicatedAccumulatorclass that consumers create themselves against an adapter.Accumulatorownscollect(),flush(), and the buffer. It exposes raw signals —count()(buffered entries) andelapsedSeconds()(since last flush) — and leaves the flush policy to the caller.Usageis now a pure adapter facade: no buffer, nocollect/flush/shouldFlush, no threshold config.Why
Usageno longer carries mutable buffer state, which is the wrong place for it in coroutine/multi-tenant setups where each context wants its own buffer.shouldFlush()+setFlushThreshold/setFlushIntervalpolicy machinery in favour of primitives the caller composes:Tests
AccumulatorTest— DB-free, uses a recording fake adapter to cover event summing, gauge last-write-wins, per-type batch separation, partial-failure buffer retention, and the signals/validation.UsageBase(the real-DBaddBatchround-trip stays covered bytestAddBatchEvent/testAddBatchGauge).Net -17 LoC overall.
🤖 Generated with Claude Code