Skip to content

Commit 2a2a479

Browse files
lohanidamodarclaude
andcommitted
fix: buffer key tag-loss, daily column validation, agg_value truncation, getTotal ambiguity
- Buffer key now includes tag hash so events with same metric but different tags (e.g. different paths) stay as separate entries instead of silently discarding the second call's tags - Daily table queries (findDaily, sumDaily, sumDailyBatch) now validate attributes against the daily schema (metric, value, time, tenant) instead of the full event schema. Querying path/method/status on the daily table now throws immediately instead of causing a ClickHouse "No such column" runtime error - Changed (int) cast to (float) for agg_value in getTimeSeries to avoid truncating fractional gauge values or large event sums - getTotal() now throws when a metric exists in both event and gauge tables instead of silently adding incompatible aggregations Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent ab62cb2 commit 2a2a479

2 files changed

Lines changed: 60 additions & 10 deletions

File tree

src/Usage/Adapter/ClickHouse.php

Lines changed: 57 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1146,6 +1146,37 @@ private function validateAttributeName(string $attributeName, string $type = 'ev
11461146
throw new Exception("Invalid attribute name: {$attributeName}");
11471147
}
11481148

1149+
/**
1150+
* Columns available in the events daily (pre-aggregated) table.
1151+
*/
1152+
private const DAILY_COLUMNS = ['metric', 'value', 'time'];
1153+
1154+
/**
1155+
* Validate that a query attribute exists in the daily table schema.
1156+
* The daily table only has metric, value, time (+ tenant if shared).
1157+
*
1158+
* @throws Exception
1159+
*/
1160+
private function validateDailyAttributeName(string $attributeName): bool
1161+
{
1162+
if ($attributeName === 'id') {
1163+
return true;
1164+
}
1165+
1166+
if ($attributeName === 'tenant' && $this->sharedTables) {
1167+
return true;
1168+
}
1169+
1170+
if (in_array($attributeName, self::DAILY_COLUMNS, true)) {
1171+
return true;
1172+
}
1173+
1174+
throw new Exception(
1175+
"Invalid attribute '{$attributeName}' for daily table. "
1176+
. "Only metric, value, time" . ($this->sharedTables ? ", tenant" : "") . " are available."
1177+
);
1178+
}
1179+
11491180
/**
11501181
* Format datetime for ClickHouse compatibility.
11511182
*
@@ -1723,7 +1754,13 @@ public function findDaily(array $queries = []): array
17231754

17241755
$fromTable = $this->buildTableReference($this->getEventsDailyTableName());
17251756

1726-
// Daily table has limited columns — only allow metric, value, time, resource, resourceId, tenant
1757+
// Validate query attributes against daily table schema (metric, value, time, tenant only)
1758+
foreach ($queries as $query) {
1759+
$attr = $query->getAttribute();
1760+
if (!empty($attr)) {
1761+
$this->validateDailyAttributeName($attr);
1762+
}
1763+
}
17271764
$parsed = $this->parseQueries($queries, Usage::TYPE_EVENT);
17281765
$whereData = $this->buildWhereClause($parsed['filters'], $parsed['params']);
17291766

@@ -1755,9 +1792,15 @@ public function sumDaily(array $queries = [], string $attribute = 'value'): int
17551792
$this->setOperationContext('sumDaily()');
17561793

17571794
$fromTable = $this->buildTableReference($this->getEventsDailyTableName());
1758-
$this->validateAttributeName($attribute, Usage::TYPE_EVENT);
1795+
$this->validateDailyAttributeName($attribute);
17591796
$escapedAttribute = $this->escapeIdentifier($attribute);
17601797

1798+
foreach ($queries as $query) {
1799+
$attr = $query->getAttribute();
1800+
if (!empty($attr)) {
1801+
$this->validateDailyAttributeName($attr);
1802+
}
1803+
}
17611804
$parsed = $this->parseQueries($queries, Usage::TYPE_EVENT);
17621805
$whereData = $this->buildWhereClause($parsed['filters'], $parsed['params']);
17631806

@@ -1785,6 +1828,13 @@ public function sumDailyBatch(array $metrics, array $queries = []): array
17851828

17861829
$this->setOperationContext('sumDailyBatch()');
17871830

1831+
foreach ($queries as $query) {
1832+
$attr = $query->getAttribute();
1833+
if (!empty($attr)) {
1834+
$this->validateDailyAttributeName($attr);
1835+
}
1836+
}
1837+
17881838
$totals = \array_fill_keys($metrics, 0);
17891839

17901840
$fromTable = $this->buildTableReference($this->getEventsDailyTableName());
@@ -1991,7 +2041,7 @@ private function getTimeSeriesFromTable(array $metrics, string $interval, string
19912041
foreach ($json['data'] as $row) {
19922042
$metricName = $row['metric'] ?? '';
19932043
$bucketTime = $row['bucket'] ?? '';
1994-
$value = (int) ($row['agg_value'] ?? 0);
2044+
$value = (float) ($row['agg_value'] ?? 0);
19952045

19962046
if (!isset($output[$metricName])) {
19972047
continue;
@@ -2084,12 +2134,11 @@ public function getTotal(string $metric, array $queries = [], ?string $type = nu
20842134
$eventTotal = $this->getTotalFromEvents($metric, $queries);
20852135
$gaugeTotal = $this->getTotalFromGauges($metric, $queries);
20862136

2087-
// If we got data from both, prioritize event (they don't overlap in practice)
2088-
// If only one has data, return that
20892137
if ($eventTotal > 0 && $gaugeTotal > 0) {
2090-
// A metric shouldn't be in both tables; return whichever is nonzero
2091-
// In practice, callers specify type for ambiguous cases
2092-
return $eventTotal + $gaugeTotal;
2138+
throw new Exception(
2139+
"Metric '{$metric}' exists in both event and gauge tables. "
2140+
. "Specify \$type explicitly to avoid ambiguous aggregation."
2141+
);
20932142
}
20942143

20952144
return $eventTotal > 0 ? $eventTotal : $gaugeTotal;

src/Usage/Usage.php

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -299,10 +299,11 @@ public function collect(string $metric, int $value, string $type, array $tags =
299299
throw new \InvalidArgumentException("Invalid metric type '{$type}'. Allowed: " . self::TYPE_EVENT . ', ' . self::TYPE_GAUGE);
300300
}
301301

302-
$key = $metric . ':' . $type;
302+
$tagsHash = !empty($tags) ? md5(json_encode($tags)) : '';
303+
$key = $metric . ':' . $type . ':' . $tagsHash;
303304

304305
if ($type === 'event') {
305-
// Additive: sum values for the same metric
306+
// Additive: sum values for the same metric + tags combination
306307
if (isset($this->buffer[$key])) {
307308
$this->buffer[$key]['value'] += $value;
308309
} else {

0 commit comments

Comments
 (0)