fix(freshness): encoder offset audit — no fix needed (writer correct on main) - #320
Merged
Merged
Conversation
Phase 3.2 audit confirms `gorillas3processor/encoder.go` writes seriesCount at the canonical [9..13] offset (line 311), the test decoder helper reads at [9..13] (encoder_test.go:210), and the canonical Rust decoder reads at header[9..13] (decoder.rs:84). All three implementations agree. Conflicting v7 / Step 2.5 reports of a `[5..9]` offset bug in the Go writer did not reproduce on origin/main; those reports were based on stale source. The Fix 2 agent's PR #307 claim (writer was already correct, only the test decoder was at the buggy offset) was correct as of today's main. Add two negative tests in asap-gorilla/tests/byte_compat.rs that pin the contract from the Rust side: 1. decoder_rejects_pre_v7_seriescount_at_offset_5_9 — synthesizes a block with seriesCount at the buggy [5..9] offset, asserts the canonical decoder rejects it with BadMagic. Documents the original failure mode in code. 2. decoder_accepts_canonical_seriescount_at_offset_9_13 — positive twin: same shape, seriesCount at [9..13], must decode cleanly. Companion to the Go-side regression guard TestChunkHeaderByteLayoutMatchesGorillaDecoder added in PR #307. The Go test pins the writer; the Rust tests pin the decoder. Either direction of future drift now fails CI. Phase 3.2 conclusion: no encoder fix is required. The freshness probe failure (all three CSV paths empty in the latest run) has upstream root causes — the controller-emitted bootstrap agent.yaml drops the gorillas3 processor entirely (only `processors: [ddsketch]`), so freshness counters never reach MinIO; the warm-tier ddsketch transform also strips the original metric name. Those are out of scope for the encoder-offset audit; they belong to a separate "controller config emission for freshness probes" task. No image rebuild needed: encoder.go is unchanged, so asap/sketchcol:dev byte content is identical. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
5 tasks
zzylol
added a commit
that referenced
this pull request
May 7, 2026
…ugh + raw endpoint (#328) * phase 3.2.5 (a): emit gorillas3 processor when archive-tier metrics planned Phase 3.2 (PR #320) diagnosed three freshness probe routing bugs. This commit fixes (a): the typed-stage-split path's emit_edge_yaml never included a `gorillas3` processor, so freshness probes (and any other archive-bound metric) never reached MinIO and the warm-tier engine's `last_over_time(...)` queries returned empty. Changes: * New `EdgeStageConfig::archive_tier_metrics: Vec<ArchiveTierMetric>` field. Populated by `ThreeStageEmitter` whenever a `RawAtEdgePrometheusArchive` node is encountered (per the spec: any Mode-3 metric also lands in the Gorilla-S3 archive so the warm-tier engine can serve `last_over_time(...)`). Callers that don't go through stage-split (the freshness probe path is the canonical example) populate the field directly. * New `ArchiveTierMetric` struct: `metric: String`, `window_secs: Option<u64>`. The L5 emitter uses the smallest declared window to size the `gorillas3.window_interval` knob. * `emit_edge_yaml` now emits a `gorillas3` processor block when `archive_tier_metrics` is non-empty. Config matches `deploy/configs/sketchcol-agent-b6-asap-single-sketch.yaml`'s canonical shape: `block_format: prometheus_tsdb` (so the Thanos store-gateway sidecar can read the blocks), `tsdb_bucket: asap-gorilla-tsdb`, `drop_original: false` (so the metric also flows downstream to the warm-tier sketch / OTLP exporter), all knobs env-overridable (`ASAP_MINIO_ENDPOINT`, `ASAP_GORILLA_BUCKET`, `ASAP_GORILLA_TSDB_BUCKET`, `ASAP_TENANT`). * The warm-tier pipeline lists `gorillas3` BEFORE the family-specific sketch processor — mirrors `sketchcol-agent-b6-asap-single-sketch.yaml`'s `[gorillas3, ddsketch, batch]` ordering so the cold-tier write happens on the raw sample BEFORE the sketch processor mutates / suffix-renames the metric stream. Mechanical updates: `stage_config_otap.rs` / `stage_config_telegraf.rs` / `config/mod.rs` add the new field to their literal `EdgeStageConfig` constructions (`Vec::new()`); these runtimes don't emit `gorillas3` themselves (Telegraf has its own `outputs.gorilla_s3` plugin path; the OTAP DAG runtime would land in Phase ε.1.5+1 if needed) so they leave the field empty. Tests: * `phase_3_2_5_bug_a_archive_tier_metrics_emit_gorillas3_processor` — locks in the gorillas3 block + every config knob the freshness probes need. * `phase_3_2_5_bug_a_gorillas3_runs_before_sketch_in_pipeline` — pins the processor ordering against `[ddsketch, gorillas3]` regression (which would lose freshness on the second-tier write). * Updated `phase_eps1_no_mode3_edge_yaml_unchanged_from_phase_b` to also assert `gorillas3` is NOT emitted when no archive tier is declared (preserves Phase β shape for plans without archive routing). Pre-existing 10 controller test failures unchanged. 537 tests pass (was 533 before; +4 new + 2 new postfix-runtime tests dragged in by the rebase). `cargo build --release -p controller` clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * phase 3.2.5 (b): warm-tier passthrough routing for freshness probes Phase 3.2 (PR #320) diagnosed three freshness probe routing bugs. This commit fixes (b): the warm-tier DDSketch processor renames `http_freshness_probe_warm` to `http_freshness_probe_warm_quantile` (the family-specific `_quantile` suffix), so the replay client's `last_over_time(http_freshness_probe_warm[10s])` query never matches anything in the backend storage and ⑥ stays at FAIL. Picked the (b.1) routing path from the spec ("route around the sketch processor"), NOT (b.2) ("rename the probe to *_quantile"). (b.2) is the ugly-hack option — the freshness probe is a counter, suffixing it `_quantile` is misleading; (b.1) preserves the metric's design intent (the wire value `unix_ts_ms_of_emission` IS the freshness signal, exact-by-construction, not a sketch readout). Changes: * New `EdgeStageConfig::warm_passthrough_metrics: Vec<String>` field. The L5 emitter's caller populates this list with metrics that must bypass the family-specific sketch processor at the warm tier — the freshness probes are the canonical example. Other callers (every use site of the typed-stage-split path so far) leave it empty so the existing wire shape stays stable. * `emit_edge_yaml` now emits a `routing` processor with OTTL `route()` statements when `warm_passthrough_metrics` is non-empty. The processor table dispatches by `metric.name` (no need to teach the fake-exporter to set an extra attribute on top of the name) and folds the Phase ε.1 Mode-3 dispatch (`asap.mode == prometheus_archive`) into the same table when both axes are active. Three pipelines fan out: - `metrics/warm_tier` — gorillas3 + sketch processors → otlp/backend - `metrics/warm_passthrough` — gorillas3 → otlp/backend (NO sketch) - `metrics/prometheus_archive` (only when prometheus_archive_metrics is also non-empty) When `warm_passthrough_metrics` is empty the legacy `from_attribute: asap.mode` form is preserved verbatim so the existing Phase ε.1 wire shape is byte-identical (the `phase_eps1_mode3_edge_yaml_has_otlphttp_prometheus_exporter` test still passes against the legacy form). * Critical: the warm-passthrough pipeline still routes through `gorillas3` (when archive_tier_metrics is also configured) so the freshness probe lands in MinIO and the backend's GorillaQueryEngine can answer `last_over_time(...)` against it. Bypassing the sketch processor != bypassing the archive write. Mechanical updates: `stage_config_otap.rs` / `stage_config_telegraf.rs` / `config/mod.rs` add the new field to their literal `EdgeStageConfig` constructions (`Vec::new()`); these runtimes don't implement the warm-passthrough split (the OTAP DAG runtime would need its own routing-node analogue, ditto for Telegraf's processor pipeline) so they leave the field empty for now. Phase ε.1.5+1 follow-up if/when those runtimes need the same freshness-probe support. Tests: * `phase_3_2_5_bug_b_warm_passthrough_routes_around_sketch` — locks in the OTTL `route() where metric.name == ...` form, the `metrics/warm_passthrough` pipeline existence, AND the critical invariants: passthrough pipeline does NOT include `ddsketchprocessor` (the bug) but DOES include `gorillas3` (the archive write). * `phase_3_2_5_bug_b_warm_passthrough_composes_with_prometheus_archive` — pins the dual-axis dispatch (warm_passthrough + prometheus_archive together) so a single routing processor handles both. Pre-existing 10 controller test failures unchanged. 553 tests pass (was 551 after Bug (a); +2 new Bug (b) tests). `cargo build --release -p controller` clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * phase 3.2.5 (c): driver raw-endpoint defaults to Prometheus B0 port Phase 3.2 (PR #320) diagnosed three freshness probe routing bugs. This commit fixes (c): `run_mvp_demo.sh`'s `freshness_phase` was passing `PIPELINE_QUERY_PORT` to `run_freshness_phase.sh --raw-endpoint`, which resolves to the *backend* query port (`HOST_BACKEND_QUERY_PORT = 19091`) when running in asap mode. Per `mvp-freshness-probes.yaml` the raw probe target is `prometheus_b0` — the path's storage IS Prometheus B0, regardless of which pipeline is currently running: - http_freshness_probe_raw → routes to Prometheus B0 - http_freshness_probe_warm → routes to sketch warm tier - http_freshness_probe_archive → routes to GorillaQueryEngine The Step 2.4 endpoint table is unambiguous: baseline → 19090 (Prometheus B0) asap → 19091 (backend) For freshness/raw.csv, the polled endpoint is the BASELINE Prometheus B0 (port 19090). In baseline mode B0 is up under `--profile b0` and the probe lands there directly; in asap mode B0 is not running and the raw poll returns empty — that's the *correct* behaviour for the baseline-vs-asap comparison (asap doesn't carry a separate raw storage tier; the comparison's whole point is to show that). Change: --raw-endpoint "${ASAP_FRESHNESS_RAW_ENDPOINT:-http://localhost:${PIPELINE_QUERY_PORT}}" + --raw-endpoint "${ASAP_FRESHNESS_RAW_ENDPOINT:-http://localhost:${HOST_PROM_B0_PORT}}" Warm / archive endpoints stay on `PIPELINE_QUERY_PORT` — both paths route through the backend's storage-routing table to whichever tier the backend has wired (warm sketch in asap, b0 Prometheus in baseline). Verification: * `shellcheck deploy/scripts/run_mvp_demo.sh` — clean. * `shellcheck deploy/scripts/run_freshness_phase.sh` — clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- 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
origin/main. Thegorillas3processor/encoder.gowriter at line 311 writesseriesCountat the canonical[9..13]offset; the test decoder helper atencoder_test.go:210reads at[9..13]; the canonical Rust decoder atasap-gorilla/src/decoder.rs:84reads atheader[9..13]. All three implementations agree.[5..9]writer regression did not reproduce — those reports were based on stale source.What's in the patch
asap-gorilla/tests/byte_compat.rs:decoder_rejects_pre_v7_seriescount_at_offset_5_9— synthesizes a block withseriesCountat the buggy[5..9]offset (the layout the v7 / Step 2.5 reports claimed was on main). Asserts the canonical decoder rejects it withDecodeError::BadMagic. This codifies the original failure mode in test form.decoder_accepts_canonical_seriescount_at_offset_9_13— positive twin: same shape,seriesCountat the canonical offset, must decode cleanly withtotal_series == 1.No production code changed.
encoder.go,encoder_test.go, anddecoder.rsare all untouched.Why the freshness probe still fails (upstream of the encoder)
The latest demo run shows all three freshness CSVs empty (raw, warm, archive — 600/600 attempts each returned "No result for query"). The encoder offset is not the cause:
controller-emitted-configs/agent.bootstrap.yaml) dropsgorillas3and runs onlyprocessors: [ddsketch]. The freshness-probe counters never reach the gorillas3 processor, so they never land in MinIO._quantile-suffixed series, dropping the originalhttp_freshness_probe_*metric name.localhost:19091(backend) instead of the documented prometheus-b0 endpoint per the YAML spec.These are controller-config-emission and demo-runner issues, not encoder issues. Filed for follow-up Phase 3.x work — out of scope for the encoder offset audit.
Image rebuild
Not required.
encoder.gois unchanged, soasap/sketchcol:devbyte content is identical to the current main image.Test plan
cargo test --test byte_compat— passes (6/6 including 2 new tests)go test ./...ingorillas3processor/— passes (existingTestChunkHeaderByteLayoutMatchesGorillaDecoderregression guard from PR fix: gorillas3processor encoder timestamp header offset #307 still green)smoke_bench_encode_decode_ratesreproduces on main; not caused by this PR🤖 Generated with Claude Code