Skip to content

Re-enable the controller-plans e2e against the ASAPPlanner path #723

Description

@zzylol

Follow-up to #703 item 18. The 12 tests in data_plane/tests/e2e_controller_plans_and_backend_serves.rs are #[ignore]d by the collector-emit removal PR and need their payload construction ported before they can be re-enabled.

Why they were ignored rather than fixed in-place

The file built its config through the legacy chain:

QueryWorkload → bind_workload_typed → split_typed_three_stage
  → BackendStageConfig → emit_backend_streaming_config_json
  → StreamingConfig::from_yaml_data → aggregation_configs → artifact

Every step of that chain is removed. The scaffolding has already been ported in the same PR — the file compiles — but making the tests pass is a different problem: they assert against, and build their OTLP payloads from, summary families the legacy path was told to use via sketch_type_override. ASAPPlanner has no such override; it picks from the accuracy target. Per-test payload construction has to follow whatever it picks, which is real work in the data path rather than a mechanical rename.

This is the work #703 item 2 already scoped separately ("those tests have to switch to an installed QueryPlan first").

What is already done

  • plan_materializations(query, accuracy) runs BackendLocalPlanningSnapshot::planning_requestPhysicalCompiler::compile and returns the materializations, exactly as the production compile-and-publish path does.
  • epsilon_delta(eps, delta) builds the QueryRequirements accuracy shape.
  • physical_fixture::artifact_from_materializations takes planner output directly, so the legacy StreamingConfig document is no longer an intermediate step. artifact(&StreamingConfig) delegates to it, so the other fixture users are unaffected.
  • post_materializations / post_full_config install through /api/v1/physical-plan + activate.
  • All 12 call sites and the 17 assertion references are converted. cargo check -p data_plane --test e2e_controller_plans_and_backend_serves is clean.

What ASAPPlanner actually picks

Measured, not assumed — the three tests that used to pin a family:

test old sketch_type_override planner's choice
controller_plan_to_query_full_roundtrip_kll KLL DDSketch
controller_plan_to_query_full_roundtrip_count_sketch CountSketch CountMinSketchWithHeap
controller_plan_to_query_full_roundtrip_count_min_sketch CountMinSketch CountMinSketchWithHeap

Remaining failures, by class

  1. summary frame for <metric> declares materialization <id> but active schema resolves 0 (4 tests). The OTLP payload is still built for the old family/parameters, so physical_fixture::stamp's schema lookup does not resolve. Note stamp matches on metric name with a _ddsketch suffix special case — the delta-subwindow test depends on that.
  2. frame has no matching transmission rule (2 tests). Same root cause.
  3. QueryPlan(Invalid("native residual substitution requires an exact selected value")) (2 tests). The PromQL chosen for the cardinality and grouping cases — count(unique_users_per_min) and quantile_over_time(0.99, sum by (zone) (http_latency_ms)[60s:]) — does not plan as a pure summary query. Needs query text that does.
  4. no entry found for key (1 test). The KLL test reads parameters["k"]; the planner picked DDSketch, which has no k.
  5. physical plan install 503: physical-plan hot-reload handles are not attached (1 test). controller_streaming_config_round_trips_through_backend_http uses start_backend_http_server(), which does not wire the physical-plan handles. It needs the FullStack harness.

Classes 3, 4 and 5 are mechanical. Classes 1 and 2 are the substance: each test's payload family and sizing has to be derived from the installed materialization instead of from a pinned constant.

Suggested approach

Work one test at a time, starting from controller_plan_to_query_full_roundtrip_ddsketch (the planner's choice there already matches what the test builds, so only the identity plumbing is in question). Build each payload from materializations[0].aggregation_type and materializations[0].parameters rather than from constants, and let the family drive which build_*_state helper is used.

🤖 Generated with Claude Code

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions