Skip to content

Commit 5893999

Browse files
lohanidamodarlohanidamodar
committed
fix: PR review correctness bugs (findDaily FINAL, orderBy aggregated, retry dedup, gauge order, cross-type validation, Database value check)
1 parent 0f843bb commit 5893999

3 files changed

Lines changed: 119 additions & 33 deletions

File tree

‎src/Usage/Adapter/ClickHouse.php‎

Lines changed: 78 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -888,11 +888,13 @@ function (int $attempt) use ($table, $data): void {
888888
}
889889
},
890890
function (Exception $e, ?int $httpCode): bool {
891-
$exceptionHttpCode = null;
892-
if (preg_match('/\|HTTP_CODE:(\d+)$/', $e->getMessage(), $matches)) {
893-
$exceptionHttpCode = (int) $matches[1];
894-
}
895-
return $this->isRetryableError($exceptionHttpCode, $e->getMessage());
891+
// Never retry inserts. The underlying MergeTree engine has
892+
// no row-level deduplication, so a retried insert that hits
893+
// the server twice (network blip + first request actually
894+
// succeeded) leaves duplicate rows behind. Surface the
895+
// failure to the caller instead — they can replay the
896+
// batch from durable storage if they choose.
897+
return false;
896898
},
897899
function (Exception $e, int $attempt) use ($table, $data): Exception {
898900
$cleanMessage = preg_replace('/\|HTTP_CODE:\d+$/', '', $e->getMessage());
@@ -1099,6 +1101,10 @@ private function createDailyTable(): void
10991101
*
11001102
* @throws Exception
11011103
*/
1104+
// NOTE: setup() uses CREATE IF NOT EXISTS for idempotency. If sharedTables
1105+
// is toggled between calls, the original MV definition is kept (DROP+CREATE
1106+
// would lose buffered data). This is acceptable for v1 since setup() is
1107+
// expected to run once per environment lifecycle.
11021108
private function createDailyMaterializedView(): void
11031109
{
11041110
$eventsTable = $this->getEventsTableName();
@@ -1157,14 +1163,11 @@ private function validateAttributeName(string $attributeName, string $type = 'ev
11571163
}
11581164
}
11591165

1160-
// Also check the other type's attributes for cross-table queries
1161-
$otherType = $type === 'event' ? 'gauge' : 'event';
1162-
foreach ($this->getAttributes($otherType) as $attribute) {
1163-
if ($attribute['$id'] === $attributeName) {
1164-
return true;
1165-
}
1166-
}
1167-
1166+
// Reject attributes that don't exist on the target type's schema.
1167+
// Falling back to the other type's columns (e.g. allowing `path` on
1168+
// a gauge query because it exists on the event schema) compiles to
1169+
// SQL that references columns the gauge table doesn't have, which
1170+
// ClickHouse rejects with "Unknown identifier".
11681171
throw new Exception("Invalid attribute name: {$attributeName}");
11691172
}
11701173

@@ -1480,18 +1483,34 @@ public function find(array $queries = [], ?string $type = null): array
14801483

14811484
// Cursor pagination is per-table — paginating across both events and
14821485
// gauges has no coherent ordering, so reject this combination upfront.
1486+
$userLimit = null;
14831487
foreach ($queries as $query) {
14841488
$method = $query->getMethod();
14851489
if ($method === Query::TYPE_CURSOR_AFTER || $method === Query::TYPE_CURSOR_BEFORE) {
14861490
throw new Exception('Cursor pagination requires an explicit $type (event or gauge)');
14871491
}
1492+
if ($method === Query::TYPE_LIMIT) {
1493+
$values = $query->getValues();
1494+
if (!empty($values) && is_numeric($values[0])) {
1495+
$userLimit = (int) $values[0];
1496+
}
1497+
}
14881498
}
14891499

1490-
// Query both tables with UNION ALL
1500+
// Query both tables and merge. Each side already applied LIMIT, so
1501+
// without a final cap callers asking for `limit(N)` could receive
1502+
// up to 2N rows. Slice the merged result back down to the user's
1503+
// requested limit.
14911504
$events = $this->findFromTable($queries, Usage::TYPE_EVENT);
14921505
$gauges = $this->findFromTable($queries, Usage::TYPE_GAUGE);
14931506

1494-
return array_merge($events, $gauges);
1507+
$merged = array_merge($events, $gauges);
1508+
1509+
if ($userLimit !== null && count($merged) > $userLimit) {
1510+
$merged = array_slice($merged, 0, $userLimit);
1511+
}
1512+
1513+
return $merged;
14951514
}
14961515

14971516
/**
@@ -1607,10 +1626,22 @@ private function findAggregatedFromTable(array $parsed, string $fromTable, strin
16071626
$whereClause = $whereData['clause'];
16081627
$params = $whereData['params'];
16091628

1610-
// Use custom ORDER BY if specified, otherwise default to bucket ASC
1629+
// Use custom ORDER BY if specified, otherwise default to bucket ASC.
1630+
// In aggregated mode the SELECT exposes `bucket` instead of `time`,
1631+
// so any user-supplied ORDER BY on `time` must be rewritten to
1632+
// reference the bucket alias — otherwise ClickHouse errors with
1633+
// "Unknown identifier: time".
16111634
$orderClause = ' ORDER BY bucket ASC';
16121635
if (!empty($parsed['orderBy'])) {
1613-
$orderClause = ' ORDER BY ' . implode(', ', $parsed['orderBy']);
1636+
$rewrittenOrderBy = array_map(
1637+
fn (string $clause): string => preg_replace(
1638+
'/^`time`(\s+(?:ASC|DESC))?$/',
1639+
'`bucket`$1',
1640+
$clause
1641+
) ?? $clause,
1642+
$parsed['orderBy']
1643+
);
1644+
$orderClause = ' ORDER BY ' . implode(', ', $rewrittenOrderBy);
16141645
}
16151646

16161647
$limitClause = isset($parsed['limit']) ? ' LIMIT {limit:UInt64}' : '';
@@ -1668,7 +1699,21 @@ private function parseAggregatedResults(string $result, string $type = 'event'):
16681699
}
16691700
$document['time'] = $parsedTime;
16701701
} elseif ($key === 'value') {
1671-
$document[$key] = $value !== null ? (int) $value : null;
1702+
// Preserve numeric precision: SUM(value) over many rows
1703+
// can exceed PHP_INT_MAX, and gauge averages are floats.
1704+
// Casting to int truncates both cases — keep numeric
1705+
// strings as int|float depending on shape.
1706+
if ($value === null) {
1707+
$document[$key] = null;
1708+
} elseif (is_int($value) || is_float($value)) {
1709+
$document[$key] = $value;
1710+
} elseif (is_numeric($value)) {
1711+
$document[$key] = (str_contains((string) $value, '.') || str_contains((string) $value, 'e') || str_contains((string) $value, 'E'))
1712+
? (float) $value
1713+
: (int) $value;
1714+
} else {
1715+
$document[$key] = $value;
1716+
}
16721717
} else {
16731718
$document[$key] = $value;
16741719
}
@@ -1704,9 +1749,17 @@ public function count(array $queries = [], ?string $type = null, ?int $max = nul
17041749
return $this->countFromTable($queries, $type, $max);
17051750
}
17061751

1707-
// Count from both tables
1708-
return $this->countFromTable($queries, Usage::TYPE_EVENT, $max)
1709-
+ $this->countFromTable($queries, Usage::TYPE_GAUGE, $max);
1752+
// Count from both tables. Each per-table count is independently
1753+
// capped at $max, so naively summing them could yield up to 2*$max.
1754+
// Cap the combined total at $max in PHP to honour the contract.
1755+
$total = $this->countFromTable($queries, Usage::TYPE_EVENT, $max)
1756+
+ $this->countFromTable($queries, Usage::TYPE_GAUGE, $max);
1757+
1758+
if ($max !== null && $total > $max) {
1759+
$total = $max;
1760+
}
1761+
1762+
return $total;
17101763
}
17111764

17121765
/**
@@ -1847,7 +1900,10 @@ public function findDaily(array $queries = []): array
18471900
$limitClause = isset($parsed['limit']) ? ' LIMIT {limit:UInt64}' : '';
18481901
$offsetClause = isset($parsed['offset']) ? ' OFFSET {offset:UInt64}' : '';
18491902

1850-
$sql = "SELECT {$selectColumns} FROM {$fromTable}{$whereData['clause']}{$orderClause}{$limitClause}{$offsetClause} FORMAT JSON";
1903+
// The daily table is SummingMergeTree. Reading raw rows returns
1904+
// un-merged duplicates until background merges run. FINAL forces
1905+
// merge-on-read so callers always see fully-collapsed values.
1906+
$sql = "SELECT {$selectColumns} FROM {$fromTable} FINAL{$whereData['clause']}{$orderClause}{$limitClause}{$offsetClause} FORMAT JSON";
18511907

18521908
return $this->parseResults($this->query($sql, $whereData['params']), Usage::TYPE_EVENT);
18531909
}

‎src/Usage/Adapter/Database.php‎

Lines changed: 27 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -134,6 +134,10 @@ public function addBatch(array $metrics, string $type, int $batchSize = 1000): b
134134
throw new \InvalidArgumentException("Invalid type '{$type}'. Allowed: event, gauge");
135135
}
136136

137+
if ($metric['value'] < 0) {
138+
throw new \InvalidArgumentException('Value cannot be negative');
139+
}
140+
137141
$tags = $metric['tags'] ?? [];
138142
ksort($tags);
139143

@@ -207,19 +211,29 @@ public function getTotal(string $metric, array $queries = [], ?string $type = nu
207211
Query::equal('metric', [$metric]),
208212
]);
209213

214+
if ($type === Usage::TYPE_GAUGE) {
215+
// For gauge, return the most recent value by time. find() does
216+
// not guarantee any ordering, so we explicitly sort + limit
217+
// here instead of relying on insertion order.
218+
$gaugeQueries = array_merge($allQueries, [
219+
Query::orderDesc('time'),
220+
Query::limit(1),
221+
]);
222+
/** @var array<Metric> $gaugeResults */
223+
$gaugeResults = $this->find($gaugeQueries, $type);
224+
if (empty($gaugeResults)) {
225+
return 0;
226+
}
227+
return (int) ($gaugeResults[0]->getValue(0) ?? 0);
228+
}
229+
210230
/** @var array<Metric> $results */
211231
$results = $this->find($allQueries, $type);
212232

213233
if (empty($results)) {
214234
return 0;
215235
}
216236

217-
if ($type === Usage::TYPE_GAUGE) {
218-
// For gauge, return the last (most recently inserted) value
219-
$lastResult = end($results);
220-
return $lastResult->getValue(0) ?? 0;
221-
}
222-
223237
if ($type === Usage::TYPE_EVENT) {
224238
// For events, SUM all values
225239
$sum = 0;
@@ -248,8 +262,14 @@ public function getTotal(string $metric, array $queries = [], ?string $type = nu
248262
}
249263

250264
if (!empty($gaugeResults)) {
265+
// find() returns rows in unspecified order; sort by time so the
266+
// "latest" gauge sample is deterministic.
267+
usort(
268+
$gaugeResults,
269+
fn (Metric $a, Metric $b): int => strcmp($a->getTime() ?? '', $b->getTime() ?? '')
270+
);
251271
$lastResult = end($gaugeResults);
252-
return $lastResult->getValue(0) ?? 0;
272+
return (int) ($lastResult->getValue(0) ?? 0);
253273
}
254274

255275
$sum = 0;

‎src/Usage/Metric.php‎

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -100,13 +100,19 @@ public function getMetric(): string
100100
* Returns the numeric value associated with this metric.
101101
* For example, number of requests, bytes transferred, or execution count.
102102
*
103-
* @param int|null $default Default value to return if not set
104-
* @return int|null The metric value, or the default if not set or invalid
103+
* Aggregated queries (SUM, argMax, AVG) can produce values that exceed
104+
* PHP_INT_MAX or include fractional parts, so this returns int|float.
105+
*
106+
* @param int|float|null $default Default value to return if not set
107+
* @return int|float|null The metric value, or the default if not set or invalid
105108
*/
106-
public function getValue(?int $default = null): ?int
109+
public function getValue(int|float|null $default = null): int|float|null
107110
{
108111
$value = $this->getAttribute('value', $default ?? 0);
109-
return is_int($value) ? $value : $default;
112+
if (is_int($value) || is_float($value)) {
113+
return $value;
114+
}
115+
return $default;
110116
}
111117

112118
/**
@@ -238,6 +244,10 @@ public function getUserAgent(): ?string
238244
*
239245
* @return array<string, mixed> Associative array of tags
240246
*/
247+
// NOTE: loks0n flagged this as a leftover from the previous Metric
248+
// implementation. Kept because tests (MetricTest, ClickHouseTest) and
249+
// downstream consumers still call it; remove once those callers are
250+
// migrated to direct `tags` attribute access.
241251
public function getTags(): array
242252
{
243253
$tags = $this->getAttribute('tags', []);

0 commit comments

Comments
 (0)