mirror of
https://github.com/SigNoz/signoz.git
synced 2026-08-14 17:00:37 +01:00
a7fd14eac9
A metric label may be named after a column the generated query builds for itself, and the metrics and meter builders selected group-by columns under the label's own name — so `group by ts` or `group by value` produced SQL with two columns of that name, which ClickHouse rejects. ### What - Metrics and meter now alias group-by columns `__GROUP_BY_KEY_<i>_<name>`, the scheme the logs and traces statement builders already use. - `pkg/querier/consume.go` already strips that prefix on all three read paths (time-series, scalar, raw), so API responses are unchanged. - Metrics' `ColumnExpressionFor` now returns the bare expression like the logs and traces mappers. It was the only one returning an aliased expression (`expr AS <name>`), which `agg_rewrite.go` splices inside a function argument — giving `sum(expr AS <name>)` if metrics ever grows expression aggregations. Callers alias and escape, as logs does. - The histogram pipeline derives its CTE-side query once — `le` appended last, plus the existing rate/sum rewrite — instead of mutating the query and restoring it around the whole pipeline. The final select takes the original minus `le`, so the remaining keys hold the positions their CTE aliases were built from. With a label named `ts`, before: ```sql SELECT ts, `ts`, multiIf(…) … GROUP BY fingerprint, ts, `ts` ``` and after: ```sql SELECT ts, `__GROUP_BY_KEY_0_ts`, multiIf(…) … ``` A label named `value` was the quieter case — the spatial CTE selected it next to the aggregate of the same name: ```sql SELECT ts, `value`, sum(per_series_value) AS value … ``` ### Notes - Meter comes along because it holds a `*metricsstatementbuilder.StatementBuilder` and calls the shared `BuildFinalSelect`; aliasing metrics alone would leave meter ordering by an alias its own select never produced. `GroupByColumnAlias` / `GroupByAliases` are exported for it, alongside the `GetKeySelectors` / `RateTmpl` already shared across that boundary. - Meter had the identical collision, so this fixes it there too. ### Testing - `TestGroupByAliasAvoidsColumnCollision` covers `ts`, `value`, `fingerprint` and an ordinary label, in both the metrics and meter builders; all three collision cases fail without the change. - `reduced_test.go` gains `histogram_p99_group_by` and `gauge_avg_avg_group_by` — the reduced path had no group-by coverage at all, so neither the aliases in its four CTE builders nor the union's `ORDER BY` were exercised. The histogram case pins that both `UNION ALL` branches emit the same columns. - `test_histogram_count_no_param` pins the `SELECT *` branch, where `le` stays unaliased so `ORDER BY toFloat64(le)` resolves. - Both new behaviours were mutation-checked: appending `le` first instead of last, and returning the bare name from `GroupByColumnAlias`, each turn the relevant tests red. - Twelve expected-SQL blobs regenerated across the metrics and meter statement builder tests — alias-only diffs. - `go test ./...` green, `make go-lint` clean. Fixes https://github.com/SigNoz/engineering-pod/issues/5868