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

DisplayValue calls record_debug #306

Closed
samscott89 opened this issue Aug 30, 2019 · 5 comments
Closed

DisplayValue calls record_debug #306

samscott89 opened this issue Aug 30, 2019 · 5 comments
Labels
crate/core Related to the `tracing-core` crate kind/question Further information is requested

Comments

@samscott89
Copy link
Contributor

Bug Report

Version

tracing v0.1.5
tracing-core v0.1.4

Platform

Linux

Crates

tracing

Description

In the implementation of Value for DisplayValue, it calls visitor.record_debug. This is slightly surprising behaviour (but also pretty understandable when looking into how its implemented).

We hit this trying to visit_str on something where we did %var, and expected to catch the field.

I don't feel like this is necessarily a bug, but maybe just a little surprising.

@hawkw hawkw added crate/core Related to the `tracing-core` crate kind/question Further information is requested labels Aug 30, 2019
@hawkw
Copy link
Member

hawkw commented Aug 30, 2019

I can see how this has the potential to be surprising, but I am not sure if there is really any changes that we can make beyond documentation.

record_str takes an &str as an argument. Using the % sigil is requesting that a type be recorded as (essentially) a &dyn fmt::Display, which is not a &str, and turning it into one would require the instrumentation point to perform an allocation that the user cannot opt out from, which we've tried very hard to avoid.

At one point, I had considered calling it record_fmt rather than record_debug, and I suppose this would make it clearer that Display values also go to that method. We could change it by doing a deprecation cycle.

@SamScott do you think there are any docs changes that would make this any less surprising?

@samscott89
Copy link
Contributor Author

Sorry for the slow response.

You know, I can't say for certain, but there's a chance that the initial misconception came from this line:

A Value which serializes as a string using fmt::Display.

https://docs.rs/tracing/0.1.8/tracing/field/struct.DisplayValue.html

I think the "as a string" bit made me assume it would use the stringyness to record it.

@samscott89
Copy link
Contributor Author

Made an attempt at fixing.

@hawkw
Copy link
Member

hawkw commented Sep 19, 2019

@samscott89 do you think the docs change in #340 is sufficient to close this ticket, or is there anything else we ought to do?

@samscott89
Copy link
Contributor Author

It was a super minor issue anyway, so I'm happy with the changes :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crate/core Related to the `tracing-core` crate kind/question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants