mirror of
https://github.com/SigNoz/signoz.git
synced 2026-08-14 00:40:35 +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
258 lines
12 KiB
Go
258 lines
12 KiB
Go
package meterstatementbuilder
|
|
|
|
import (
|
|
"context"
|
|
"fmt"
|
|
"strings"
|
|
"testing"
|
|
"time"
|
|
|
|
"github.com/SigNoz/signoz/pkg/flagger/flaggertest"
|
|
"github.com/SigNoz/signoz/pkg/instrumentation/instrumentationtest"
|
|
"github.com/SigNoz/signoz/pkg/statementbuilder/metricsstatementbuilder"
|
|
"github.com/SigNoz/signoz/pkg/telemetryschema/metricstelemetryschema"
|
|
"github.com/SigNoz/signoz/pkg/types/metrictypes"
|
|
qbtypes "github.com/SigNoz/signoz/pkg/types/querybuildertypes/querybuildertypesv5"
|
|
"github.com/SigNoz/signoz/pkg/types/telemetrytypes"
|
|
"github.com/SigNoz/signoz/pkg/types/telemetrytypes/telemetrytypestest"
|
|
"github.com/SigNoz/signoz/pkg/valuer"
|
|
"github.com/stretchr/testify/assert"
|
|
"github.com/stretchr/testify/require"
|
|
)
|
|
|
|
func TestStatementBuilder(t *testing.T) {
|
|
cases := []struct {
|
|
name string
|
|
requestType qbtypes.RequestType
|
|
query qbtypes.QueryBuilderQuery[qbtypes.MetricAggregation]
|
|
expected qbtypes.Statement
|
|
expectedErr error
|
|
}{
|
|
{
|
|
name: "test_cumulative_rate_sum",
|
|
requestType: qbtypes.RequestTypeTimeSeries,
|
|
query: qbtypes.QueryBuilderQuery[qbtypes.MetricAggregation]{
|
|
Signal: telemetrytypes.SignalMetrics,
|
|
StepInterval: qbtypes.Step{Duration: 24 * time.Hour},
|
|
Aggregations: []qbtypes.MetricAggregation{
|
|
{
|
|
MetricName: "signoz_calls_total",
|
|
Type: metrictypes.SumType,
|
|
Temporality: metrictypes.Cumulative,
|
|
TimeAggregation: metrictypes.TimeAggregationRate,
|
|
SpaceAggregation: metrictypes.SpaceAggregationSum,
|
|
},
|
|
},
|
|
Filter: &qbtypes.Filter{
|
|
Expression: "service.name = 'cartservice'",
|
|
},
|
|
Limit: 10,
|
|
GroupBy: []qbtypes.GroupByKey{
|
|
{
|
|
TelemetryFieldKey: telemetrytypes.TelemetryFieldKey{
|
|
Name: "service.name",
|
|
},
|
|
},
|
|
},
|
|
},
|
|
expected: qbtypes.Statement{
|
|
Query: "WITH __temporal_aggregation_cte AS (SELECT ts, `__GROUP_BY_KEY_0_service.name`, multiIf(row_number() OVER rate_window = 1, nan, (per_series_value - lagInFrame(per_series_value, 1) OVER rate_window) < 0, per_series_value / (ts - lagInFrame(ts, 1) OVER rate_window), (per_series_value - lagInFrame(per_series_value, 1) OVER rate_window) / (ts - lagInFrame(ts, 1) OVER rate_window)) AS per_series_value FROM (SELECT fingerprint, toStartOfInterval(toDateTime(intDiv(unix_milli, 1000)), toIntervalSecond(86400)) AS ts, JSONExtractString(labels, 'service.name') AS `__GROUP_BY_KEY_0_service.name`, max(value) AS per_series_value FROM signoz_meter.distributed_samples AS points WHERE metric_name IN (?) AND unix_milli >= ? AND unix_milli < ? AND JSONExtractString(labels, 'service.name') = ? AND LOWER(temporality) LIKE LOWER(?) GROUP BY fingerprint, ts, `__GROUP_BY_KEY_0_service.name` ORDER BY fingerprint, ts) WINDOW rate_window AS (PARTITION BY fingerprint ORDER BY fingerprint, ts)), __spatial_aggregation_cte AS (SELECT ts, `__GROUP_BY_KEY_0_service.name`, sum(per_series_value) AS value FROM __temporal_aggregation_cte WHERE isNaN(per_series_value) = ? GROUP BY ts, `__GROUP_BY_KEY_0_service.name`) SELECT * FROM __spatial_aggregation_cte ORDER BY `__GROUP_BY_KEY_0_service.name`, ts",
|
|
Args: []any{"signoz_calls_total", uint64(1747785600000), uint64(1747983420000), "cartservice", "cumulative", 0},
|
|
},
|
|
expectedErr: nil,
|
|
},
|
|
{
|
|
name: "test_delta_rate_sum",
|
|
requestType: qbtypes.RequestTypeTimeSeries,
|
|
query: qbtypes.QueryBuilderQuery[qbtypes.MetricAggregation]{
|
|
Signal: telemetrytypes.SignalMetrics,
|
|
StepInterval: qbtypes.Step{Duration: 24 * time.Hour},
|
|
Aggregations: []qbtypes.MetricAggregation{
|
|
{
|
|
MetricName: "signoz_calls_total",
|
|
Type: metrictypes.SumType,
|
|
Temporality: metrictypes.Delta,
|
|
TimeAggregation: metrictypes.TimeAggregationRate,
|
|
SpaceAggregation: metrictypes.SpaceAggregationSum,
|
|
},
|
|
},
|
|
Filter: &qbtypes.Filter{
|
|
Expression: "service.name = 'cartservice'",
|
|
},
|
|
Limit: 10,
|
|
GroupBy: []qbtypes.GroupByKey{
|
|
{
|
|
TelemetryFieldKey: telemetrytypes.TelemetryFieldKey{
|
|
Name: "service.name",
|
|
},
|
|
},
|
|
},
|
|
},
|
|
expected: qbtypes.Statement{
|
|
Query: "WITH __spatial_aggregation_cte AS (SELECT toStartOfInterval(toDateTime(intDiv(unix_milli, 1000)), toIntervalSecond(86400)) AS ts, JSONExtractString(labels, 'service.name') AS `__GROUP_BY_KEY_0_service.name`, sum(value)/86400 AS value FROM signoz_meter.distributed_samples AS points WHERE metric_name IN (?) AND unix_milli >= ? AND unix_milli < ? AND JSONExtractString(labels, 'service.name') = ? AND LOWER(temporality) LIKE LOWER(?) GROUP BY ts, `__GROUP_BY_KEY_0_service.name`) SELECT * FROM __spatial_aggregation_cte ORDER BY `__GROUP_BY_KEY_0_service.name`, ts",
|
|
Args: []any{"signoz_calls_total", uint64(1747872000000), uint64(1747983420000), "cartservice", "delta"},
|
|
},
|
|
expectedErr: nil,
|
|
},
|
|
{
|
|
name: "test_delta_rate_avg",
|
|
requestType: qbtypes.RequestTypeTimeSeries,
|
|
query: qbtypes.QueryBuilderQuery[qbtypes.MetricAggregation]{
|
|
Signal: telemetrytypes.SignalMetrics,
|
|
StepInterval: qbtypes.Step{Duration: 24 * time.Hour},
|
|
Aggregations: []qbtypes.MetricAggregation{
|
|
{
|
|
MetricName: "signoz_calls_total",
|
|
Type: metrictypes.SumType,
|
|
Temporality: metrictypes.Delta,
|
|
TimeAggregation: metrictypes.TimeAggregationRate,
|
|
SpaceAggregation: metrictypes.SpaceAggregationAvg,
|
|
},
|
|
},
|
|
Filter: &qbtypes.Filter{
|
|
Expression: "service.name = 'cartservice'",
|
|
},
|
|
Limit: 10,
|
|
GroupBy: []qbtypes.GroupByKey{
|
|
{
|
|
TelemetryFieldKey: telemetrytypes.TelemetryFieldKey{
|
|
Name: "service.name",
|
|
},
|
|
},
|
|
},
|
|
},
|
|
expected: qbtypes.Statement{
|
|
Query: "WITH __temporal_aggregation_cte AS (SELECT fingerprint, toStartOfInterval(toDateTime(intDiv(unix_milli, 1000)), toIntervalSecond(86400)) AS ts, JSONExtractString(labels, 'service.name') AS `__GROUP_BY_KEY_0_service.name`, sum(value)/86400 AS per_series_value FROM signoz_meter.distributed_samples AS points WHERE metric_name IN (?) AND unix_milli >= ? AND unix_milli < ? AND JSONExtractString(labels, 'service.name') = ? AND LOWER(temporality) LIKE LOWER(?) GROUP BY fingerprint, ts, `__GROUP_BY_KEY_0_service.name` ORDER BY fingerprint, ts), __spatial_aggregation_cte AS (SELECT ts, `__GROUP_BY_KEY_0_service.name`, avg(per_series_value) AS value FROM __temporal_aggregation_cte WHERE isNaN(per_series_value) = ? GROUP BY ts, `__GROUP_BY_KEY_0_service.name`) SELECT * FROM __spatial_aggregation_cte ORDER BY `__GROUP_BY_KEY_0_service.name`, ts",
|
|
Args: []any{"signoz_calls_total", uint64(1747872000000), uint64(1747983420000), "cartservice", "delta", 0},
|
|
},
|
|
expectedErr: nil,
|
|
},
|
|
{
|
|
name: "test_gauge_avg_sum",
|
|
requestType: qbtypes.RequestTypeTimeSeries,
|
|
query: qbtypes.QueryBuilderQuery[qbtypes.MetricAggregation]{
|
|
Signal: telemetrytypes.SignalMetrics,
|
|
StepInterval: qbtypes.Step{Duration: 24 * time.Hour},
|
|
Aggregations: []qbtypes.MetricAggregation{
|
|
{
|
|
MetricName: "system.memory.usage",
|
|
Type: metrictypes.GaugeType,
|
|
Temporality: metrictypes.Unspecified,
|
|
TimeAggregation: metrictypes.TimeAggregationAvg,
|
|
SpaceAggregation: metrictypes.SpaceAggregationSum,
|
|
},
|
|
},
|
|
Filter: &qbtypes.Filter{
|
|
Expression: "host.name = 'big-data-node-1'",
|
|
},
|
|
Limit: 10,
|
|
GroupBy: []qbtypes.GroupByKey{
|
|
{
|
|
TelemetryFieldKey: telemetrytypes.TelemetryFieldKey{
|
|
Name: "host.name",
|
|
},
|
|
},
|
|
},
|
|
},
|
|
expected: qbtypes.Statement{
|
|
Query: "WITH __temporal_aggregation_cte AS (SELECT fingerprint, toStartOfInterval(toDateTime(intDiv(unix_milli, 1000)), toIntervalSecond(86400)) AS ts, JSONExtractString(labels, 'host.name') AS `__GROUP_BY_KEY_0_host.name`, avg(value) AS per_series_value FROM signoz_meter.distributed_samples AS points WHERE metric_name IN (?) AND unix_milli >= ? AND unix_milli < ? AND JSONExtractString(labels, 'host.name') = ? AND LOWER(temporality) LIKE LOWER(?) GROUP BY fingerprint, ts, `__GROUP_BY_KEY_0_host.name` ORDER BY fingerprint, ts), __spatial_aggregation_cte AS (SELECT ts, `__GROUP_BY_KEY_0_host.name`, sum(per_series_value) AS value FROM __temporal_aggregation_cte WHERE isNaN(per_series_value) = ? GROUP BY ts, `__GROUP_BY_KEY_0_host.name`) SELECT * FROM __spatial_aggregation_cte ORDER BY `__GROUP_BY_KEY_0_host.name`, ts",
|
|
Args: []any{"system.memory.usage", uint64(1747872000000), uint64(1747983420000), "big-data-node-1", "unspecified", 0},
|
|
},
|
|
expectedErr: nil,
|
|
},
|
|
}
|
|
|
|
fm := metricstelemetryschema.NewFieldMapper()
|
|
cb := metricstelemetryschema.NewConditionBuilder(fm)
|
|
mockMetadataStore := telemetrytypestest.NewMockMetadataStore()
|
|
keys, err := telemetrytypestest.LoadFieldKeysFromJSON("testdata/keys_map.json")
|
|
if err != nil {
|
|
t.Fatalf("failed to load field keys: %v", err)
|
|
}
|
|
mockMetadataStore.KeysMap = keys
|
|
|
|
flagger := flaggertest.New(t)
|
|
|
|
metricStmtBuilder := metricsstatementbuilder.NewMetricQueryStatementBuilder(instrumentationtest.New().ToProviderSettings(), mockMetadataStore, fm, cb, flagger)
|
|
|
|
statementBuilder := NewMeterQueryStatementBuilder(
|
|
instrumentationtest.New().ToProviderSettings(),
|
|
mockMetadataStore,
|
|
fm,
|
|
cb,
|
|
metricStmtBuilder,
|
|
)
|
|
|
|
for _, c := range cases {
|
|
t.Run(c.name, func(t *testing.T) {
|
|
|
|
q, err := statementBuilder.Build(context.Background(), valuer.UUID{}, 1747947419000, 1747983448000, c.requestType, c.query, nil)
|
|
|
|
if c.expectedErr != nil {
|
|
require.Error(t, err)
|
|
require.Contains(t, err.Error(), c.expectedErr.Error())
|
|
} else {
|
|
require.NoError(t, err)
|
|
require.Equal(t, c.expected.Query, q.Query)
|
|
require.Equal(t, c.expected.Args, q.Args)
|
|
require.Equal(t, c.expected.Warnings, q.Warnings)
|
|
}
|
|
})
|
|
}
|
|
}
|
|
|
|
func TestGroupByAliasAvoidsColumnCollision(t *testing.T) {
|
|
fm := metricstelemetryschema.NewFieldMapper()
|
|
cb := metricstelemetryschema.NewConditionBuilder(fm)
|
|
mockMetadataStore := telemetrytypestest.NewMockMetadataStore()
|
|
keys, err := telemetrytypestest.LoadFieldKeysFromJSON("testdata/keys_map.json")
|
|
require.NoError(t, err)
|
|
mockMetadataStore.KeysMap = keys
|
|
|
|
flagger := flaggertest.New(t)
|
|
|
|
statementBuilder := NewMeterQueryStatementBuilder(
|
|
instrumentationtest.New().ToProviderSettings(),
|
|
mockMetadataStore,
|
|
fm,
|
|
cb,
|
|
metricsstatementbuilder.NewMetricQueryStatementBuilder(instrumentationtest.New().ToProviderSettings(), mockMetadataStore, fm, cb, flagger),
|
|
)
|
|
|
|
for _, groupBy := range []string{"ts", "value", "fingerprint", "service.name"} {
|
|
t.Run(groupBy, func(t *testing.T) {
|
|
stmt, err := statementBuilder.Build(
|
|
context.Background(),
|
|
valuer.UUID{},
|
|
1747947419000,
|
|
1747983448000,
|
|
qbtypes.RequestTypeTimeSeries,
|
|
qbtypes.QueryBuilderQuery[qbtypes.MetricAggregation]{
|
|
Signal: telemetrytypes.SignalMetrics,
|
|
StepInterval: qbtypes.Step{Duration: 24 * time.Hour},
|
|
Aggregations: []qbtypes.MetricAggregation{
|
|
{
|
|
MetricName: "signoz_calls_total",
|
|
Type: metrictypes.SumType,
|
|
Temporality: metrictypes.Cumulative,
|
|
TimeAggregation: metrictypes.TimeAggregationRate,
|
|
SpaceAggregation: metrictypes.SpaceAggregationSum,
|
|
},
|
|
},
|
|
GroupBy: []qbtypes.GroupByKey{
|
|
{TelemetryFieldKey: telemetrytypes.TelemetryFieldKey{Name: groupBy}},
|
|
},
|
|
},
|
|
nil,
|
|
)
|
|
require.NoError(t, err)
|
|
|
|
assert.Contains(t, stmt.Query, fmt.Sprintf("`__GROUP_BY_KEY_0_%s`", groupBy))
|
|
assert.NotContains(t, stmt.Query, fmt.Sprintf("`%s`", groupBy),
|
|
"the group-by column must not be selected under the label's own name")
|
|
assert.Equal(t, 1, strings.Count(stmt.Query, " AS ts,"),
|
|
"the step bucket is the only column named ts")
|
|
})
|
|
}
|
|
}
|