fix(query): port resolve_sketch_metric_alias to modern execute() (schema-retirement #5 step 2) - #274
Merged
Merged
Conversation
…chema-retirement #5 step 1) The modern execute() trait path and execute_range_promql_modern resolved ASAP-tier candidates to sids via `sids_for_policy(fp)` only, on the assumption that every production sid registration would populate `policy_fp` (per the comment retired by an earlier PR). That assumption breaks for sketches arriving via OTLP: `derive_sketch_policy_fp` only returns `Some(fp)` when a streaming-config policy's `grouping_labels` EXACTLY matches the wire DP's `group_by_keys`. When the agent emits sketches with the full wire-attr set (no upstream attribute reduction — the common case when the controller's OpAMP-pushed runtime config doesn't take effect, see ASAPCollector#381), `find_policy_by_content` returns `None`, all sids land in the catalog with `policy_fp = UNSET`, and `sids_for_policy(streaming_config_fp)` returns empty even though the data is sitting in the SketchStore right there. Fix: union the `instances_matching(metric, group_by_keys)` catalog walk into the sid set. `instances_matching` is the more general primitive: it returns sids whose `group_by_keys` is a SUPERSET of the candidate's asked grouping, which subsumes the policy-fp reverse-index hit (an ExactAgg-style sid minted via `ingest_precompute_for_agg_config` has `group_by_keys == streaming_config.grouping_labels` so it satisfies the subset check) AND covers the full-attr sketch case the prior path missed. End-to-end verified via the single-node MVP smoke test (`/mydata/mvp-smoke-test/`): $ curl --data-urlencode 'query=quantile_over_time(0.99, http_requests_total_latency_ms[5m])' \ http://localhost:19091/api/v1/query {"accuracy":{"delta":0.0,"epsilon":0.01,"kind":"relative_quantile"}, "data":{"result":[{"metric":{"zone":""}, "value":[1779043268.182,"90.93548893834691"]}], "resultType":"vector"}, "infos":["accuracy: ε=0.01, δ=0, kind=relative_quantile", ...]} Pre-fix this returned `{"data":null,"error":"No result for query"}`. Smoke-test `D` axis already showed 51 sids registered for the metric; the gap was purely the query path's sid-resolution step. Test plan: * `cargo test -p data_plane --lib` 756/756 green (756 pass / 5 ignored) * `cargo test -p control_plane --lib` 691/691 green * New regression test `full_attr_sketch_sid_findable_via_subset_grouping` in the `asap_tier_classify_tests` module pins the smoke-test scenario at unit level — a sketch sid with `policy_fp = UNSET` and full wire-attr `group_by_keys` must be findable by a query whose `group_by_keys` is a subset. * End-to-end smoke test (`bash /mydata/mvp-smoke-test/run_smoke.sh`) Axis C `quantile_over_time` now returns a real DDSketch quantile; pre-fix it returned "No result for query". Scope: this is step 1 of [#272 (schema-retirement #5)](#272). The ingest bucketing (`otel.rs:561`, `WorkerMessage::AccumulatorInput`), precompute output sink, and backfill processor still key on `config.aggregation_id()`; those are separate per-subsystem retirements tracked in the same issue. The legacy `handle_query` / `execute_context` path also still routes through agg_id and the "No precomputed outputs found for metric: X, aggregation_id: Y" error message — its retirement is the next step. Closes the sid-resolution sub-step of #272. Related: issue #271 (MVP demo axis C) — sister fix path 2. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ema-retirement #5 step 2) Continuing #272 retirement work. The agent-side `_quantile` / `_hll` INGEST-time metric renames (DDSketch+KLL append `_quantile`, HLL appends `_hll`) used to be handled only at the entry of the legacy `handle_query_promql` path. With #273 making modern `execute()` and `execute_range_promql_modern` the producing paths for the common sketch-backed-query case, those paths also need the rename rewrite — otherwise a user query `quantile_over_time(0.99, http_latency[5m])` that lands in modern (via the legacy→modern fallback in `process_via_simple_engine`) misses the suffixed series the ASAP tier actually holds. Port: call `self.resolve_sketch_metric_alias(query).unwrap_or_else(...)` at the top of both modern execute() trait impl and `execute_range_promql_modern`. The helper is already a pure function (parse + classify shape + replace metric token); no refactor needed, just an additional caller. Tested with the smoke test (`/mydata/mvp-smoke-test/run_smoke.sh`) — the bare-metric case still works because the smoke fake-exporter emits `http_requests_total_latency_ms` literally (no suffix), and the streaming-config metric name matches, so `resolve_sketch_metric_alias` correctly no-ops via `bare_present`. Once the agent's DDSketch processor's `metric_suffix: "_quantile"` takes effect (when ASAPCollector#381 is resolved and OpAMP-pushed runtime config applies), modern's PromQL queries will need this rename to bind back to the suffixed series — this PR pre-stages it. Why not reorder process_via_simple_engine to call modern first now: the `http_capability_miss_feedback_loop_closes_over_http` test relies on the capability-miss notify side-effect inside `find_compatible_aggregation_with_miss_notify` (which only legacy calls); reordering also surfaces a pre-existing time=0 underflow bug at engine.rs:792. Modern needs its own capability-miss notify before that flip is safe — staged as the next sub-PR of #272. Test plan: * `cargo test -p data_plane --lib` 756/756 pass * `cargo test -p control_plane --lib` 691/691 pass Related: #272 (schema-retirement #5 umbrella), #273 (step 1 — sid resolution fallback). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced May 17, 2026
Closed
zzylol
added a commit
that referenced
this pull request
May 17, 2026
…#275) * fix(query): union instances_matching into ASAP-tier sid resolution (schema-retirement #5 step 1) The modern execute() trait path and execute_range_promql_modern resolved ASAP-tier candidates to sids via `sids_for_policy(fp)` only, on the assumption that every production sid registration would populate `policy_fp` (per the comment retired by an earlier PR). That assumption breaks for sketches arriving via OTLP: `derive_sketch_policy_fp` only returns `Some(fp)` when a streaming-config policy's `grouping_labels` EXACTLY matches the wire DP's `group_by_keys`. When the agent emits sketches with the full wire-attr set (no upstream attribute reduction — the common case when the controller's OpAMP-pushed runtime config doesn't take effect, see ASAPCollector#381), `find_policy_by_content` returns `None`, all sids land in the catalog with `policy_fp = UNSET`, and `sids_for_policy(streaming_config_fp)` returns empty even though the data is sitting in the SketchStore right there. Fix: union the `instances_matching(metric, group_by_keys)` catalog walk into the sid set. `instances_matching` is the more general primitive: it returns sids whose `group_by_keys` is a SUPERSET of the candidate's asked grouping, which subsumes the policy-fp reverse-index hit (an ExactAgg-style sid minted via `ingest_precompute_for_agg_config` has `group_by_keys == streaming_config.grouping_labels` so it satisfies the subset check) AND covers the full-attr sketch case the prior path missed. End-to-end verified via the single-node MVP smoke test (`/mydata/mvp-smoke-test/`): $ curl --data-urlencode 'query=quantile_over_time(0.99, http_requests_total_latency_ms[5m])' \ http://localhost:19091/api/v1/query {"accuracy":{"delta":0.0,"epsilon":0.01,"kind":"relative_quantile"}, "data":{"result":[{"metric":{"zone":""}, "value":[1779043268.182,"90.93548893834691"]}], "resultType":"vector"}, "infos":["accuracy: ε=0.01, δ=0, kind=relative_quantile", ...]} Pre-fix this returned `{"data":null,"error":"No result for query"}`. Smoke-test `D` axis already showed 51 sids registered for the metric; the gap was purely the query path's sid-resolution step. Test plan: * `cargo test -p data_plane --lib` 756/756 green (756 pass / 5 ignored) * `cargo test -p control_plane --lib` 691/691 green * New regression test `full_attr_sketch_sid_findable_via_subset_grouping` in the `asap_tier_classify_tests` module pins the smoke-test scenario at unit level — a sketch sid with `policy_fp = UNSET` and full wire-attr `group_by_keys` must be findable by a query whose `group_by_keys` is a subset. * End-to-end smoke test (`bash /mydata/mvp-smoke-test/run_smoke.sh`) Axis C `quantile_over_time` now returns a real DDSketch quantile; pre-fix it returned "No result for query". Scope: this is step 1 of [#272 (schema-retirement #5)](#272). The ingest bucketing (`otel.rs:561`, `WorkerMessage::AccumulatorInput`), precompute output sink, and backfill processor still key on `config.aggregation_id()`; those are separate per-subsystem retirements tracked in the same issue. The legacy `handle_query` / `execute_context` path also still routes through agg_id and the "No precomputed outputs found for metric: X, aggregation_id: Y" error message — its retirement is the next step. Closes the sid-resolution sub-step of #272. Related: issue #271 (MVP demo axis C) — sister fix path 2. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(query): port resolve_sketch_metric_alias to modern execute() (schema-retirement #5 step 2) Continuing #272 retirement work. The agent-side `_quantile` / `_hll` INGEST-time metric renames (DDSketch+KLL append `_quantile`, HLL appends `_hll`) used to be handled only at the entry of the legacy `handle_query_promql` path. With #273 making modern `execute()` and `execute_range_promql_modern` the producing paths for the common sketch-backed-query case, those paths also need the rename rewrite — otherwise a user query `quantile_over_time(0.99, http_latency[5m])` that lands in modern (via the legacy→modern fallback in `process_via_simple_engine`) misses the suffixed series the ASAP tier actually holds. Port: call `self.resolve_sketch_metric_alias(query).unwrap_or_else(...)` at the top of both modern execute() trait impl and `execute_range_promql_modern`. The helper is already a pure function (parse + classify shape + replace metric token); no refactor needed, just an additional caller. Tested with the smoke test (`/mydata/mvp-smoke-test/run_smoke.sh`) — the bare-metric case still works because the smoke fake-exporter emits `http_requests_total_latency_ms` literally (no suffix), and the streaming-config metric name matches, so `resolve_sketch_metric_alias` correctly no-ops via `bare_present`. Once the agent's DDSketch processor's `metric_suffix: "_quantile"` takes effect (when ASAPCollector#381 is resolved and OpAMP-pushed runtime config applies), modern's PromQL queries will need this rename to bind back to the suffixed series — this PR pre-stages it. Why not reorder process_via_simple_engine to call modern first now: the `http_capability_miss_feedback_loop_closes_over_http` test relies on the capability-miss notify side-effect inside `find_compatible_aggregation_with_miss_notify` (which only legacy calls); reordering also surfaces a pre-existing time=0 underflow bug at engine.rs:792. Modern needs its own capability-miss notify before that flip is safe — staged as the next sub-PR of #272. Test plan: * `cargo test -p data_plane --lib` 756/756 pass * `cargo test -p control_plane --lib` 691/691 pass Related: #272 (schema-retirement #5 umbrella), #273 (step 1 — sid resolution fallback). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * revert+retire(query): delete dead resolve_sketch_metric_alias rewrite Revert PR #274 (and remove the legacy `handle_query_promql` call site plus the function itself and its test module). The `_quantile` / `_hll` metric-suffix rewrite is obsolete dead code post the 2026-05 processor refactor. Per `opentelemetry-collector-contrib-patch/processor/ddsketchprocessor/shim_helpers.go:189`: // Refactor-2026-05: metric name is PRESERVED from the input metric. // MetricSuffix is intentionally NOT applied — the sketch type is // carried by the OTLP pdata.Metric variant tag (DDSketch), so the // downstream backend can identify the encoding without a name suffix // and PromQL queries fired against the raw input metric name resolve // directly against the stored sketch state. (See also `deploy/mvp-singlenode/configs/backend-streaming.yaml`'s header note documenting the same refactor.) The wire-side input metric name IS the served metric name; the per-processor `metric_suffix` config field is now a no-op in production paths. The backend's `resolve_sketch_metric_alias` was the query-side counterpart to that long-since-retired emit-side suffix. With the suffix never applied, the resolver's `bare_present` check always short-circuits to `None` — making it pure overhead (one PromQL parse + one streaming-config snapshot scan per query) and worse, misleading: the doc comments + the in-tree `mod sketch_alias_resolver_tests` keep an architectural story alive that the runtime no longer matches. This PR removes: * The 3 call sites of `resolve_sketch_metric_alias`: - `handle_query_promql` (line 2038 — pre-existing) - `QueryEngine::execute()` trait impl (line ~3219 — added by #274) - `execute_range_promql_modern` (line ~3720 — added by #274) * The `resolve_sketch_metric_alias` method itself (~90 LOC + ~30 LOC doc comment) * The `replace_metric_token` helper (only caller was the resolver) * The `utf8_char_len` helper (only caller was `replace_metric_token`) * The `sketch_alias_resolver_tests` module (~160 LOC, 8 tests, all asserting behavior that's been a no-op for months) Net: -381 LOC, no additions. Test plan: * `cargo test -p data_plane --lib` → 748 pass / 0 fail / 5 ignored (756 → 748 reflects the 8 deleted alias-resolver tests) * `cargo test -p control_plane --lib` → 691 pass / 0 fail Follow-ups (out of scope for this PR): * `docs/design-controller-into-backend.md` still has two `metric_suffix: "_quantile"` references that should be rewritten in a docs-only PR — the live config (`gateway-aggregate-from-raw.yaml`) no longer carries the suffix, so the doc table is stale. Apologies for the original PR #274 — I called it a "port" without checking that the function being ported is dead code. Thanks for the catch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2 tasks
zzylol
added a commit
that referenced
this pull request
May 17, 2026
…tch_metric_alias callers (B6) (#277) Two fixes in `data_plane/src/query_engines/asap_query_engine/engine.rs`: 1. **`calculate_start_timestamp_promql` underflow.** The `OnlySpatial` branch did `end_timestamp - (scrape_interval * 1000)` with `end_timestamp: u64`. When the PromQL request has `time=0` (capability-miss feedback-loop probe shape, see `tests::capability_miss_http_e2e_tests`), `end_timestamp` is 0 and the bare subtraction underflows with `attempt to subtract with overflow`. Switch to `saturating_sub` — clamps to 0, which the downstream store query treats as a [0, 0]-width range. Empty result is the right answer for a probe looking for the capability-miss signal, not data. Currently dormant under the current call ordering (`process_via_simple_engine` calls legacy first, which short-circuits at the empty-streaming-config check before reaching line 611), but surfaces the moment the dispatch is reordered to modern-first. Fix removes the latent blocker for step 4 of #272. 2. **Delete dangling `resolve_sketch_metric_alias` callers at engine.rs:3024 and engine.rs:3525.** PR #275 retired the `resolve_sketch_metric_alias` method itself (the runtime refactor preserves metric names through the sketch processors — the suffix rewrite hadn't done anything in production for months). The PR removed the method definition + the legacy handle_query_promql call site but missed these two call sites inside `execute_range_promql_modern` and `QueryEngine::execute()` (both added by PR #274, which #275 was supposed to revert). Result: main fails to compile with two E0599s. This PR removes them inline as a fix-forward. Both call sites had the same shape: let query_owned = self .resolve_sketch_metric_alias(query) .unwrap_or_else(|| query.to_string()); let query = query_owned.as_str(); Removing the snippet leaves the original `query: &str` parameter in scope, which is what every downstream user wants — the analyzer accepts a `&str` directly. No semantic change beyond "don't try to rewrite a metric name that doesn't get rewritten by anyone." Test plan: * `cargo test -p data_plane --lib`: 749/749 pass (was 748; +1 for the new `calculate_start_timestamp_promql_handles_time_zero_without_underflow` regression test that pins the saturating_sub semantics). * Existing `capability_miss_http_e2e` tests still pass — the legacy path that previously triggered the underflow is still reached the same way (modern is still called second); the fix is dormant until the reorder lands but is now safe. Unblocks the next step of #272 — `process_via_simple_engine` modern-first reorder. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Step 2 of #272 (schema-retirement #5). The agent-side `_quantile` / `_hll` INGEST-time metric renames (DDSketch+KLL append `_quantile`, HLL appends `_hll`) used to be handled only at the entry of the legacy `handle_query_promql` path. With #273 making modern `execute()` / `execute_range_promql_modern` the producing paths for the common sketch-backed-query case, those paths also need the rename rewrite — otherwise a user query like `quantile_over_time(0.99, http_latency[5m])` that lands in modern via the legacy→modern fallback in `process_via_simple_engine` misses the suffixed series the ASAP tier actually holds.
Port: call `self.resolve_sketch_metric_alias(query).unwrap_or_else(|| query.to_string())` at the top of both the modern `execute()` trait impl and `execute_range_promql_modern`. The helper is already a pure function (parse + classify shape + replace metric token); no refactor needed, just an additional caller.
Why not reorder `process_via_simple_engine` to call modern first yet
An earlier draft of this PR reordered the dispatch to "modern first, legacy as fallback" so the legacy path could be retired entirely. That broke `http_capability_miss_feedback_loop_closes_over_http` — the capability-miss notify side-effect happens inside legacy's `find_compatible_aggregation_with_miss_notify` (engine.rs:~1772), and a pre-existing time=0 underflow bug at engine.rs:792 surfaces when legacy is reached via the modern-Err fallback path under that test's setup. Modern needs to spawn its own capability-miss notify before the reorder is safe — staged as the next sub-PR of #272. This PR keeps the call order unchanged.
Test plan
What's still left in #272
🤖 Generated with Claude Code