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

Recording for Floats #1370

Closed
wants to merge 2 commits into from
Closed

Conversation

jracollins
Copy link

Motivation

Currently floats cannot be logged/recorded other than in Debug or Display format which ends up stringifying them on output.

Simple Example:

#[macro_use]
extern crate tracing;

use std::error::Error;

fn main() -> Result<(), Box<dyn Error>> {
    let builder = tracing_subscriber::fmt().json();
    builder.init();

    let integer: i64 = 25;
    let float: f64 = 25.567;

    event!(tracing::Level::INFO, integer = integer, "Test Integer");
    event!(tracing::Level::INFO, float = %float, "Test Display Float");
    event!(tracing::Level::INFO, float = ?float, "Test Debug Float");

    Ok(())
}

results in:

{"timestamp":"Apr 25 14:15:35.469","level":"INFO","fields":{"message":"Test Integer","integer":25},"target":"tracing_test"}
{"timestamp":"Apr 25 14:15:35.469","level":"INFO","fields":{"message":"Test Display Float","float":"25.567"},"target":"tracing_test"}
{"timestamp":"Apr 25 14:15:35.469","level":"INFO","fields":{"message":"Test Debug Float","float":"25.567"},"target":"tracing_test"}

And won't compile if passed the float directly:

error[E0277]: the trait bound `f64: Value` is not satisfied
  --> src/main.rs:15:5
   |
15 |     event!(tracing::Level::INFO, float = float, "Test Float");
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `Value` is not implemented for `f64`
   |
   = note: required for the cast to the object type `dyn Value`
   = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)

error: aborting due to previous error

Log ingesting/processing tools (eg Datadog/ELK etc) cannot parse these float strings without custom rules for each field to try and coerce the outputted string into the appropriate type, which is required in order to generate metrics etc.

This resolves issue mentioned in the Tokio Discord a few times and #726

Solution

In order to implement I have added the relevant record_64 functions.

However, in order to get it working I had to comment out the impl_one_value!(nonzero, ...) macro.

I don't currently understand the benefits of the implementation (why it is needed), or how to avoid it causing conflict with the float implementation.

Before I write/fix the tests for the implementation/remove current NonZero failing test, if a reviewer could explain why it is needed and offer pointers as to how to work around it, or green light its removal it would be much appreciated 🙏

@jracollins jracollins requested review from carllerche, davidbarsky, hawkw and a team as code owners April 25, 2021 13:38
@hawkw
Copy link
Member

hawkw commented Oct 15, 2021

Obsoleted by #1507.

@hawkw hawkw closed this Oct 15, 2021
@jracollins
Copy link
Author

Hi @hawkw

I just updated my app to use version 1.29 instead of this PR's forked version, and whilst it does now accept floats as input values, the actual output of the floats are stringified? (Which makes it basically the same as before using output = %float_value) and is one of the problems described that I was trying to fix?

i.e. output from

fn main() -> Result<(), Box<dyn Error>> {
    tracing_subscriber::fmt().json().init();
    event!(tracing::Level::INFO, float = 0.24, "Float");
    Ok(())
}

using 1.29 is:

{"timestamp":"Oct 22 13:29:02.154","level":"INFO","fields":{"message":"Float","float":"0.24"},"target":"trace_test"}

Where float is "0.24" rather than 0.24

@w1th0utnam3
Copy link

I just updated my app to use version 1.29 instead of this PR's forked version, and whilst it does now accept floats as input values, the actual output of the floats are stringified?

This still seems to be an open problem as of now. Is there an update on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants