Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for visiting floating point values #1507

Merged
merged 6 commits into from
Aug 25, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 16 additions & 4 deletions tracing-core/src/field.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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. Collectors 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. Collectors 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
Expand Down Expand Up @@ -179,6 +179,11 @@ pub struct Iter {
/// [set of `Value`s added to a `Span`]: super::collect::Collect::record
/// [`Event`]: super::event::Event
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)
Expand Down Expand Up @@ -330,6 +335,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);
};
Expand Down Expand Up @@ -382,7 +393,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<T: crate::sealed::Sealed> crate::sealed::Sealed for Wrapping<T> {}
Expand Down
22 changes: 22 additions & 0 deletions tracing-opentelemetry/src/subscriber.rs
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,21 @@ impl<'a> field::Visit for SpanEventVisitor<'a> {
}
}

/// Record events on the underlying OpenTelemetry [`Span`] from `f64` values.
///
/// [`Span`]: opentelemetry::trace::Span
fn record_f64(&mut self, field: &field::Field, value: f64) {
match field.name() {
"message" => self.0.name = value.to_string().into(),
// Skip fields that are actually log metadata that have already been handled
#[cfg(feature = "tracing-log")]
name if name.starts_with("log.") => (),
name => {
self.0.attributes.push(KeyValue::new(name, value));
}
}
}

/// Record events on the underlying OpenTelemetry [`Span`] from `i64` values.
///
/// [`Span`]: opentelemetry::trace::Span
Expand Down Expand Up @@ -197,6 +212,13 @@ impl<'a> field::Visit for SpanAttributeVisitor<'a> {
self.record(KeyValue::new(field.name(), value));
}

/// Set attributes on the underlying OpenTelemetry [`Span`] from `f64` values.
///
/// [`Span`]: opentelemetry::trace::Span
fn record_f64(&mut self, field: &field::Field, value: f64) {
self.record(KeyValue::new(field.name(), value));
}

/// Set attributes on the underlying OpenTelemetry [`Span`] from `i64` values.
///
/// [`Span`]: opentelemetry::trace::Span
Expand Down
12 changes: 12 additions & 0 deletions tracing-serde/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -403,6 +403,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)
Expand Down Expand Up @@ -449,6 +455,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)
Expand Down
5 changes: 5 additions & 0 deletions tracing-subscriber/src/field/debug.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,11 @@ impl<V> Visit for Alt<V>
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)
Expand Down
5 changes: 5 additions & 0 deletions tracing-subscriber/src/field/display.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,11 @@ impl<V> Visit for Messages<V>
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)
Expand Down
88 changes: 87 additions & 1 deletion tracing-subscriber/src/filter/env/field.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<MatchPattern>),
}

impl Eq for ValueMatch {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly to Ord, the Eq implementation is only correct if we ensure there are no NaN values present in the ValueMatch::F64 variant. See my comment on the Ord impl for details.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍


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<Ordering> {
Some(self.cmp(other))
}
}

#[derive(Debug, Clone)]
pub(crate) struct MatchPattern {
pub(crate) matcher: Pattern,
Expand Down Expand Up @@ -127,13 +190,22 @@ 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<Self, Self::Err> {
s.parse::<bool>()
.map(ValueMatch::Bool)
.or_else(|_| s.parse::<u64>().map(ValueMatch::U64))
.or_else(|_| s.parse::<i64>().map(ValueMatch::I64))
.or_else(|_| s.parse::<f64>().map(value_match_f64))
.or_else(|_| {
s.parse::<MatchPattern>()
.map(|p| ValueMatch::Pat(Box::new(p)))
Expand All @@ -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),
Expand Down Expand Up @@ -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;

Expand Down
6 changes: 6 additions & 0 deletions tracing-subscriber/src/fmt/format/json.rs
Original file line number Diff line number Diff line change
Expand Up @@ -425,6 +425,12 @@ impl<'a> crate::field::VisitOutput<fmt::Result> 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
Expand Down
3 changes: 2 additions & 1 deletion tracing/tests/event.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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"
);
});
Expand Down
22 changes: 22 additions & 0 deletions tracing/tests/span.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
/*
Expand Down
38 changes: 37 additions & 1 deletion tracing/tests/support/field.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,9 @@ pub struct MockField {
value: MockValue,
}

#[derive(Debug, Eq, PartialEq)]
#[derive(Debug)]
pub enum MockValue {
F64(f64),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor nit, but since the test support code now handles f64 values, we should probably also add some tests that actually try to record f64s...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where would be the best place to put these? I looked for example in the tracing-core/src/field.rs but most of the tests there were calling record_debug directly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I64(i64),
U64(u64),
Bool(bool),
Expand All @@ -29,6 +30,31 @@ pub enum MockValue {
Any,
}

impl Eq for MockValue {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only correct if we ensure no NaN values are present in the F64 case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍


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<K>(name: K) -> MockField
where
String: From<K>,
Expand Down Expand Up @@ -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),
Expand All @@ -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[..])
Expand Down Expand Up @@ -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));
}
Expand Down