fix(core): lenient shim strips null-valued AnyValue fields (upstream #3603 case)#604
Conversation
…#3603 case)
The OTLP/JSON lenient shim already rewrote the two whole-value unset encodings
(`{}` and `null`) to an absent field so the broken upstream `with-serde`
deserialiser doesn't reject the batch. It did NOT handle a *field* set to null
inside an AnyValue object — `{"intValue":null}` / `{"doubleValue":null}` —
which proto3-JSON maps to that field's default (for a oneof, unset). Real
exporters emit this: the Vercel AI SDK writes `doubleValue: null` for
non-finite token counts (upstream opentelemetry-rust#3603), and one such value
anywhere in a batch failed the whole payload parse — exactly the GenAI traffic
RFC 0037 targets.
Widen the per-slot unset check from `inner.is_empty()` to
`inner.values().all(is_null)`: it subsumes `{}` (vacuously true), catches
all-null-field objects, and crucially LEAVES a meaningful `{"stringValue":""}`
intact (an empty string is a set value the spec says MUST be stored — not
unset). Array-element unset AnyValues stay a parse error (the pre-existing
positional limitation only the deserializer-level upstream fix can reach).
Same bounded, documented fidelity concession as before (unset AnyValue → absent
`None`, not present-but-unset `Some(None)`); RFC 0018's Some(empty)-vs-None
round-trip tests are unchanged. Covered at the core shim level and the
production `decode_json` path; the ingester test also carries the null-field
arm of the shim-retirement flip signal, so the shim is not retired until BOTH
upstream #3595 (`{}`) and #3603 (null field) ship.
Refs #549, #546
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y
Signed-off-by: Jens Holdgaard Pedersen <Jens@holdgaard.org>
|
Warning Review limit reached
Next review available in: 10 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe OTLP JSON leniency shim now recognizes AnyValue objects containing only null fields as unset. Core tests cover body and nested attribute values, while an RFC0003.6 integration test verifies lenient decoding and absent results. ChangesOTLP AnyValue leniency
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/ourios-core/src/otlp.rs`:
- Around line 1067-1069: Update the AnyValue-object detection around the
serde_json::Value::Object handling so it only treats an object as removable when
every key is a recognized AnyValue variant and every associated value is null;
unknown keys such as bogusValue must prevent stripping. Add a regression case
covering an object containing bogusValue: null.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 98542361-f3a5-476a-8e70-b94ebcc3a38b
📒 Files selected for processing (2)
crates/ourios-core/src/otlp.rscrates/ourios-ingester/tests/it/rfc0003_6_json_protobuf_equivalence.rs
There was a problem hiding this comment.
Pull request overview
Extends ourios_core::otlp::lenient_json to tolerate proto3-JSON AnyValue objects where the oneof field is explicitly set to null (e.g. {"intValue":null}, {"doubleValue":null}), preventing valid OTLP/JSON payloads from being rejected by the current upstream opentelemetry-proto with-serde deserializer.
Changes:
- Broaden unset-
AnyValuedetection in the lenient JSON shim to treat all-null-fieldAnyValueobjects as “unset” and strip the correspondingbody/valuefield before retrying parse. - Add a core unit test ensuring null-field stripping works while preserving meaningful empty-string values.
- Add an ingester integration test that
decode_jsonaccepts a payload containing null-valuedAnyValuefields, serving as part of the shim-retirement flip signal.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| crates/ourios-core/src/otlp.rs | Widen lenient OTLP/JSON normalization to strip null-valued AnyValue fields; add coverage for null-field behavior and empty-string preservation. |
| crates/ourios-ingester/tests/it/rfc0003_6_json_protobuf_equivalence.rs | Add production-path test ensuring decode_json accepts null-field AnyValue payloads via the lenient retry. |
Comments suppressed due to low confidence (1)
crates/ourios-core/src/otlp.rs:1330
- Consider adding a regression test (near the end of this module) that an
AnyValueobject with an unknown key set tonull(e.g.{ "bogusValue": null }) is still rejected. The existing unknown-key test covers only a non-null value, and thenullcase is the one most likely to regress when widening “unset” detection.
);
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Review fix (CodeRabbit + Copilot): the first cut used
`inner.values().all(is_null)`, which also stripped `{"bogusValue":null}` — an
unknown key with a null value — turning a malformed AnyValue into valid input
and weakening the existing unknown-key-rejection guarantee. Tighten the check to
require every entry be a *recognized* oneof field name (stringValue/boolValue/
intValue/doubleValue/bytesValue/arrayValue/kvlistValue) AND null-valued. `{}`
still strips (vacuously true), `{"intValue":null}` strips, `{"stringValue":""}`
is preserved, and `{"bogusValue":null}` now stays a parse error. Extended
lenient_json_does_not_loosen_unknown_key_rejection to cover the null-valued
unknown key.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y
Signed-off-by: Jens Holdgaard Pedersen <Jens@holdgaard.org>
…t doc Review fixes (Copilot): - The core shim unit test asserted lenient == true, which would break once the upstream with-serde fix lands (direct parse succeeds → false). Parse via from_slice instead of from_slice_flagged so it's a pure behaviour test — asserts the decoded result, not which path produced it — and stays green across the upstream fix. The lenient-flag flip signal stays where it belongs, in the ingester equivalence test. - Align the new ingester test's doc comment to the file's established "Scenario RFC0003.6 — <title>. See docs/rfcs/0003-otlp-receiver.md §5." format so scenarios stay uniformly greppable. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y Signed-off-by: Jens Holdgaard Pedersen <Jens@holdgaard.org>
Bridge the null-field
AnyValuegap in the lenient OTLP/JSON shimOur
ourios_core::otlp::lenient_jsonshim already rewrites the two whole-value unsetAnyValueencodings —{}andnull— to an absent field, so the broken upstreamwith-serdedeserialiser doesn't reject the whole batch. It did not handle a field set to null inside anAnyValueobject:{"intValue":null}/{"doubleValue":null}.Why it matters. proto3-JSON maps a
nullon any field to that field's default — for aoneof, unset. Real exporters emit this: the Vercel AI SDK writesdoubleValue: nullfor non-finite token counts (upstream opentelemetry-rust#3603), and one such value anywhere in a payload failed the entire parse — exactly the GenAI/agent traffic RFC 0037 targets, and the same gap #3603 fixes at the deserializer level.The change. Widen the per-slot unset check from
inner.is_empty()toinner.values().all(is_null):{}(vacuously true — no behaviour change for the existing case),{"intValue":null},{"doubleValue":null}),{"stringValue":""}— an empty string is a set value the spec says MUST be stored, not unset (values().all(is_null)is false for it).AnyValues stay a parse error — the pre-existing positional limitation only the deserializer-level upstream fix (#3603) can reach; unchanged and still documented.Invariant (OTLP fidelity — RFC 0003.6 / RFC 0018)
This is the same bounded, documented fidelity concession the shim already makes: an unset
AnyValuedecodes to absent (None) rather than present-but-unset (Some(AnyValue{value:None})). Forbodythe two collapse identically; for aKeyValue.valuethe presence bit is the documented concession. RFC 0018'sSome(empty)-vs-Noneround-trip tests are unchanged — I'm widening the shim's coverage of unset encodings, not changing what it does with them. No RFC needed (a bug-fix extension of the shim's existing charter).Verification
otlp.rs):lenient_json_strips_a_null_field_but_keeps_a_meaningful_empty_string— one doc exercises both the null-field strip (2 positions) and empty-string preservation through the lenient retry.ourios-ingester):rfc0003_6_null_valued_anyvalue_field_decodes_via_lenient_shim—decode_jsonaccepts the null-field payload (doesn't reject the batch). It doubles as the null-field arm of the shim-retirement flip signal: the shim is not retired until both upstream #3595 ({}) and #3603 (null field) ship.Refs #549, #546.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests