fix(data_plane): make format_series_key + parse_labels_from_series_key roundtrip cleanly - #285
Merged
Merged
Conversation
…y roundtrip cleanly `format_series_key` (in `drivers/ingest/otel.rs`) emitted unquoted `key=value` pairs, but `parse_labels_from_series_key` (in `precompute_engine/worker.rs`) required quoted `key="value"`. The mismatch meant `IngestState::extract_group_key_for` returned `""` for every OTLP wire-format input, and any other consumer that roundtripped a freshly-formatted series key through the parser got an empty label map back. Discovery: PR #284 (B7.6 ingest sid rekey) noticed the bug while implementing the bucketing rekey, side-stepped it by having the new helper read `point.labels` directly via HashMap lookup, and left a TODO-style note in the test docstring. Bucketing was unaffected because B7.6 routes by sid; emit-time `KeyByLabelValues` content elsewhere was silently broken. Decision: option (B) — bend `format_series_key`. The quoted PromQL- style `metric{k="v",...}` form is the canonical shape every other producer / consumer in the data plane already uses: * `render_series_key` in `storage_engines/sketch_db/backfill/prometheus_reader.rs` emits quoted-with-escapes (matches the Prometheus wire format) * `RawSample.labels`' rustdoc documents the quoted form * `sample_matches` in `storage_engines/sketch_db/backfill/raw_sample_reader.rs` strips `"` from values when parsing — expects quoted * Every existing parser test passes the quoted form * Nothing persists `format_series_key`'s output to disk (it flows into in-memory `WorkerMessage::GroupSamples` payloads and debug log lines only) `format_series_key` now escapes embedded `"`, `\`, `\n` per the PromQL lexer rules (matching `render_series_key`'s `escape_label_value`). The parser walks past `\<x>` escape pairs when scanning for the closing quote so values containing embedded `"` no longer terminate early. The returned `&str` is still the raw (un-decoded) slice — a new `decode_label_value(&str) -> Cow<str>` helper unescapes when needed. Returning the un-decoded slice keeps the existing `HashMap<&str, &str>` API (and its `processor.rs` caller, which is off-limits this PR for the B7.7 parallel agent) working without change; most live callers compare against literal config values that never contain escapable characters, so the borrow is fine. Regression coverage (new `series_key_roundtrip_tests` module in `otel.rs` + new unit tests in `worker.rs`): * canonical PromQL quoted shape pinned * roundtrip with commas in value * roundtrip with equals in value * roundtrip with embedded `"` (exercises the escape pair scan + `decode_label_value`) * roundtrip with `\` in value * roundtrip with `\n` in value * roundtrip with all metacharacters in one value * empty-labels bare-braces case * `decode_label_value` borrows when no escapes, unescapes when present, passes unknown escapes through verbatim All 725 `cargo test -p data_plane --lib` tests pass (2 pre-existing ignored, unchanged). Updated the stale "this returns empty" note in B7.6's `raw_otlp_buckets_by_sid_with_distinct_group_keys` test docstring to point at this fix. 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
Fix a latent series-key roundtrip bug discovered by PR #284 (B7.6
ingest sid rekey).
format_series_keyemitted unquotedk=vpairsbut
parse_labels_from_series_keyrequired quotedk=\"v\", soIngestState::extract_group_key_forreturned\"\"for every OTLPwire-format input. B7.6 side-stepped the bug by reading
point.labelsdirectly via HashMap; bucketing was unaffected, but emit-time
KeyByLabelValuescontent elsewhere was silently broken.Decision: option (B) — fix the formatter
The quoted PromQL-style
metric{k=\"v\",...}form is the canonicalshape the rest of the data plane already uses:
render_series_keyinprometheus_reader.rsemits quoted-with-escapesRawSample.labelsrustdoc documents the quoted formsample_matchesinraw_sample_reader.rsstrips\"from valuesformat_series_key's output to disk (in-memoryWorkerMessage::GroupSamplespayloads + debug log lines only)format_series_keynow escapes embedded\",\\,\\nper thePromQL lexer rules (matching
render_series_key'sescape_label_value).The parser walks past
\\<x>escape pairs when scanning for theclosing quote, so values containing embedded
\"no longer terminateearly. A new
decode_label_value(&str) -> Cow<str>helper unescapeswhen needed; the parser still returns the raw
&strslice so theexisting
HashMap<&str, &str>API (and itsprocessor.rscaller —off-limits this PR for the B7.7 parallel agent) works unchanged.
Regression coverage
New
series_key_roundtrip_testsinotel.rsplus extra unit testsin
worker.rs:\"/\\/\\n/all-metacharacters
decode_label_valueborrows when no escapes, unescapes when present,passes unknown escapes through verbatim
Stale doc comment in B7.6's
raw_otlp_buckets_by_sid_with_distinct_group_keystest (which referenced the bug) updated to point at this fix.
Test plan
cargo build -p data_planecleancargo test -p data_plane --lib— 725 passed, 0 failed, 2pre-existing ignored (same as refactor(data_plane): rekey ingest bucketing from (agg_id, group_key) to sid (B7.6) #284 baseline)
anything about group_key content)