test(otel): decode the wire encoder's output with an independent decoder - #231
Merged
Conversation
The existing wire tests already decode the emitted bytes, but with a hand-written extractor that shares its author's reading of the encoding spec with the encoder it checks. Three blind spots follow: `only`/`all` select a field by number and drop the wire type (a varint payload comes back as the same eight little-endian bytes a fixed64 would); they only look for the fields they expect, so a duplicated or undeclared one goes unseen; and `read_varint` accepts non-canonical encodings `put_varint` never emits. A wrong length prefix is not an error at the far end — the collector drops the record silently — so the oracle has to be independent. prost as a dev-dependency, with the OTLP messages transcribed as `prost::Message` derives in the test module: no .proto, no build script, no protoc, four crates in the dev graph and none in the shipped binary. Five tests: varints byte-for-byte against prost's own encoder over both sides of every 7-bit boundary, a full semantic decode of a request, byte-identity on decode-then-re-encode (the unknown/duplicate/ non-canonical check), length prefixes around 127/128 and 16383/16384, and int attributes at the signed extremes. Mutation-checked, seven mutations. Wrong length prefix, dropped continuation byte and a swapped field number are caught by both old and new tests. Four are caught only by the new ones: a fixed64 written as a varint under the same field number, a duplicated severity_text, an undeclared field 12 on LogRecord, and a padded two-byte varint for zero. DESIGN.md's dependency-decision bullet claimed zero crates in the lock file and nothing new for cargo deny; both are now false, so the claims are narrowed to the shipped binary and the dev-graph cost is stated. Targeted boundary cases rather than proptest: the encoder's input space is narrow and the varint widths are enumerable, so the table is both exhaustive where it matters and legible. It also earns its keep against the byte-identity test — the padded-zero mutation slips past that one on a fixture with no zero-valued varint, and only the boundary and extremes tables catch it.
plusky
added
the
ai-assisted
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.
Closes #201.
otel.rs'swiremodule hand-rolls protobuf. A wrong length prefix does notthrow — the collector silently drops the record and the audit-adjacent
export goes dark. Everything else in the audit path fails loudly or not at all.
The gap is not where the issue says
The existing hand-written extractor already decodes the emitted bytes, and it
catches all three failure modes the issue names. What it misses:
fields()normalizes a decoded varint toto_le_bytes()—byte-identical to a
fixed64payload. Afixed64emitted as a varint passes.A real collector rejects it.
only()takes the first match by field number andnothing asserts the buffer holds no more, so duplicated fields, undeclared
fields and non-canonical varints are all invisible.
Seven-mutation matrix, reproduced independently by review:
len+1Shape
prostas a dev-dependency only — noprotoc, no.proto, nobuild.rs.The messages are hand-transcribed derives, which is what makes them an
independent oracle rather than generated code sharing the encoder's assumptions.
Review diffed every one against
opentelemetry-proto-0.32.0: no mirroredtranscription error.
proptest rejected: the input space is narrow and varint widths enumerable, so
a targeted table is exhaustive where it matters and deterministic.
That choice earned itself — the byte-identity test passed under the
padded-zero mutation, because its fixtures contain no zero-valued varint. Only
the boundary tables caught it. They are not redundant with it.
Cost
Four crates in the dev graph (
prost,prost-derive,itertools,either),all already-allowed licences, no new duplicates, none reaching the shipped
binary (
cargo tree -e normal: zero hits). MSRV clear.No encoder defect found —
wireis correct and untouched.DESIGN.md's rationale for hand-rolling said
cargo deny"has nothing new tojudge, and
Cargo.lockis unchanged by this feature". Both were falsified bythis change, so the clauses are rewritten rather than corrected by juxtaposition:
the runtime graph is unchanged; the lock and deny's scope are not.
Review: MERGE-SAFE.