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 4 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 => {
Copy link
Member

Choose a reason for hiding this comment

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

We probably need special behavior for NaN values here. Two NaNs will never be equal to each other. However, a user may want to write a filter configuration that enables all spans where a particular value is a NaN. Therefore, we probably want to change this behavior.

I would probably change this by adding a separate ValueMatch::NaN type, and change the parser to return that instead if s.parse::<f64>() returns a value where is_nan() == true. Then, in this method, we would add another match arm like

            Some((ValueMatch::NaN, ref matched)) if value.is_nan() => {
                 matched.store(true, Release);
            }

This way, users can match any NaN value by writing filters like {foo=NaN}, which is probably the desired behavior.

This also avoids the problem of NaNs in the Ord impl breaking the total ordering for ValueMatches. Since no ValueMatch::F64 variants will be created with NaN values, we actually can rely on the ordering of ValueMatch::F64s being total. We would probably want to add a (debug?) assertion in that case that the values are not actually NaN, to ensure no NaN values sneak in accidentally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hawkw I had a question for this particular code location, unrelated to the NaN support. Should this method here perform an equality compare against the matching value, or should it do an epsilon compare?

Clippy warns against doing an equality compare but I'm not sure it feels right to be doing an epsilon compare here.

Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

It's probably best to do an epsilon compare here. In most cases, I think the user will be looking for values that are logically "equal" to the provided decimal number, rather than looking for floating-point values whose bit patterns are exactly equal.

Either way, it's probably worth documenting how floats are compared in the documentation for EnvFilter, whether we do an exact comparison or an epsilon comparison.

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
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