<!--A few plain bullets saying what changed and why, for a reviewer
skimming it - not a wall of text, not a restatement of the diff, not
generated boilerplate.-->
#### Description
Heatmap support here is only for metrics (except exponential histograms)
via all three query types: builder, clickhouse and promql. Logs and
traces can be plugged in into this later.
<!--Reference issues using `Closes #issue-number` to enable automatic
closure on merge. -->
#### Issues closed by this PR
Closes https://github.com/SigNoz/pulse-pod/issues/311
#### Description
- Every user-controlled field name that reaches generated SQL goes
through the new `pkg/clickhousesql` package (`Identifier`,
`StringLiteral`, `Literal`, `LikePattern`): map reads and `mapContains`,
JSON sub-column paths and the JSON body access plan, labels, fingerprint
labels, materialized column names, select aliases, group-by and order-by
references, the legacy string-body JSONPath, and the raw SQL in the
trace funnel, trace detail and infra monitoring modules. Filter
expressions built from request or telemetry values use
`querybuilder.FilterStringLiteral`. The same package now also renders
dashboard variable values in the querier, LIKE patterns in the metadata
store and label lists in the PromQL transpiler, which each had their own
escaping.
- A `$` followed by a digit, `{` or `?` is written as `\x24`, which
ClickHouse decodes in identifiers and literals. Those are the forms the
tools react to: go-sqlbuilder resolves `$0` in a compiled fragment to
its own WHERE clause and recurses until the stack overflows, and
clickhouse-go rejects a query mixing `$<digits>` with `?` arguments. Any
other `$` stays literal, so materialized column names keep their `$$`
and render exactly as before; a key like `http.2xx` becomes ``
`attribute_string_http$\x242xx` `` instead of failing in the driver.
- Compiled sqlbuilder fragments (Select, GroupBy, OrderBy, raw Where
text) are wrapped with `sqlbuilder.Escape`; the metrics builder escapes
its compiled time-series subquery, which is compiled a second time when
joined.
- The raw statement validator (`ErrIfStatementIsNotValid`,
`LogIfStatementIsNotValid`) moves from
`pkg/querybuilder/clickhouse_sql.go` to
`pkg/clickhousesql/statement.go`. Its `Code*` identifiers drop the
`ClickHouseSQL` prefix; the code strings are unchanged.
- Unit tests round-trip the helpers over hostile names and drive them
through the modules' raw SQL;
`tests/integration/tests/queriercommon/08_field_name_quoting.py` and
`querier_json_body/07_field_name_quoting.py` query such names through
the logs, traces and metrics builders against a real ClickHouse.
#### Additional Information
- `docs/contributing/go/clickhousesql.md` documents the quoting
functions, where `sqlbuilder.Escape` belongs, the `$` rule and the
statement validator; `.claude/rules/go-contrib.md` points at it.
- `pkg/clickhousesql` is a leaf package so `telemetrytypes` (JSON access
plan) and `querybuilder` share one implementation without a cycle.
- For names without special characters the generated SQL is byte
identical.
- Not covered here: the legacy v3/v4 query_range builders and the
`pkg/query-service/utils` quoting helpers (`QuoteEscapedString`,
`QuoteEscapedStringForContains`, `ClickHouseFormattedValue`,
`AddBackTickToFormatTag`), the collector's `JSONSubColumnIndexExpr`, and
aggregation arguments naming a key that contains a backtick (rejected by
the SQL parser, a 500 as before).
🤖 Generated with [Claude Code](https://claude.com/claude-code)
<!--A few plain bullets saying what changed and why, for a reviewer
skimming it - not a wall of text, not a restatement of the diff, not
generated boilerplate.-->
#### Description
Column for exponential histograms is not decided by samples tables so it
should not run for exp histogrms
<!--Reference issues using `Closes #issue-number` to enable automatic
closure on merge. -->
#### Issues closed by this PR
Closes https://github.com/SigNoz/pulse-pod/issues/295
#### Description
- Every label lives in the `labels` JSON and reads back as `String`
whatever data type the metadata claims, so `success = true` compared
`String` with `Bool` and failed the whole query with ClickHouse error
386. The read is now cast with `accurateCastOrNull(..., 'Bool')`, which
also matches the `1`/`True` spellings exporters write.
- `IN`/`NOT IN` expand into `=`/`!=` chains like the logs and traces
condition builders already do. The driver binds `IN (?)` as a single
array literal, which needs one common supertype across the set, so a set
mixing text with numbers or bools failed the same way. Each value is now
type-matched on its own.
- Intrinsic columns keep their own type and are compared as they are,
which also stops `toFloat64OrNull()` being applied to
`unix_milli`/`fingerprint` (error 43).
#### Additional Information
- A label value that isn't boolean text casts to NULL and so matches
neither side of the comparison —
`tests/integration/tests/queriermetrics/13_bool_label_filter.py` asserts
that, alongside the statement-builder unit tests.
- `BETWEEN` takes its cast from the lower bound: the where-clause
visitor already rejects mixed-type operands (and bool ones outright), so
both bounds are the same number-or-string type by the time the condition
builder sees them.
A filter on a metrics label that isn't in metadata ran silently: the
query fell back to reading the label directly, but nothing told the user
the key was unknown. Removes the `TODO(srikanthccv)` in the metrics
statement builder.
### What
The detection was already written, and already in the right place.
`conditionBuilder.ConditionFor` spots a filter key with no metadata
match, warns, and synthesizes an attribute-context key so the query
still runs — and it only ever sees terms in **key position**, so it
cannot mistake a value or a dashboard variable for a key. That is
exactly what the TODO was waiting for.
Two things hid it:
- `Build` pre-seeded the field-key map with a synthesized entry for
every lexer-derived selector, so `MatchingFieldKeys` always matched and
the missing-key branch was dead code.
- The metrics builder never read `PrepareWhereClause`'s warnings — the
visitor collects them and `unionStatements` merges them, but nothing
ever set them.
So: drop the pre-seeding, and carry the warnings out of
`buildTimeSeriesCTE` onto the statement.
### Notes
- **Generated SQL is unchanged.** The key the condition builder
synthesizes (attribute context, name as written) is what the pre-seeding
was injecting, so `test_missing_key_falls_back_to_labels` still expects
byte-identical SQL and only gains the warning.
- A full-text term routes through `labels`, which is a real column, so
it takes the `isColumn` branch and stays silent — bare-word searches
don't start warning.
- The reduced statement prepares the same filter over the same keys, so
only the main path's warnings go into the union; carrying both would
show each warning twice.
### Testing
- `test_missing_key_falls_back_to_labels` gains the expected warning,
same SQL.
-
`queriermetrics/10_key_resolution.py::test_metrics_filter_unknown_label_matches_nothing`
now asserts the warning instead of asserting silence.
- `queriermetrics/02_warnings.py` already covered the TODO's own example
(`my_tag = $tag`). It passed before only because every warning was
suppressed; it is now a real guard that a value-position variable is not
flagged.
- `queriermetrics` integration suite: 118 passed. `go test
./pkg/statementbuilder/... ./pkg/telemetryschema/...` green. `make
go-lint` and `make py-lint` clean.
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
primus bumped golangci-lint to v2.12.2, whose govet now runs the inline
analyzer and whose sloglint is stricter. CI resolves primus.workflows@main,
so every PR started failing lint the moment that landed.
- reflect.Ptr is a deprecated alias carrying //go:fix inline, so it is now
reflect.Pointer at all five call sites
- metricsstatementbuilder imported golang.org/x/exp/slices, which carries
//go:fix inline pointing at the stdlib; the analyzer cannot inline
generics, so switch the import to stdlib slices as the directive intends
- pkg/instrumentation/loghandler emits OpenTelemetry semantic-convention
attributes (code.filepath, exception.type, ...), which are dotted rather
than snake_case by definition. Renaming them would break every log
consumer, so the keys move to constants in instrumentationtypes, which
already held this kind of key -- and already defined code.function, so
source.go was duplicating it. Six of the seven alias the semconv
constants that define them; exception.code has no OTel equivalent.
sloglint resolves a same-package constant back to its literal but skips
a qualified one, so this needs no exclusion.
CI reported 8 issues but capped at max-same-issues=3, hiding 2 more
reflect.Ptr sites and 4 more sloglint ones.
Separately, TestTimeout/WaitTillNoTimeoutForExcludedPath failed with
"transport connection broken: http: CloseIdleConnections called".
TestTimeout and TestCache issue requests through http.DefaultClient while
a parallel subtest in response_test.go closes an httptest.Server, and
httptest.Server.Close calls http.DefaultTransport.CloseIdleConnections.
Both tests now use their own client and transport, and are closed via
t.Cleanup. That makes Serve return ErrServerClosed on every run, so the
require.NoError wrapping it is dropped -- it could never have held, and
require runs t.FailNow off the test goroutine anyway. Bare Serve in a
goroutine matches routerweb and render tests.
Remove the statementbuilder.Builders aggregate. The querier chain (querier.New,
signozquerier.NewFactory, NewQuerierProviderFactories) now takes the six per-signal
statement builders individually, and signoz.go::newQueryStack returns them directly
via a plain multi-return. The statementbuilder package is now config-only.
Remove each <signal>statementbuilder/new.go and fold its NewFactory into that
package's statement_builder.go (traces' NewOperatorFactory into
trace_operator_statement_builder.go), giving the layout: struct -> NewFactory ->
New<X>QueryStatementBuilder.
Split the five telemetry<signal> packages, which mixed three concerns, into
three per-signal layers with a cycle-free dependency direction:
- telemetryschema/<signal>telemetryschema — primitives (const + table
selection, field_mapper, condition_builder, trace helpers); leaf layer.
- statementbuilder/<signal>statementbuilder — SQL generation. The parent
statementbuilder package is contract-only (the Builders bundle + Config);
each sub-package exposes a factory.ProviderFactory[..., statementbuilder.Config]
whose New internalizes FieldMapper/ConditionBuilder/AggExprRewriter and reads
SkipResourceFingerprint. Traces exposes two factories (query + operator).
telemetryresourcefilter moves here as statementbuilder/resourcefilter.
- telemetrymetadata — key/value resolution; NewTelemetryMetaStore collapses
from 24 args to (settings, telemetrystore, flagger), sourcing table names
from the schema constants.
Centralize query-stack assembly in signoz.go via newQueryStack: build the
single metadata store, run each per-signal statement-builder factory, assemble
the statementbuilder.Builders bundle, and build the bucket cache — once. This
is the only place that imports the concrete sub-packages (so the edge runs
subs -> parent, cycle-free), and it removes the duplicate metadata store that
signozquerier used to build, leaving signozquerier a thin
querier.New(*statementbuilder.Builders) adapter.
Also:
- statementbuilder.Config owns SkipResourceFingerprint (moved off
querier.Config). YAML key moves querier.skip_resource_fingerprint ->
statementbuilder.skip_resource_fingerprint.
- Querier interface moves into querier.go (interfaces.go removed); BucketCache
-> bucket_cache.go, Handler -> api.go.
- Add pkg/querier/queriertest.MockQuerier.
