Skip to content

Commit a456325

Browse files
lohanidamodarclaude
lohanidamodar
and
claude
committed
fix: PR review comments - daily filters, purge MV, context flush, type strictness
- ClickHouse purge() now also deletes from the daily aggregated table when purging events. Materialized views are forward-only, so purges on the source table left stale daily rows behind. Daily delete is skipped if any query references an event-only column (path/method/etc). - ClickHouse getTotalBatch() now raises when a metric appears in both the event and gauge tables under $type=null, matching the existing safeguard in getTotal(). Mixing SUM (events) with argMax (gauges) silently produced meaningless totals. - Usage::setNamespace/setTenant/setSharedTables now flush the buffer before changing adapter context. Buffered metrics carry no context, so changing it pre-flush would write them under the new context. - Database adapter now stores a 'type' field per document and filters by it in find/count/purge/getTotal when $type is non-null. Previously the $type argument was accepted but ignored, returning rows of both kinds. - composer.json: add 'test' script. - .github/workflows: bump actions/checkout v3 -> v4. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent f241e0d commit a456325

7 files changed

Lines changed: 165 additions & 14 deletions

File tree

‎.github/workflows/codeql-analysis.yml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ jobs:
88

99
steps:
1010
- name: Checkout repository
11-
uses: actions/checkout@v3
11+
uses: actions/checkout@v4
1212
with:
1313
fetch-depth: 2
1414

‎.github/workflows/linter.yml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ jobs:
88

99
steps:
1010
- name: Checkout repository
11-
uses: actions/checkout@v3
11+
uses: actions/checkout@v4
1212
with:
1313
fetch-depth: 2
1414

‎.github/workflows/tests.yml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ jobs:
88

99
steps:
1010
- name: Checkout repository
11-
uses: actions/checkout@v3
11+
uses: actions/checkout@v4
1212
with:
1313
fetch-depth: 2
1414

‎composer.json‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,8 @@
1212
"scripts": {
1313
"lint": "./vendor/bin/pint --test",
1414
"format": "./vendor/bin/pint",
15-
"check": "./vendor/bin/phpstan analyse --level max src tests"
15+
"check": "./vendor/bin/phpstan analyse --level max src tests",
16+
"test": "./vendor/bin/phpunit --configuration phpunit.xml tests"
1617
},
1718
"minimum-stability": "stable",
1819
"require": {

‎src/Usage/Adapter/ClickHouse.php‎

Lines changed: 77 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2288,6 +2288,12 @@ private function getTotalFromGauges(string $metric, array $queries): int
22882288
/**
22892289
* Get totals for multiple metrics in a single query.
22902290
*
2291+
* When $type is null both tables are queried with their type-appropriate
2292+
* aggregator (SUM for events, argMax for gauges). If a metric appears in
2293+
* both tables the result of mixing those aggregators is meaningless, so
2294+
* the second occurrence raises an exception — callers must specify $type
2295+
* to disambiguate.
2296+
*
22912297
* @param array<string> $metrics
22922298
* @param array<Query> $queries
22932299
* @param string|null $type 'event', 'gauge', or null (both)
@@ -2305,6 +2311,9 @@ public function getTotalBatch(array $metrics, array $queries = [], ?string $type
23052311
// Initialize all metrics to 0
23062312
$totals = \array_fill_keys($metrics, 0);
23072313

2314+
// Track which type contributed a non-zero value to detect ambiguous mixing.
2315+
$contributingType = [];
2316+
23082317
$typesToQuery = [];
23092318
if ($type === Usage::TYPE_EVENT || $type === null) {
23102319
$typesToQuery[] = Usage::TYPE_EVENT;
@@ -2369,7 +2378,22 @@ public function getTotalBatch(array $metrics, array $queries = [], ?string $type
23692378
continue;
23702379
}
23712380

2372-
$totals[$metricName] += (int) ($row['agg_val'] ?? 0);
2381+
$rowValue = (int) ($row['agg_val'] ?? 0);
2382+
if ($rowValue === 0) {
2383+
continue;
2384+
}
2385+
2386+
if ($type === null
2387+
&& isset($contributingType[$metricName])
2388+
&& $contributingType[$metricName] !== $queryType) {
2389+
throw new Exception(
2390+
"Metric '{$metricName}' exists in both event and gauge tables. "
2391+
. "Specify \$type explicitly to avoid ambiguous aggregation."
2392+
);
2393+
}
2394+
2395+
$contributingType[$metricName] = $queryType;
2396+
$totals[$metricName] = $rowValue;
23732397
}
23742398
}
23752399
}
@@ -2999,6 +3023,13 @@ private function getTenantFilter(): string
29993023
* Purge usage metrics matching the given queries.
30003024
* Deletes from the specified table(s).
30013025
*
3026+
* For event purges, also deletes matching rows from the pre-aggregated
3027+
* daily table — materialized views are forward-only triggers, so deletes
3028+
* on the source table do not propagate to the MV target. Only daily-table
3029+
* compatible filters (metric, value, time, tenant) are forwarded; queries
3030+
* with event-only attributes (path/method/status/etc.) leave existing
3031+
* daily rows in place.
3032+
*
30023033
* @param array<Query> $queries
30033034
* @param string|null $type 'event', 'gauge', or null (purge both)
30043035
* @throws Exception
@@ -3030,8 +3061,53 @@ public function purge(array $queries = [], ?string $type = null): bool
30303061

30313062
$sql = "DELETE FROM {$escapedTable}{$whereClause}";
30323063
$this->query($sql, $params);
3064+
3065+
if ($purgeType === Usage::TYPE_EVENT) {
3066+
$this->purgeDaily($queries);
3067+
}
30333068
}
30343069

30353070
return true;
30363071
}
3072+
3073+
/**
3074+
* Purge matching rows from the daily aggregated table.
3075+
*
3076+
* Only forwarded when every query attribute is daily-compatible
3077+
* (metric, value, time, tenant). If any query references an
3078+
* event-only column, the daily delete is skipped — silently
3079+
* leaving the daily rows in place is safer than throwing here
3080+
* because callers commonly purge by path/method/etc.
3081+
*
3082+
* @param array<Query> $queries
3083+
* @throws Exception
3084+
*/
3085+
private function purgeDaily(array $queries): void
3086+
{
3087+
$dailyQueries = [];
3088+
foreach ($queries as $query) {
3089+
$attr = $query->getAttribute();
3090+
if (!empty($attr)) {
3091+
if ($attr !== 'id'
3092+
&& !in_array($attr, self::DAILY_COLUMNS, true)
3093+
&& !($attr === 'tenant' && $this->sharedTables)) {
3094+
return;
3095+
}
3096+
}
3097+
$dailyQueries[] = $query;
3098+
}
3099+
3100+
$dailyTable = $this->buildTableReference($this->getEventsDailyTableName());
3101+
3102+
$parsed = $this->parseQueries($dailyQueries, Usage::TYPE_EVENT);
3103+
$whereData = $this->buildWhereClause($parsed['filters'], $parsed['params']);
3104+
$whereClause = $whereData['clause'];
3105+
3106+
if (empty($whereClause)) {
3107+
$whereClause = ' WHERE 1=1';
3108+
}
3109+
3110+
$sql = "DELETE FROM {$dailyTable}{$whereClause}";
3111+
$this->query($sql, $whereData['params']);
3112+
}
30373113
}

‎src/Usage/Adapter/Database.php‎

Lines changed: 69 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,23 @@ public function setup(): void
7777
$attributes = $this->getAttributeDocuments('event');
7878
$indexDocs = $this->getIndexDocuments('event');
7979

80+
// Append a `type` column so a single collection can disambiguate event vs gauge rows.
81+
// ClickHouse uses separate tables instead, so this lives in the Database adapter only.
82+
$attributes[] = new Document([
83+
'$id' => 'type',
84+
'type' => 'string',
85+
'size' => 16,
86+
'required' => false,
87+
'signed' => true,
88+
'array' => false,
89+
'filters' => [],
90+
]);
91+
$indexDocs[] = new Document([
92+
'$id' => 'index-type',
93+
'type' => 'key',
94+
'attributes' => ['type'],
95+
]);
96+
8097
try {
8198
$this->db->createCollection(
8299
$this->collection,
@@ -125,6 +142,7 @@ public function addBatch(array $metrics, string $type, int $batchSize = 1000): b
125142
'$permissions' => [],
126143
'metric' => $metric['metric'],
127144
'value' => $metric['value'],
145+
'type' => $type,
128146
'time' => (new \DateTime())->format('Y-m-d H:i:s.v'),
129147
'tags' => $tags,
130148
];
@@ -189,10 +207,8 @@ public function getTotal(string $metric, array $queries = [], ?string $type = nu
189207
Query::equal('metric', [$metric]),
190208
]);
191209

192-
// If type is specified, query only that type
193-
$queryType = $type;
194210
/** @var array<Metric> $results */
195-
$results = $this->find($allQueries, $queryType);
211+
$results = $this->find($allQueries, $type);
196212

