Make ast transformations skippable#7012
Conversation
AST optimizations can be skipped via hints or global config
| } | ||
|
|
||
| rootExpr, _, _, _, spanRequest, err := traceql.Compile(traceQLQuery) | ||
| rootExpr, _, _, _, spanRequest, err := traceql.Compile(traceQLQuery, traceql.WithSkipOptimization(traceql.TransformationAll)) |
There was a problem hiding this comment.
I'm unsure about this one: I think it actually makes more sense to calculate the weight based on the optimized query.
On the other hand: this compilation is not configurable via skip_ast_transformations and unsafe_query_hints
There was a problem hiding this comment.
Pull request overview
This PR makes TraceQL AST rewrites/optimizations selectively skippable (via per-request hints and a query-frontend global config), and plumbs the skip list through HTTP/proto requests into the TraceQL compiler/engine.
Changes:
- Introduces named AST transformations + a unified
CompileOptionAPI to control rewrite skipping and unsafe-hint handling across parse/compile/search/metrics compilation paths. - Adds
skip_ast_transformationspropagation through HTTP query params and proto fields for search and query-range requests, with frontend-side merge/dedup behavior. - Updates querier/livestore/frontend code paths and tests/benchmarks to use the new APIs and to verify propagation/merging.
Reviewed changes
Copilot reviewed 36 out of 36 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tempodb/tempodb_search_test.go | Updates ExecuteSearch calls to new signature. |
| tempodb/tempodb_metrics_test.go | Updates metrics compile option type usage. |
| tempodb/encoding/vparquet5/block_traceql_test.go | Updates ExecuteSearch + unsafe hint option rename in benchmarks. |
| tempodb/encoding/vparquet4/block_traceql_test.go | Updates ExecuteSearch calls to new signature in benchmarks. |
| tempodb/encoding/vparquet3/block_traceql_test.go | Updates ExecuteSearch calls to new signature in benchmarks. |
| pkg/traceql/parse_test.go | Adjusts parse rewrite tests to use the new unsafe-hints flow. |
| pkg/traceql/parse.go | Reworks parsing API to support skip lists + unsafe hint control; adds ParseNoOptimizations. |
| pkg/traceql/options.go | Adds shared CompileOption plumbing for parse/compile/engine + metrics compilation. |
| pkg/traceql/lenient_parser.go | Switches lenient parsing to ParseNoOptimizations. |
| pkg/traceql/lenient_extract_test.go | Updates tests to use ParseNoOptimizations. |
| pkg/traceql/fake_data_test.go | Updates ExecuteSearch calls to pass WithUnsafeHints(true) option. |
| pkg/traceql/enum_hints.go | Replaces boolean skip hint with string-slice skip list hint + helper accessor. |
| pkg/traceql/engine_test.go | Updates ExecuteSearch calls to new signature. |
| pkg/traceql/engine_metrics.go | Unifies metrics compile options; threads options through Compile calls; renames unsafe option. |
| pkg/traceql/engine.go | Unifies Compile/ExecuteSearch APIs around CompileOption and unsafe-hint gating. |
| pkg/traceql/ast_stringer_test.go | Switches to ParseNoOptimizations for stable stringify round-trips. |
| pkg/traceql/ast_rewriter_test.go | Updates rewrite test to use new Parse API. |
| pkg/traceql/ast_rewriter.go | Introduces named transformations registry, validation helpers, and skip-aware rewrite application. |
| pkg/tempopb/tempo.proto | Adds skipASTTransformations fields to SearchRequest and QueryRangeRequest. |
| pkg/tempopb/tempo.pb.go | Regenerates Go proto for new skip list fields. |
| pkg/api/http.go | Adds skip_ast_transformations query param parsing + request building support. |
| modules/querier/querier_query_range.go | Parses without rewrites for hint inspection; applies skip list via compile options. |
| modules/querier/querier.go | Applies per-request skip list + unsafe hint gating when executing TraceQL searches. |
| modules/livestore/instance_search.go | Parses without rewrites for hint inspection; applies skip list via compile options for TraceQL paths. |
| modules/frontend/search_sharder_test.go | Updates expected URIs and adds merge/propagation tests for skip list. |
| modules/frontend/search_sharder.go | Adds global skip list support, merges/dedupes per-request skip list, includes it in request hashes. |
| modules/frontend/search_handlers.go | Uses ParseNoOptimizations when only inspecting hints/query structure. |
| modules/frontend/pipeline/async_weight_middleware.go | Adjusts compilation used for weight computation to avoid AST rewrites. |
| modules/frontend/pipeline/async_query_validator_middleware.go | Uses ParseNoOptimizations before validation. |
| modules/frontend/metrics_query_range_sharder.go | Adds global skip list support and propagates it into backend query-range requests + hashes. |
| modules/frontend/metrics_query_range_handler.go | Uses ParseNoOptimizations for exemplar normalization logic. |
| modules/frontend/mcp_tools.go | Uses ParseNoOptimizations for MCP query parsing. |
| modules/frontend/frontend.go | Wires global SkipASTTransformations config into sharders. |
| modules/frontend/config.go | Adds query_frontend.skip_ast_transformations config field. |
| cmd/tempo/app/config.go | Adds startup warning for unknown transformation names in config. |
| CHANGELOG.md | Adds a changelog entry for skippable AST transformations. |
| for _, name := range strings.Split(s, ",") { | ||
| if name = strings.TrimSpace(name); name != "" { | ||
| req.SkipASTTransformations = append(req.SkipASTTransformations, name) | ||
| } |
There was a problem hiding this comment.
MEDIUM: skip_ast_transformations values from the URL are accepted without validation, so typos/unknown transformation names will be silently ignored (and can also create duplicate entries). Could we validate each name with traceql.IsValidTransformationName (allowing "all") and return a 400 (or at least drop invalid names with a clear error)? The same pattern appears in ParseQueryRangeRequest too.
| for _, name := range strings.Split(s, ",") { | |
| if name = strings.TrimSpace(name); name != "" { | |
| req.SkipASTTransformations = append(req.SkipASTTransformations, name) | |
| } | |
| seen := make(map[string]struct{}) | |
| for _, name := range strings.Split(s, ",") { | |
| name = strings.TrimSpace(name) | |
| if name == "" { | |
| continue | |
| } | |
| if name != "all" && !traceql.IsValidTransformationName(name) { | |
| return nil, fmt.Errorf("invalid %s: unknown transformation %q", urlParamSkipASTTransformations, name) | |
| } | |
| if _, ok := seen[name]; ok { | |
| continue | |
| } | |
| seen[name] = struct{}{} | |
| req.SkipASTTransformations = append(req.SkipASTTransformations, name) |
There was a problem hiding this comment.
This is actually fine
| } | ||
|
|
||
| rootExpr, _, _, _, spanRequest, err := traceql.Compile(traceQLQuery) | ||
| rootExpr, _, _, _, spanRequest, err := traceql.Compile(traceQLQuery, traceql.WithSkipOptimization(traceql.TransformationAll)) |
There was a problem hiding this comment.
MEDIUM: setTraceQLWeight compiles with TransformationAll skipped, which prevents the OR→IN rewrite from running. That can leave spanRequest.AllConditions=false for queries that would normally be rewritten into storage-optimizable IN/MATCH ANY forms, inflating the computed weight and potentially throttling more than necessary. Is that conservatism intentional? If not, consider compiling with the same skip list used for execution (global cfg + request skip list) so the weight reflects the actual plan.
| rootExpr, _, _, _, spanRequest, err := traceql.Compile(traceQLQuery, traceql.WithSkipOptimization(traceql.TransformationAll)) | |
| rootExpr, _, _, _, spanRequest, err := traceql.Compile(traceQLQuery) |
What this PR does:
AST optimizations can be skipped via hints or global config
Which issue(s) this PR fixes:
Fixes grafana/tempo-squad#1054
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]