Compare commits

...

1 Commits

Author SHA1 Message Date
Tushar Vats
acc2c93bc7 fix(metrics): warn when a filtered label is missing from metadata
The metrics condition builder already detects a filter term whose key has no
metadata match, warns, and synthesizes an attribute-context key so the query
still runs. It only ever sees terms in key position, so it cannot mistake a
value or a dashboard variable for a key.

Two things hid it. Build pre-seeded the field-key map with a synthesized entry
for every lexer-derived selector, so the lookup always matched and the warning
branch was dead. And the builder never read PrepareWhereClause's warnings, so
nothing reached the statement.

Drop the pre-seeding and carry the warnings out of buildTimeSeriesCTE. The
reduced statement prepares the same filter over the same keys, so only the main
path's warnings go into the union. Generated SQL is unchanged: the key the
condition builder synthesizes matches what the pre-seeding was injecting.

A full-text term routes through `labels`, a real column, so it takes the column
branch and stays silent.
2026-08-10 11:33:46 +05:30
3 changed files with 14 additions and 27 deletions

View File

@@ -123,23 +123,6 @@ func (b *StatementBuilder) Build(
return nil, err
}
// TODO(srikanthccv): move the missing-key detection into the where clause
// visitor. Doing it here over the lexer-derived selectors can't tell a key
// from a value, so dashboard variables and bare literals in value position
// (e.g. `service.name = $service`) get flagged as missing keys. We still add
// a labels fallback for any unresolved selector so the query can be built,
// but we no longer emit a warning until the visitor can classify keys.
for _, sel := range keySelectors {
if _, ok := keys[sel.Name]; !ok {
keys[sel.Name] = []*telemetrytypes.TelemetryFieldKey{{
Name: sel.Name,
FieldContext: telemetrytypes.FieldContextAttribute,
FieldDataType: telemetrytypes.FieldDataTypeString,
Signal: telemetrytypes.SignalMetrics,
}}
}
}
start, end = querybuilder.AdjustedMetricTimeRange(start, end, uint64(query.StepInterval.Seconds()), query)
return b.buildPipelineStatement(ctx, orgID, start, end, query, keys, variables)
@@ -215,9 +198,10 @@ func (b *StatementBuilder) buildPipelineStatement(
var timeSeriesCTE string
var timeSeriesCTEArgs []any
var filterWarnings []string
var err error
if timeSeriesCTE, timeSeriesCTEArgs, err = b.buildTimeSeriesCTE(ctx, orgID, tsStart, tsEnd, query, keys, variables, tsTable); err != nil {
if timeSeriesCTE, timeSeriesCTEArgs, filterWarnings, err = b.buildTimeSeriesCTE(ctx, orgID, tsStart, tsEnd, query, keys, variables, tsTable); err != nil {
return nil, err
}
@@ -277,6 +261,7 @@ func (b *StatementBuilder) buildPipelineStatement(
if err != nil {
return nil, err
}
mainStmt.Warnings = append(mainStmt.Warnings, filterWarnings...)
if reducedFragments == nil {
return mainStmt, nil
}
@@ -524,7 +509,7 @@ func (b *StatementBuilder) buildTimeSeriesCTE(
keys map[string][]*telemetrytypes.TelemetryFieldKey,
variables map[string]qbtypes.VariableItem,
tsTable string,
) (string, []any, error) {
) (string, []any, []string, error) {
sb := sqlbuilder.NewSelectBuilder()
var preparedWhereClause querybuilder.PreparedWhereClause
@@ -544,7 +529,7 @@ func (b *StatementBuilder) buildTimeSeriesCTE(
EndNs: end,
})
if err != nil {
return "", nil, err
return "", nil, nil, err
}
}
@@ -554,7 +539,7 @@ func (b *StatementBuilder) buildTimeSeriesCTE(
for _, g := range query.GroupBy {
col, err := b.fm.ColumnExpressionFor(ctx, orgID, start, end, &g.TelemetryFieldKey, telemetrytypes.FieldDataTypeString, keys)
if err != nil {
return "", nil, err
return "", nil, nil, err
}
sb.SelectMore(col)
}
@@ -583,7 +568,7 @@ func (b *StatementBuilder) buildTimeSeriesCTE(
sb.GroupBy(querybuilder.GroupByKeys(query.GroupBy)...)
q, args := sb.BuildWithFlavor(sqlbuilder.ClickHouse)
return fmt.Sprintf("(%s) AS filtered_time_series", q), args, nil
return fmt.Sprintf("(%s) AS filtered_time_series", q), args, preparedWhereClause.Warnings, nil
}
func (b *StatementBuilder) buildTemporalAggregationCTE(

View File

@@ -246,8 +246,9 @@ func TestStatementBuilder(t *testing.T) {
},
},
expected: qbtypes.Statement{
Query: "WITH __temporal_aggregation_cte AS (SELECT ts, `k8s.statefulset.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(30)) AS ts, `k8s.statefulset.name`, max(value) AS per_series_value FROM signoz_metrics.distributed_samples_v4 AS points INNER JOIN (SELECT fingerprint, JSONExtractString(labels, 'k8s.statefulset.name') AS `k8s.statefulset.name` FROM signoz_metrics.time_series_v4_6hrs WHERE metric_name IN (?) AND unix_milli >= ? AND unix_milli <= ? AND LOWER(temporality) LIKE LOWER(?) AND JSONExtractString(labels, 'k8s.statefulset.name') = ? GROUP BY fingerprint, `k8s.statefulset.name`) AS filtered_time_series ON points.fingerprint = filtered_time_series.fingerprint WHERE metric_name IN (?) AND unix_milli >= ? AND unix_milli < ? GROUP BY fingerprint, ts, `k8s.statefulset.name` ORDER BY fingerprint, ts) WINDOW rate_window AS (PARTITION BY fingerprint ORDER BY fingerprint, ts)), __spatial_aggregation_cte AS (SELECT ts, `k8s.statefulset.name`, sum(per_series_value) AS value FROM __temporal_aggregation_cte WHERE isNaN(per_series_value) = ? GROUP BY ts, `k8s.statefulset.name`) SELECT * FROM __spatial_aggregation_cte ORDER BY `k8s.statefulset.name`, ts",
Args: []any{"signoz_calls_total", uint64(1747936800000), uint64(1747983420000), "cumulative", "my-statefulset", "signoz_calls_total", uint64(1747947360000), uint64(1747983420000), 0},
Query: "WITH __temporal_aggregation_cte AS (SELECT ts, `k8s.statefulset.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(30)) AS ts, `k8s.statefulset.name`, max(value) AS per_series_value FROM signoz_metrics.distributed_samples_v4 AS points INNER JOIN (SELECT fingerprint, JSONExtractString(labels, 'k8s.statefulset.name') AS `k8s.statefulset.name` FROM signoz_metrics.time_series_v4_6hrs WHERE metric_name IN (?) AND unix_milli >= ? AND unix_milli <= ? AND LOWER(temporality) LIKE LOWER(?) AND JSONExtractString(labels, 'k8s.statefulset.name') = ? GROUP BY fingerprint, `k8s.statefulset.name`) AS filtered_time_series ON points.fingerprint = filtered_time_series.fingerprint WHERE metric_name IN (?) AND unix_milli >= ? AND unix_milli < ? GROUP BY fingerprint, ts, `k8s.statefulset.name` ORDER BY fingerprint, ts) WINDOW rate_window AS (PARTITION BY fingerprint ORDER BY fingerprint, ts)), __spatial_aggregation_cte AS (SELECT ts, `k8s.statefulset.name`, sum(per_series_value) AS value FROM __temporal_aggregation_cte WHERE isNaN(per_series_value) = ? GROUP BY ts, `k8s.statefulset.name`) SELECT * FROM __spatial_aggregation_cte ORDER BY `k8s.statefulset.name`, ts",
Args: []any{"signoz_calls_total", uint64(1747936800000), uint64(1747983420000), "cumulative", "my-statefulset", "signoz_calls_total", uint64(1747947360000), uint64(1747983420000), 0},
Warnings: []string{"label `k8s.statefulset.name` not found in metadata; check the label name for typos"},
},
expectedErr: nil,
},

View File

@@ -175,8 +175,9 @@ def test_metrics_filter_unknown_label_matches_nothing(
insert_metrics: Callable[[list[Metrics]], None],
) -> None:
"""A filter on a label no metric carries resolves to JSONExtractString(labels,'<missing>')
= '' and matches nothing: metrics returns 200 with an empty result and — unlike the
logs/traces synthesize path — emits no key-not-found warning."""
= '' and matches nothing: metrics returns 200 with an empty result, and warns that the
label is absent from metadata. Only keys are flagged — a value or dashboard variable in
value position never reaches the condition builder, so it cannot be mistaken for one."""
now = datetime.now(tz=UTC).replace(second=0, microsecond=0)
insert_metrics(
[
@@ -208,7 +209,7 @@ def test_metrics_filter_unknown_label_matches_nothing(
)
assert response.status_code == HTTPStatus.OK, response.text
assert querier.get_scalar_table_data(response.json()) == []
assert querier.get_all_warnings(response.json()) == []
assert [w["message"] for w in querier.get_all_warnings(response.json())] == ["label `does_not_exist_label` not found in metadata; check the label name for typos"]
def test_metrics_full_text_filter_does_not_error(