diff --git a/crates/ourios-core/src/otlp.rs b/crates/ourios-core/src/otlp.rs index a087c2db4..818cd3ecc 100644 --- a/crates/ourios-core/src/otlp.rs +++ b/crates/ourios-core/src/otlp.rs @@ -229,9 +229,14 @@ impl Body { /// round-trip bit-exact — the RFC 0001 §6.1 faithfulness /// guarantee (`lossy_flag = false`) rests on it. Pinned by the /// `finite_doubles_round_trip_bit_exact` property test below. -/// Non-finite doubles (`NaN`, ±∞) have no JSON-number form and -/// encode as `null` — bytes that do **not** decode; a known, -/// pre-existing gap independent of #130. +/// Non-finite doubles (`NaN`, ±∞) have no JSON-number form, so +/// `serde_json` alone would render them as `null` (lossy). The +/// canonical codec detects a non-finite double and routes it +/// through a guarded path that emits **and** decodes the proto3- +/// JSON string forms `"NaN"`/`"Infinity"`/`"-Infinity"`, so they +/// round-trip (RFC 0018 §3.4); the finite case stays on the exact +/// serde above. Pinned by the `nonfinite_doubles_round_trip_via_proto3_strings` +/// test below. /// /// **Note on "canonical".** RFC 0005 §3.3 uses "canonical" to /// mean "the single normative encoding the writer / reader @@ -241,16 +246,17 @@ impl Body { /// the encode → store → decode round-trip is asserted at the /// `AnyValue` / `Vec` level (not on bytes). pub mod canonical { - use super::{AnyValue, KeyValue}; + use super::{AnyValue, ArrayValue, KeyValue, KeyValueList, any_value}; /// Error returned by the canonical encoders / decoders. /// Encoders are infallible on every `AnyValue` / /// `Vec` the type system admits — /// `opentelemetry-proto`'s `with-serde` ships custom /// serializers for the proto3-JSON oddities (`i64` → - /// decimal string, `bytes` → base64; non-finite - /// `f64` falls back to `serde_json`'s `null`, not the proto3 - /// `"NaN"` strings) so the recursive primitives never + /// decimal string, `bytes` → base64), and the canonical codec + /// handles non-finite `f64` itself via a guarded path emitting + /// the proto3 `"NaN"`/`"Infinity"`/`"-Infinity"` string forms + /// (RFC 0018 §3.4), so the recursive primitives never /// panic or return an error. The `Encode` arm exists only /// for `Result`-symmetry with `Decode` and as a defence- /// in-depth surface if a future `opentelemetry-proto` @@ -292,7 +298,17 @@ pub mod canonical { /// `opentelemetry-proto` release adds a fallible /// serializer. pub fn encode_any_value(value: &AnyValue) -> Result, CanonicalJsonError> { - serde_json::to_vec(value).map_err(CanonicalJsonError::Encode) + // Fast path (the common case): `opentelemetry-proto`'s `with-serde` + // is exact for finite `AnyValue`s. Only when a non-finite double is + // present do we route through the robust encoder, which emits the + // proto3-JSON string forms (`"NaN"`/`"Infinity"`/`"-Infinity"`) that + // `serde_json` would otherwise lossily render as `null` (RFC 0018 + // §3.4). + if has_nonfinite(value) { + serde_json::to_vec(&av_to_json(value)).map_err(CanonicalJsonError::Encode) + } else { + serde_json::to_vec(value).map_err(CanonicalJsonError::Encode) + } } /// Inverse of [`encode_any_value`]. Used by the reader to @@ -304,7 +320,20 @@ pub mod canonical { /// corruption or a foreign producer that doesn't honour the /// §3.3 spec). pub fn decode_any_value(bytes: &[u8]) -> Result { - serde_json::from_slice(bytes).map_err(CanonicalJsonError::Decode) + match serde_json::from_slice::(bytes) { + Ok(av) => Ok(av), + // `opentelemetry-proto`'s deserializer rejects the proto3-JSON + // non-finite string forms ("invalid type: string, expected + // f64"). Re-parse as a generic JSON tree and convert ourselves, + // honouring `"NaN"`/`"Infinity"`/`"-Infinity"` (RFC 0018 §3.4). + // Genuinely corrupt bytes fail the generic parse too → original + // error. + Err(fast) => { + let json = serde_json::from_slice::(bytes) + .map_err(|_| CanonicalJsonError::Decode(fast))?; + json_to_av(&json).map_err(CanonicalJsonError::Decode) + } + } } /// Encode a `Vec` (the in-memory shape of @@ -318,7 +347,15 @@ pub mod canonical { /// at infallible serializers (see [`encode_any_value`] / /// the [`CanonicalJsonError`] doc). pub fn encode_attributes(attrs: &[KeyValue]) -> Result, CanonicalJsonError> { - serde_json::to_vec(attrs).map_err(CanonicalJsonError::Encode) + if attrs + .iter() + .any(|kv| kv.value.as_ref().is_some_and(has_nonfinite)) + { + let arr = serde_json::Value::Array(attrs.iter().map(kv_to_json).collect()); + serde_json::to_vec(&arr).map_err(CanonicalJsonError::Encode) + } else { + serde_json::to_vec(attrs).map_err(CanonicalJsonError::Encode) + } } /// Inverse of [`encode_attributes`]. The reader uses this @@ -329,7 +366,165 @@ pub mod canonical { /// /// [`CanonicalJsonError::Decode`] on malformed bytes. pub fn decode_attributes(bytes: &[u8]) -> Result, CanonicalJsonError> { - serde_json::from_slice(bytes).map_err(CanonicalJsonError::Decode) + match serde_json::from_slice::>(bytes) { + Ok(kvs) => Ok(kvs), + Err(fast) => { + let json = serde_json::from_slice::(bytes) + .map_err(|_| CanonicalJsonError::Decode(fast))?; + let arr = json.as_array().ok_or_else(|| { + CanonicalJsonError::Decode(de_err("attributes must be a JSON array")) + })?; + arr.iter() + .map(json_to_kv) + .collect::, _>>() + .map_err(CanonicalJsonError::Decode) + } + } + } + + // ---- RFC 0018 §3.4 non-finite-double support ---------------------- + // + // `serde_json` renders a non-finite `f64` as `null` (lossy: the three + // non-finite values collapse to one shape, and `opentelemetry-proto`'s + // deserializer rejects both `null` and the proto3 string form for an + // `f64` field). When — and only when — a non-finite double is present, + // we route through these hand-built converters, which use the proto3-JSON + // string forms `"NaN"`/`"Infinity"`/`"-Infinity"` and round-trip them. + // The finite case stays on `opentelemetry-proto`'s exact serde (above). + + /// A canonical-decode error with `msg` (constructed without a direct + /// `serde` dependency — `serde_json::Error::io` is the public escape + /// hatch for "the bytes were valid JSON but not a valid `AnyValue`"). + fn de_err(msg: impl Into) -> serde_json::Error { + serde_json::Error::io(std::io::Error::other(msg.into())) + } + + /// Whether `av` contains a non-finite double anywhere in its tree. + fn has_nonfinite(av: &AnyValue) -> bool { + match &av.value { + Some(any_value::Value::DoubleValue(d)) => !d.is_finite(), + Some(any_value::Value::ArrayValue(a)) => a.values.iter().any(has_nonfinite), + Some(any_value::Value::KvlistValue(kv)) => kv + .values + .iter() + .any(|k| k.value.as_ref().is_some_and(has_nonfinite)), + _ => false, + } + } + + /// The proto3-JSON string form for a non-finite double. + fn nonfinite_token(d: f64) -> &'static str { + if d.is_nan() { + "NaN" + } else if d > 0.0 { + "Infinity" + } else { + "-Infinity" + } + } + + /// `AnyValue` → JSON, emitting the proto3 string form for non-finite + /// doubles. Subtrees with no non-finite double delegate to + /// `opentelemetry-proto`'s exact serde (correct shapes / escaping / + /// int-decimal-string / base64), so only the non-finite spine is + /// hand-built. + fn av_to_json(av: &AnyValue) -> serde_json::Value { + use serde_json::json; + match &av.value { + Some(any_value::Value::DoubleValue(d)) if !d.is_finite() => { + json!({ "doubleValue": nonfinite_token(*d) }) + } + Some(any_value::Value::ArrayValue(a)) if a.values.iter().any(has_nonfinite) => { + json!({ "arrayValue": { "values": a.values.iter().map(av_to_json).collect::>() } }) + } + Some(any_value::Value::KvlistValue(kv)) + if kv + .values + .iter() + .any(|k| k.value.as_ref().is_some_and(has_nonfinite)) => + { + json!({ "kvlistValue": { "values": kv.values.iter().map(kv_to_json).collect::>() } }) + } + // Finite-only subtree (incl. finite double, string, int, bool, + // bytes, empty) — `opentelemetry-proto` serde is exact + infallible. + _ => serde_json::to_value(av).expect("opentelemetry-proto serde is infallible here"), + } + } + + /// `KeyValue` → `{"key": …, "value": …}` (value omitted when absent), + /// matching `opentelemetry-proto`'s shape. + fn kv_to_json(kv: &KeyValue) -> serde_json::Value { + use serde_json::json; + match &kv.value { + Some(v) => json!({ "key": kv.key, "value": av_to_json(v) }), + None => json!({ "key": kv.key }), + } + } + + /// Inverse of [`av_to_json`]: a generic JSON tree → `AnyValue`, decoding + /// the proto3 non-finite string forms; finite leaves delegate to + /// `opentelemetry-proto`'s serde. + fn json_to_av(j: &serde_json::Value) -> Result { + if let Some(obj) = j.as_object() { + if let Some(serde_json::Value::String(s)) = obj.get("doubleValue") { + let d = match s.as_str() { + "NaN" => f64::NAN, + "Infinity" => f64::INFINITY, + "-Infinity" => f64::NEG_INFINITY, + other => { + return Err(de_err(format!("invalid doubleValue string {other:?}"))); + } + }; + return Ok(AnyValue { + value: Some(any_value::Value::DoubleValue(d)), + }); + } + if let Some(arr) = obj.get("arrayValue").and_then(|a| a.get("values")) { + let values = arr + .as_array() + .ok_or_else(|| de_err("arrayValue.values must be an array"))? + .iter() + .map(json_to_av) + .collect::, _>>()?; + return Ok(AnyValue { + value: Some(any_value::Value::ArrayValue(ArrayValue { values })), + }); + } + if let Some(vals) = obj.get("kvlistValue").and_then(|k| k.get("values")) { + let values = vals + .as_array() + .ok_or_else(|| de_err("kvlistValue.values must be an array"))? + .iter() + .map(json_to_kv) + .collect::, _>>()?; + return Ok(AnyValue { + value: Some(any_value::Value::KvlistValue(KeyValueList { values })), + }); + } + } + // No non-finite-double / structured spine here → exact serde. + serde_json::from_value(j.clone()) + } + + /// Inverse of [`kv_to_json`]. + fn json_to_kv(j: &serde_json::Value) -> Result { + let obj = j + .as_object() + .ok_or_else(|| de_err("KeyValue must be a JSON object"))?; + let key = obj + .get("key") + .and_then(serde_json::Value::as_str) + .ok_or_else(|| de_err("KeyValue.key must be a string"))? + .to_string(); + let value = match obj.get("value") { + Some(v) => Some(json_to_av(v)?), + None => None, + }; + Ok(KeyValue { + key, + value, + ..Default::default() + }) } #[cfg(test)] @@ -579,23 +774,34 @@ pub mod canonical { } } - /// Non-finite doubles have no JSON-number form; the - /// `with-serde` encoder emits `{"doubleValue":null}` for - /// them (captured empirically — **not** the proto3-JSON - /// `"NaN"` / `"Infinity"` strings). Pinned so an upstream - /// change can't drift stored bytes silently. Decoding - /// these bytes fails — a known, pre-existing gap - /// independent of #130. + /// RFC 0018 §3.4 overturns the prior clamp-to-`null` gap: a + /// non-finite double now encodes to the proto3-JSON string form + /// (`"NaN"`/`"Infinity"`/`"-Infinity"`) and round-trips. (Was: the + /// `with-serde` encoder emitted `{"doubleValue":null}`, which did not + /// decode — silently dropping the value.) #[test] - fn nonfinite_doubles_encode_to_the_null_shape() { - for x in [f64::NAN, f64::INFINITY, f64::NEG_INFINITY] { + // `x == y` on the ±Infinity arms is intentional exact equality — the + // round-trip contract is value-identity, and Inf == Inf is well-defined. + #[allow(clippy::float_cmp)] + fn nonfinite_doubles_round_trip_via_proto3_strings() { + for (x, token) in [ + (f64::NAN, "NaN"), + (f64::INFINITY, "Infinity"), + (f64::NEG_INFINITY, "-Infinity"), + ] { let bytes = encode_any_value(&double_av(x)).expect("encode"); - assert_eq!(bytes, br#"{"doubleValue":null}"#, "drift for {x:?}"); - // The other half of the pinned gap: those bytes do not - // decode, so non-finite doubles never round-trip. + assert_eq!( + bytes, + format!(r#"{{"doubleValue":"{token}"}}"#).into_bytes(), + "non-finite double encodes to the proto3 string form", + ); + let back = decode_any_value(&bytes).expect("decode"); + let Some(any_value::Value::DoubleValue(y)) = back.value else { + panic!("decoded variant drifted for {x:?}"); + }; assert!( - decode_any_value(&bytes).is_err(), - "the null shape unexpectedly decoded for {x:?}", + (x.is_nan() && y.is_nan()) || x == y, + "non-finite double round-trips: {x:?} -> {y:?}", ); } } diff --git a/crates/ourios-core/tests/rfc0018_otlp_compliance.rs b/crates/ourios-core/tests/rfc0018_otlp_compliance.rs index 486d4f0f9..791a88e2a 100644 --- a/crates/ourios-core/tests/rfc0018_otlp_compliance.rs +++ b/crates/ourios-core/tests/rfc0018_otlp_compliance.rs @@ -1,22 +1,121 @@ //! RFC 0018 — OTLP log-spec compliance acceptance scenario (§5), the //! canonical-encoding arm (`.5`). //! -//! **Status: `red`.** Failing stub driving the `green` implementation: it -//! encodes RFC 0018 §5 scenario .5 and currently `todo!()`s. It is `#[ignore]`d -//! so the default `cargo test` (and CI) stays green while non-finite-double -//! handling is fixed — `green` replaces the body with a round-trip assertion -//! (and overturns the existing "encodes to null" test) and removes the -//! `#[ignore]`. +//! Non-finite doubles (`NaN`/`Infinity`/`-Infinity`) round-trip through the +//! Ourios-canonical JSON via the proto3-JSON string forms — at the top level, +//! nested inside a structured `Body` (`arrayValue`/`kvlistValue`), and as an +//! attribute value. The finite case stays on the exact (`#130`-bit-exact) +//! fast path; this exercises the non-finite robust path. //! //! See `docs/rfcs/0018-otlp-log-spec-compliance.md` §5/§6. -/// Scenario RFC0018.5 — non-finite doubles round-trip through canonical JSON: -/// an `AnyValue` containing `NaN`, `Infinity`, and `-Infinity`, canonical-encoded -/// (proto3-JSON string forms "NaN"/"Infinity"/"-Infinity") and decoded, equals -/// the original — no `null` collapse. +use ourios_core::otlp::canonical::{ + decode_any_value, decode_attributes, encode_any_value, encode_attributes, +}; +use ourios_core::otlp::{AnyValue, ArrayValue, KeyValue, KeyValueList, any_value}; + +fn double(x: f64) -> AnyValue { + AnyValue { + value: Some(any_value::Value::DoubleValue(x)), + } +} + +fn string(s: &str) -> AnyValue { + AnyValue { + value: Some(any_value::Value::StringValue(s.to_owned())), + } +} + +fn kv(key: &str, value: AnyValue) -> KeyValue { + KeyValue { + key: key.to_owned(), + value: Some(value), + ..Default::default() + } +} + +/// True when `a` and `b` are equal treating any two NaNs as equal (proto3-JSON +/// canonicalises NaN, so a NaN payload is not preserved — value equality is +/// the contract, not bit-identity). +// `x == y` is intentional exact equality (round-trip is value-identity; the +// NaN case is handled explicitly, and Inf == Inf is well-defined). +#[allow(clippy::float_cmp)] +fn nan_eq(a: &AnyValue, b: &AnyValue) -> bool { + use any_value::Value::{ArrayValue as Arr, DoubleValue as D, KvlistValue as Kv}; + match (&a.value, &b.value) { + (Some(D(x)), Some(D(y))) => (x.is_nan() && y.is_nan()) || x == y, + (Some(Arr(x)), Some(Arr(y))) => { + x.values.len() == y.values.len() + && x.values.iter().zip(&y.values).all(|(p, q)| nan_eq(p, q)) + } + (Some(Kv(x)), Some(Kv(y))) => { + x.values.len() == y.values.len() + && x.values.iter().zip(&y.values).all(|(p, q)| { + p.key == q.key + && match (&p.value, &q.value) { + (Some(pv), Some(qv)) => nan_eq(pv, qv), + (None, None) => true, + _ => false, + } + }) + } + _ => a == b, + } +} + +/// Scenario RFC0018.5 — non-finite doubles round-trip through canonical JSON, +/// at the top level, nested in a structured body, and as an attribute value. /// See `docs/rfcs/0018-otlp-log-spec-compliance.md` §5. #[test] -#[ignore = "RFC0018.5 — red until non-finite doubles use the proto3-JSON string forms (green)"] fn rfc0018_5_non_finite_doubles_round_trip() { - todo!("RFC0018.5: NaN/Infinity/-Infinity round-trip via proto3-JSON string forms") + // Top level: each of the three non-finite values. + for x in [f64::NAN, f64::INFINITY, f64::NEG_INFINITY] { + let av = double(x); + let back = decode_any_value(&encode_any_value(&av).expect("encode")).expect("decode"); + assert!( + nan_eq(&av, &back), + "top-level non-finite double round-trips" + ); + } + + // Structured body: a kvlist + nested array mixing the three non-finite + // values with finite doubles and a string — the robust path must walk the + // whole tree and leave the finite/string leaves intact. + let body = AnyValue { + value: Some(any_value::Value::KvlistValue(KeyValueList { + values: vec![ + kv("nan", double(f64::NAN)), + kv("inf", double(f64::INFINITY)), + kv("neg_inf", double(f64::NEG_INFINITY)), + kv("finite", double(3.5)), + kv("label", string("ok")), + kv( + "mixed", + AnyValue { + value: Some(any_value::Value::ArrayValue(ArrayValue { + values: vec![double(f64::INFINITY), double(-2.0), string("x")], + })), + }, + ), + ], + })), + }; + let back = decode_any_value(&encode_any_value(&body).expect("encode")).expect("decode"); + assert!( + nan_eq(&body, &back), + "structured body with non-finite doubles round-trips" + ); + + // Attribute value: a non-finite double in a `Vec` (the + // attributes / resource_attributes / scope_attributes encoding). + let attrs = vec![ + kv("rate", double(f64::NEG_INFINITY)), + kv("name", string("svc")), + ]; + let decoded = decode_attributes(&encode_attributes(&attrs).expect("encode")).expect("decode"); + assert_eq!(decoded.len(), attrs.len()); + for (a, b) in attrs.iter().zip(&decoded) { + assert_eq!(a.key, b.key); + assert!(nan_eq(a.value.as_ref().unwrap(), b.value.as_ref().unwrap())); + } } diff --git a/docs/rfcs/0018-otlp-log-spec-compliance.md b/docs/rfcs/0018-otlp-log-spec-compliance.md index db09421af..d8f50e081 100644 --- a/docs/rfcs/0018-otlp-log-spec-compliance.md +++ b/docs/rfcs/0018-otlp-log-spec-compliance.md @@ -1,7 +1,7 @@ --- rfc: 0018 title: OTLP log-spec compliance amendments -status: red +status: green author: Jens Holdgaard Pedersen drafting-assistance: Claude created: 2026-06-20