From c90e5c8c5e62fa6b88de0ca669a75f524a9bedc2 Mon Sep 17 00:00:00 2001 From: Max Burke Date: Tue, 24 Aug 2021 17:27:14 -0700 Subject: [PATCH] core: add support for visiting floating point values (#1507) ## Motivation Tracing is a really useful framework but a lack of floating point value support for the core visitors means these get coerced unnecessarily to strings. ## Solution This change adds support for floating point (`f64`) visitors. --- tracing-core/src/field.rs | 20 ++++- tracing-serde/src/lib.rs | 12 +++ tracing-subscriber/src/field/debug.rs | 5 ++ tracing-subscriber/src/field/display.rs | 5 ++ tracing-subscriber/src/filter/env/field.rs | 88 +++++++++++++++++++++- tracing-subscriber/src/fmt/format/json.rs | 6 ++ tracing/tests/event.rs | 3 +- tracing/tests/span.rs | 22 ++++++ tracing/tests/support/field.rs | 38 +++++++++- 9 files changed, 192 insertions(+), 7 deletions(-) diff --git a/tracing-core/src/field.rs b/tracing-core/src/field.rs index 22c9365669..14cadb8f64 100644 --- a/tracing-core/src/field.rs +++ b/tracing-core/src/field.rs @@ -16,9 +16,9 @@ //! will contain any fields attached to each event. //! //! `tracing` represents values as either one of a set of Rust primitives -//! (`i64`, `u64`, `bool`, and `&str`) or using a `fmt::Display` or `fmt::Debug` -//! implementation. `Subscriber`s are provided these primitive value types as -//! `dyn Value` trait objects. +//! (`i64`, `u64`, `f64`, `bool`, and `&str`) or using a `fmt::Display` or +//! `fmt::Debug` implementation. `Subscriber`s are provided these primitive +//! value types as `dyn Value` trait objects. //! //! These trait objects can be formatted using `fmt::Debug`, but may also be //! recorded as typed data by calling the [`Value::record`] method on these @@ -184,6 +184,11 @@ pub struct Iter { /// [`Event`]: ../event/struct.Event.html /// [`ValueSet`]: struct.ValueSet.html pub trait Visit { + /// Visit a double-precision floating point value. + fn record_f64(&mut self, field: &Field, value: f64) { + self.record_debug(field, &value) + } + /// Visit a signed 64-bit integer value. fn record_i64(&mut self, field: &Field, value: i64) { self.record_debug(field, &value) @@ -335,6 +340,12 @@ macro_rules! ty_to_nonzero { } macro_rules! impl_one_value { + (f32, $op:expr, $record:ident) => { + impl_one_value!(normal, f32, $op, $record); + }; + (f64, $op:expr, $record:ident) => { + impl_one_value!(normal, f64, $op, $record); + }; (bool, $op:expr, $record:ident) => { impl_one_value!(normal, bool, $op, $record); }; @@ -387,7 +398,8 @@ impl_values! { record_u64(usize, u32, u16, u8 as u64), record_i64(i64), record_i64(isize, i32, i16, i8 as i64), - record_bool(bool) + record_bool(bool), + record_f64(f64, f32 as f64) } impl crate::sealed::Sealed for Wrapping {} diff --git a/tracing-serde/src/lib.rs b/tracing-serde/src/lib.rs index 94b486fbbc..9629480bc1 100644 --- a/tracing-serde/src/lib.rs +++ b/tracing-serde/src/lib.rs @@ -384,6 +384,12 @@ where } } + fn record_f64(&mut self, field: &Field, value: f64) { + if self.state.is_ok() { + self.state = self.serializer.serialize_entry(field.name(), &value) + } + } + fn record_str(&mut self, field: &Field, value: &str) { if self.state.is_ok() { self.state = self.serializer.serialize_entry(field.name(), &value) @@ -430,6 +436,12 @@ where } } + fn record_f64(&mut self, field: &Field, value: f64) { + if self.state.is_ok() { + self.state = self.serializer.serialize_field(field.name(), &value) + } + } + fn record_str(&mut self, field: &Field, value: &str) { if self.state.is_ok() { self.state = self.serializer.serialize_field(field.name(), &value) diff --git a/tracing-subscriber/src/field/debug.rs b/tracing-subscriber/src/field/debug.rs index 1e7512f796..10b9eb274c 100644 --- a/tracing-subscriber/src/field/debug.rs +++ b/tracing-subscriber/src/field/debug.rs @@ -38,6 +38,11 @@ impl Visit for Alt where V: Visit, { + #[inline] + fn record_f64(&mut self, field: &Field, value: f64) { + self.0.record_f64(field, value) + } + #[inline] fn record_i64(&mut self, field: &Field, value: i64) { self.0.record_i64(field, value) diff --git a/tracing-subscriber/src/field/display.rs b/tracing-subscriber/src/field/display.rs index 7e96de39cf..15f66c4b71 100644 --- a/tracing-subscriber/src/field/display.rs +++ b/tracing-subscriber/src/field/display.rs @@ -40,6 +40,11 @@ impl Visit for Messages where V: Visit, { + #[inline] + fn record_f64(&mut self, field: &Field, value: f64) { + self.0.record_f64(field, value) + } + #[inline] fn record_i64(&mut self, field: &Field, value: i64) { self.0.record_i64(field, value) diff --git a/tracing-subscriber/src/filter/env/field.rs b/tracing-subscriber/src/filter/env/field.rs index 994a9406dd..a6b849a8a8 100644 --- a/tracing-subscriber/src/filter/env/field.rs +++ b/tracing-subscriber/src/filter/env/field.rs @@ -36,14 +36,77 @@ pub(crate) struct MatchVisitor<'a> { inner: &'a SpanMatch, } -#[derive(Debug, Clone, PartialOrd, Ord, Eq, PartialEq)] +#[derive(Debug, Clone)] pub(crate) enum ValueMatch { Bool(bool), + F64(f64), U64(u64), I64(i64), + NaN, Pat(Box), } +impl Eq for ValueMatch {} + +impl PartialEq for ValueMatch { + fn eq(&self, other: &Self) -> bool { + use ValueMatch::*; + match (self, other) { + (Bool(a), Bool(b)) => a.eq(b), + (F64(a), F64(b)) => { + debug_assert!(!a.is_nan()); + debug_assert!(!b.is_nan()); + + a.eq(b) + } + (U64(a), U64(b)) => a.eq(b), + (I64(a), I64(b)) => a.eq(b), + (NaN, NaN) => true, + (Pat(a), Pat(b)) => a.eq(b), + _ => false, + } + } +} + +impl Ord for ValueMatch { + fn cmp(&self, other: &Self) -> Ordering { + use ValueMatch::*; + match (self, other) { + (Bool(this), Bool(that)) => this.cmp(that), + (Bool(_), _) => Ordering::Less, + + (F64(this), F64(that)) => this + .partial_cmp(that) + .expect("`ValueMatch::F64` may not contain `NaN` values"), + (F64(_), Bool(_)) => Ordering::Greater, + (F64(_), _) => Ordering::Less, + + (NaN, NaN) => Ordering::Equal, + (NaN, Bool(_)) | (NaN, F64(_)) => Ordering::Greater, + (NaN, _) => Ordering::Less, + + (U64(this), U64(that)) => this.cmp(that), + (U64(_), Bool(_)) | (U64(_), F64(_)) | (U64(_), NaN) => Ordering::Greater, + (U64(_), _) => Ordering::Less, + + (I64(this), I64(that)) => this.cmp(that), + (I64(_), Bool(_)) | (I64(_), F64(_)) | (I64(_), NaN) | (I64(_), U64(_)) => { + Ordering::Greater + } + (I64(_), _) => Ordering::Less, + + (Pat(this), Pat(that)) => this.cmp(that), + (Pat(_), _) => Ordering::Greater, + } + } +} + +impl PartialOrd for ValueMatch { + fn partial_cmp(&self, other: &Self) -> Option { + Some(self.cmp(other)) + } +} + #[derive(Debug, Clone)] pub(crate) struct MatchPattern { pub(crate) matcher: Pattern, @@ -127,6 +190,14 @@ impl PartialOrd for Match { // === impl ValueMatch === +fn value_match_f64(v: f64) -> ValueMatch { + if v.is_nan() { + ValueMatch::NaN + } else { + ValueMatch::F64(v) + } +} + impl FromStr for ValueMatch { type Err = matchers::Error; fn from_str(s: &str) -> Result { @@ -134,6 +205,7 @@ impl FromStr for ValueMatch { .map(ValueMatch::Bool) .or_else(|_| s.parse::().map(ValueMatch::U64)) .or_else(|_| s.parse::().map(ValueMatch::I64)) + .or_else(|_| s.parse::().map(value_match_f64)) .or_else(|_| { s.parse::() .map(|p| ValueMatch::Pat(Box::new(p))) @@ -145,6 +217,8 @@ impl fmt::Display for ValueMatch { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { ValueMatch::Bool(ref inner) => fmt::Display::fmt(inner, f), + ValueMatch::F64(ref inner) => fmt::Display::fmt(inner, f), + ValueMatch::NaN => fmt::Display::fmt(&f64::NAN, f), ValueMatch::I64(ref inner) => fmt::Display::fmt(inner, f), ValueMatch::U64(ref inner) => fmt::Display::fmt(inner, f), ValueMatch::Pat(ref inner) => fmt::Display::fmt(inner, f), @@ -275,6 +349,18 @@ impl SpanMatch { } impl<'a> Visit for MatchVisitor<'a> { + fn record_f64(&mut self, field: &Field, value: f64) { + match self.inner.fields.get(field) { + Some((ValueMatch::NaN, ref matched)) if value.is_nan() => { + matched.store(true, Release); + } + Some((ValueMatch::F64(ref e), ref matched)) if (value - *e).abs() < f64::EPSILON => { + matched.store(true, Release); + } + _ => {} + } + } + fn record_i64(&mut self, field: &Field, value: i64) { use std::convert::TryInto; diff --git a/tracing-subscriber/src/fmt/format/json.rs b/tracing-subscriber/src/fmt/format/json.rs index 8e06db111b..8c4137bb93 100644 --- a/tracing-subscriber/src/fmt/format/json.rs +++ b/tracing-subscriber/src/fmt/format/json.rs @@ -431,6 +431,12 @@ impl<'a> crate::field::VisitOutput for JsonVisitor<'a> { } impl<'a> field::Visit for JsonVisitor<'a> { + /// Visit a double precision floating point value. + fn record_f64(&mut self, field: &Field, value: f64) { + self.values + .insert(field.name(), serde_json::Value::from(value)); + } + /// Visit a signed 64-bit integer value. fn record_i64(&mut self, field: &Field, value: i64) { self.values diff --git a/tracing/tests/event.rs b/tracing/tests/event.rs index d61ef748b8..5d757d821a 100644 --- a/tracing/tests/event.rs +++ b/tracing/tests/event.rs @@ -145,6 +145,7 @@ fn one_with_everything() { ))) .and(field::mock("foo").with_value(&666)) .and(field::mock("bar").with_value(&false)) + .and(field::mock("like_a_butterfly").with_value(&42.0)) .only(), ) .at_level(Level::ERROR) @@ -157,7 +158,7 @@ fn one_with_everything() { event!( target: "whatever", Level::ERROR, - { foo = 666, bar = false }, + { foo = 666, bar = false, like_a_butterfly = 42.0 }, "{:#x} make me one with{what:.>20}", 4_277_009_102u64, what = "everything" ); }); diff --git a/tracing/tests/span.rs b/tracing/tests/span.rs index 406207d9c5..f7991931c5 100644 --- a/tracing/tests/span.rs +++ b/tracing/tests/span.rs @@ -458,6 +458,28 @@ fn move_field_out_of_struct() { handle.assert_finished(); } +#[cfg_attr(target_arch = "wasm32", wasm_bindgen_test::wasm_bindgen_test)] +#[test] +fn float_values() { + let (collector, handle) = collector::mock() + .new_span( + span::mock().named("foo").with_field( + field::mock("x") + .with_value(&3.234) + .and(field::mock("y").with_value(&-1.223)) + .only(), + ), + ) + .run_with_handle(); + + with_default(collector, || { + let foo = span!(Level::TRACE, "foo", x = 3.234, y = -1.223); + foo.in_scope(|| {}); + }); + + handle.assert_finished(); +} + // TODO(#1138): determine a new syntax for uninitialized span fields, and // re-enable these. /* diff --git a/tracing/tests/support/field.rs b/tracing/tests/support/field.rs index 25f17d5dc8..0e7799fa26 100644 --- a/tracing/tests/support/field.rs +++ b/tracing/tests/support/field.rs @@ -19,8 +19,9 @@ pub struct MockField { value: MockValue, } -#[derive(Debug, Eq, PartialEq)] +#[derive(Debug)] pub enum MockValue { + F64(f64), I64(i64), U64(u64), Bool(bool), @@ -29,6 +30,31 @@ pub enum MockValue { Any, } +impl Eq for MockValue {} + +impl PartialEq for MockValue { + fn eq(&self, other: &Self) -> bool { + use MockValue::*; + + match (self, other) { + (F64(a), F64(b)) => { + debug_assert!(!a.is_nan()); + debug_assert!(!b.is_nan()); + + a.eq(b) + } + (I64(a), I64(b)) => a.eq(b), + (U64(a), U64(b)) => a.eq(b), + (Bool(a), Bool(b)) => a.eq(b), + (Str(a), Str(b)) => a.eq(b), + (Debug(a), Debug(b)) => a.eq(b), + (Any, _) => true, + (_, Any) => true, + _ => false, + } + } +} + pub fn mock(name: K) -> MockField where String: From, @@ -120,6 +146,7 @@ impl Expect { impl fmt::Display for MockValue { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { + MockValue::F64(v) => write!(f, "f64 = {:?}", v), MockValue::I64(v) => write!(f, "i64 = {:?}", v), MockValue::U64(v) => write!(f, "u64 = {:?}", v), MockValue::Bool(v) => write!(f, "bool = {:?}", v), @@ -136,6 +163,11 @@ pub struct CheckVisitor<'a> { } impl<'a> Visit for CheckVisitor<'a> { + fn record_f64(&mut self, field: &Field, value: f64) { + self.expect + .compare_or_panic(field.name(), &value, &self.ctx[..]) + } + fn record_i64(&mut self, field: &Field, value: i64) { self.expect .compare_or_panic(field.name(), &value, &self.ctx[..]) @@ -180,6 +212,10 @@ impl<'a> From<&'a dyn Value> for MockValue { } impl Visit for MockValueBuilder { + fn record_f64(&mut self, _: &Field, value: f64) { + self.value = Some(MockValue::F64(value)); + } + fn record_i64(&mut self, _: &Field, value: i64) { self.value = Some(MockValue::I64(value)); }