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.

Sign up for GitHub

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

Add support for visiting floating point values #1507

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f1774e4
Add support for visiting floating point values
maxburke Aug 14, 2021
373a77f
Merge branch 'master' into add-float-visitor
maxburke Aug 17, 2021
0724604
Merge branch 'master' of github.com:tokio-rs/tracing into add-float-v…
maxburke Aug 20, 2021
7c6b83b
Code review feedback
maxburke Aug 20, 2021
8ded0f1
epsilon compare for f64 value match
maxburke Aug 22, 2021
b144620
extend tests to ensure float values are recorded
maxburke Aug 22, 2021
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
14 changes: 13 additions & 1 deletion tracing-core/src/field.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
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
View file
Open in desktop
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 `i64` values.
Copy link
Member

Choose a reason for hiding this comment

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

i think this should say

Suggested change
/// Set attributes on the underlying OpenTelemetry [`Span`] from `i64` values.
/// 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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
22 changes: 21 additions & 1 deletion tracing-subscriber/src/filter/env/field.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,14 +36,23 @@ pub(crate) struct MatchVisitor<'a> {
inner: &'a SpanMatch,
}

#[derive(Debug, Clone, PartialOrd, Ord, Eq, PartialEq)]
#[derive(Debug, Clone, PartialOrd, PartialEq)]
Copy link
Member

Choose a reason for hiding this comment

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

the manual implementation of Ord for a type that derives PartialOrd triggers a clippy lint: /~https://github.com/tokio-rs/tracing/runs/3363637695#step:4:753

I think we need to add a manual PartialOrd impl as well, to fix the clippy lint.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

pub(crate) enum ValueMatch {
Bool(bool),
F64(f64),
U64(u64),
I64(i64),
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 Ord for ValueMatch {
fn cmp(&self, other: &Self) -> Ordering {
self.partial_cmp(other).unwrap()
}
}
Copy link
Member

Choose a reason for hiding this comment

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

This isn't really correct, because there's nothing stopping the ValueMatch::F64 case from containing NaN values, and NaNs don't have a total order.

The total ordering is required because ValueMatches are stored in a BTreeMap, so we can't just remove the Ord implementation. Instead, what I think we should do is make some additional changes to ensure that ValueMatch::F64 variants will never contain NaN values, as I discussed in other comments. I think if we make those changes, we can then implement Ord for ValueMatch if we add an assertion when comparing ValueMatch::F64 variants that check that they aren't NaN (or expect the result of the partial_cmp comparison). For every other variant, the Ord impl can just call that type's Ord::cmp.

I would expect the implementation to look something like this:

Suggested change
impl Ord for ValueMatch {
fn cmp(&self, other: &Self) -> Ordering {
self.partial_cmp(other).unwrap()
}
}
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))
}
}

or similar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍


#[derive(Debug, Clone)]
pub(crate) struct MatchPattern {
pub(crate) matcher: Pattern,
Expand Down Expand Up @@ -134,6 +143,7 @@ impl FromStr for ValueMatch {
.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(ValueMatch::F64))
.or_else(|_| {
s.parse::<MatchPattern>()
.map(|p| ValueMatch::Pat(Box::new(p)))
Expand All @@ -145,6 +155,7 @@ 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::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 +286,15 @@ 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::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
View file
Open in desktop
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
15 changes: 14 additions & 1 deletion tracing/tests/support/field.rs
View file
Open in desktop
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, PartialEq)]
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.

I would probably put them in /~https://github.com/tokio-rs/tracing/blob/master/tracing/tests/event.rs and /~https://github.com/tokio-rs/tracing/blob/master/tracing/tests/span.rs

I64(i64),
U64(u64),
Bool(bool),
Expand All @@ -29,6 +30,8 @@ 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.

👍


pub fn mock<K>(name: K) -> MockField
where
String: From<K>,
Expand Down Expand Up @@ -120,6 +123,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 +140,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 +189,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