197213
if (empty($results)) {
198214
return 0;
@@ -213,17 +229,31 @@ public function getTotal(string $metric, array $queries = [], ?string $type = nu
213229
return $sum;
214230
}
215231

216-
// Type is null — try to detect from results
217-
$firstType = $results[0]->getType();
232+
// Type is null — partition results by stored type and reject ambiguous mixes.
233+
$eventResults = [];
234+
$gaugeResults = [];
235+
foreach ($results as $result) {
236+
if ($result->getType() === Usage::TYPE_GAUGE) {
237+
$gaugeResults[] = $result;
238+
} else {
239+
$eventResults[] = $result;
240+
}
241+
}
218242

219-
if ($firstType === 'gauge') {
220-
$lastResult = end($results);
243+
if (!empty($eventResults) && !empty($gaugeResults)) {
244+
throw new \Exception(
245+
"Metric '{$metric}' exists as both event and gauge. "
246+
. "Specify \$type explicitly to avoid ambiguous aggregation."
247+
);
248+
}
249+
250+
if (!empty($gaugeResults)) {
251+
$lastResult = end($gaugeResults);
221252
return $lastResult->getValue(0) ?? 0;
222253
}
223254

224-
// Default to SUM for events
225255
$sum = 0;
226-
foreach ($results as $result) {
256+
foreach ($eventResults as $result) {
227257
$sum += (int) ($result->getValue(0) ?? 0);
228258
}
229259

@@ -412,6 +442,8 @@ private function convertQueriesToDatabase(array $queries): array
412442
*/
413443
public function purge(array $queries = [], ?string $type = null): bool
414444
{
445+
$queries = $this->withTypeFilter($queries, $type);
446+
415447
$this->db->getAuthorization()->skip(function () use ($queries) {
416448
$dbQueries = $this->convertQueriesToDatabase($queries);
417449
$dbQueries[] = DatabaseQuery::limit(100);
@@ -434,12 +466,18 @@ public function purge(array $queries = [], ?string $type = null): bool
434466
/**
435467
* Find metrics using Query objects.
436468
*
469+
* When $type is non-null an additional `type = $type` filter is applied
470+
* so callers can isolate event vs gauge rows. When $type is null both
471+
* are returned (caller distinguishes via Metric::getType()).
472+
*
437473
* @param array<Query> $queries
438474
* @param string|null $type
439475
* @return array<Metric>
440476
*/
441477
public function find(array $queries = [], ?string $type = null): array
442478
{
479+
$queries = $this->withTypeFilter($queries, $type);
480+
443481
/** @var array<Document> $result */
444482
$result = $this->db->getAuthorization()->skip(function () use ($queries) {
445483
$dbQueries = $this->convertQueriesToDatabase($queries);
@@ -466,6 +504,8 @@ public function find(array $queries = [], ?string $type = null): array
466504
*/
467505
public function count(array $queries = [], ?string $type = null, ?int $max = null): int
468506
{
507+
$queries = $this->withTypeFilter($queries, $type);
508+
469509
/** @var int $count */
470510
$count = $this->db->getAuthorization()->skip(function () use ($queries, $max) {
471511
$dbQueries = $this->convertQueriesToDatabase($queries);
@@ -479,6 +519,26 @@ public function count(array $queries = [], ?string $type = null, ?int $max = nul
479519
return $count;
480520
}
481521

522+
/**
523+
* Append a `type = $type` filter to the query list when $type is non-null.
524+
*
525+
* @param array<Query> $queries
526+
* @param string|null $type
527+
* @return array<Query>
528+
*/
529+
private function withTypeFilter(array $queries, ?string $type): array
530+
{
531+
if ($type === null) {
532+
return $queries;
533+
}
534+
535+
if ($type !== Usage::TYPE_EVENT && $type !== Usage::TYPE_GAUGE) {
536+
throw new \InvalidArgumentException("Invalid type '{$type}'. Allowed: " . Usage::TYPE_EVENT . ', ' . Usage::TYPE_GAUGE);
537+
}
538+
539+
return array_merge($queries, [Query::equal('type', [$type])]);
540+
}
541+
482542
/**
483543
* Set the namespace prefix for table names.
484544
*

‎src/Usage/Usage.php‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -266,36 +266,50 @@ public function sumDailyBatch(array $metrics, array $queries = []): array
266266
/**
267267
* Set the namespace prefix for table names.
268268
*
269+
* Flushes the buffer first so any pending metrics are written under the
270+
* previous namespace — buffered entries don't carry adapter context.
271+
*
269272
* @param string $namespace
270273
* @return $this
271274
* @throws \Exception
272275
*/
273276
public function setNamespace(string $namespace): self
274277
{
278+
$this->flush();
275279
$this->adapter->setNamespace($namespace);
276280
return $this;
277281
}
278282

279283
/**
280284
* Set the tenant ID for multi-tenant support.
281285
*
286+
* Flushes the buffer first so any pending metrics are written under the
287+
* previous tenant — buffered entries don't carry adapter context.
288+
*
282289
* @param string|null $tenant
283290
* @return $this
291+
* @throws \Exception
284292
*/
285293
public function setTenant(?string $tenant): self
286294
{
295+
$this->flush();
287296
$this->adapter->setTenant($tenant);
288297
return $this;
289298
}
290299

291300
/**
292301
* Enable or disable shared tables mode (multi-tenant with tenant column).
293302
*
303+
* Flushes the buffer first so any pending metrics are written under the
304+
* previous mode — buffered entries don't carry adapter context.
305+
*
294306
* @param bool $sharedTables
295307
* @return $this
308+
* @throws \Exception
296309
*/
297310
public function setSharedTables(bool $sharedTables): self
298311
{
312+
$this->flush();
299313
$this->adapter->setSharedTables($sharedTables);
300314
return $this;
301315
}

0 commit comments

Comments
 (0)