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

attributes: don't record primitive types of the function arguments as fmt::Debug #1378

Merged
merged 3 commits into from
May 6, 2021

Conversation

Folyd
Copy link
Contributor

@Folyd Folyd commented Apr 29, 2021

Motivation

The default behavior of tracing::instrument attribution will record all of the function arguments as fmt::Debug, which is overwhelmed and unnecessary for those primitive types, such as bool, u8, i8, u16, i16, u32, i32,
u64, i64, usize, and isize. Another concerning reason is that we‘ll lose the type info of those primitive types when record by a Visitor, while those type infos is essential to some people. For example, I need to show my spans in Jaeger UI.

Solution

Make the tracing::instrument records other function arguments as fmt::Debug while not for those primitive types.

However, I'm not good at naming. Maybe the RecordType enum and its variant aren't a good name? I'd love to seek suggestions. Thanks.

@Folyd Folyd requested review from carllerche, davidbarsky, hawkw and a team as code owners April 29, 2021 04:21
@Folyd Folyd force-pushed the instrument-arguments branch from e2851e5 to 3f1a4fb Compare April 29, 2021 13:10
@Folyd
Copy link
Contributor Author

Folyd commented Apr 30, 2021

Here is the macro expansion diff of a simple function:

#[tracing::instrument] 
fn hard_work(unit: u64, a: u8) {
}
            ::tracing::Span::new(meta, &{
                #[allow(unused_imports)]
                use ::tracing::field::{debug, display, Value};
                let mut iter = meta.fields().iter();
                meta.fields().value_set(&[
                    (
                        &iter.next().expect("FieldSet corrupted (this is a bug)"),
-                       Some(&tracing::field::debug(&unit) as &Value),
+                       Some(&unit as &Value),
                    ),
                    (
                        &iter.next().expect("FieldSet corrupted (this is a bug)"),
-                      Some(&tracing::field::debug(&a) as &Value),
+                      Some(&a as &Value),
                    ),
                ])
            })

Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

Thanks for working on this --- overall, this change looks great. I had a few minor suggestions, most of which were documentation-related.

Also, would you mind moving some of the unrelated changes to separate PRs? I'd prefer to keep each commit in the git history as focused as possible. Thank you!

tracing-attributes/src/lib.rs Outdated Show resolved Hide resolved
tracing-attributes/src/lib.rs Show resolved Hide resolved
tracing-attributes/src/lib.rs Outdated Show resolved Hide resolved
tracing-attributes/src/lib.rs Outdated Show resolved Hide resolved
tracing-core/src/field.rs Outdated Show resolved Hide resolved
tracing-subscriber/src/subscribe.rs Outdated Show resolved Hide resolved
tracing-attributes/src/lib.rs Outdated Show resolved Hide resolved
tracing-attributes/src/lib.rs Outdated Show resolved Hide resolved
tracing-attributes/src/lib.rs Show resolved Hide resolved
@Folyd Folyd force-pushed the instrument-arguments branch from 3f1a4fb to d06c7d8 Compare May 1, 2021 03:17
hawkw pushed a commit that referenced this pull request May 3, 2021
Impl `Value` for `&mut T` where `T: Value`. 

#1378 improve the `tracing::instrument` macro's recording behavior: all
primitive types implementing the `Value` trait will be recorded as
fields of that type instead of `fmt::Debug`. This PR is a prerequisite
for those `&mut` function arguments to achieve that.
@Folyd Folyd force-pushed the instrument-arguments branch from e73ee61 to 292cd06 Compare May 4, 2021 14:44
@Folyd Folyd force-pushed the instrument-arguments branch from 292cd06 to 3c61a44 Compare May 4, 2021 16:15
@hawkw hawkw merged commit b6c07e7 into tokio-rs:master May 6, 2021
@Folyd Folyd deleted the instrument-arguments branch May 7, 2021 03:01
@Folyd
Copy link
Contributor Author

Folyd commented May 7, 2021

BTW, does this should be ported to v0.1.x?

Folyd added a commit to Folyd/tracing that referenced this pull request May 31, 2021
… `fmt::Debug` (tokio-rs#1378)

The default behavior of `tracing::instrument` attribution will record
all of the function arguments as `fmt::Debug`, which is overwhelmed and
unnecessary for those primitive types, such as `bool`, `u8`, `i8`,
`u16`, `i16`, `u32`, `i32`, `u64`, `i64`, `usize`, and `isize`. Another
concerning reason is that we‘ll lose the type info of those primitive
types when record by a `Visitor`, while those type infos is essential to
some people. For example, I need to show my spans in Jaeger UI.

Make the `tracing::instrument` records other function arguments as
`fmt::Debug ` while not for those primitive types.

However, I'm not good at naming. Maybe the `RecordType` enum and its
variant aren't a good name? I'd love to seek suggestions. Thanks.
Folyd added a commit to Folyd/tracing that referenced this pull request May 31, 2021
Impl `Value` for `&mut T` where `T: Value`. 

tokio-rs#1378 improve the `tracing::instrument` macro's recording behavior: all
primitive types implementing the `Value` trait will be recorded as
fields of that type instead of `fmt::Debug`. This PR is a prerequisite
for those `&mut` function arguments to achieve that.
Folyd added a commit to Folyd/tracing that referenced this pull request May 31, 2021
… `fmt::Debug` (tokio-rs#1378)

The default behavior of `tracing::instrument` attribution will record
all of the function arguments as `fmt::Debug`, which is overwhelmed and
unnecessary for those primitive types, such as `bool`, `u8`, `i8`,
`u16`, `i16`, `u32`, `i32`, `u64`, `i64`, `usize`, and `isize`. Another
concerning reason is that we‘ll lose the type info of those primitive
types when record by a `Visitor`, while those type infos is essential to
some people. For example, I need to show my spans in Jaeger UI.

Make the `tracing::instrument` records other function arguments as
`fmt::Debug ` while not for those primitive types.

However, I'm not good at naming. Maybe the `RecordType` enum and its
variant aren't a good name? I'd love to seek suggestions. Thanks.
Folyd added a commit to Folyd/tracing that referenced this pull request May 31, 2021
Impl `Value` for `&mut T` where `T: Value`. 

tokio-rs#1378 improve the `tracing::instrument` macro's recording behavior: all
primitive types implementing the `Value` trait will be recorded as
fields of that type instead of `fmt::Debug`. This PR is a prerequisite
for those `&mut` function arguments to achieve that.
hawkw pushed a commit that referenced this pull request Jun 1, 2021
… `fmt::Debug` (#1378)

The default behavior of `tracing::instrument` attribution will record
all of the function arguments as `fmt::Debug`, which is overwhelmed and
unnecessary for those primitive types, such as `bool`, `u8`, `i8`,
`u16`, `i16`, `u32`, `i32`, `u64`, `i64`, `usize`, and `isize`. Another
concerning reason is that we‘ll lose the type info of those primitive
types when record by a `Visitor`, while those type infos is essential to
some people. For example, I need to show my spans in Jaeger UI.

Make the `tracing::instrument` records other function arguments as
`fmt::Debug ` while not for those primitive types.

However, I'm not good at naming. Maybe the `RecordType` enum and its
variant aren't a good name? I'd love to seek suggestions. Thanks.
hawkw pushed a commit that referenced this pull request Jun 1, 2021
Impl `Value` for `&mut T` where `T: Value`. 

#1378 improve the `tracing::instrument` macro's recording behavior: all
primitive types implementing the `Value` trait will be recorded as
fields of that type instead of `fmt::Debug`. This PR is a prerequisite
for those `&mut` function arguments to achieve that.
hawkw added a commit that referenced this pull request Sep 13, 2021
# 0.1.16 (September 13, 2021)

This release adds a new `#[instrument(skip_all)]` option to skip
recording *all* arguments to an instrumented function as fields.
Additionally, it adds support for recording arguments that are `tracing`
primitive types as typed values, rather than as `fmt::Debug`.

### Added

- add `skip_all` option to `#[instrument]` ([#1548])
- record primitive types as primitive values rather than as `fmt::Debug`
  ([#1378])
- added support for `f64`s as typed values ([#1522])

Thanks to @Folyd and @jsgf for contributing to this release!

[#1548]: #1548
[#1378]: #1378
[#1522]: #1524
hawkw added a commit that referenced this pull request Sep 13, 2021
# 0.1.16 (September 13, 2021)

This release adds a new `#[instrument(skip_all)]` option to skip
recording *all* arguments to an instrumented function as fields.
Additionally, it adds support for recording arguments that are `tracing`
primitive types as typed values, rather than as `fmt::Debug`.

### Added

- add `skip_all` option to `#[instrument]` ([#1548])
- record primitive types as primitive values rather than as `fmt::Debug`
  ([#1378])
- added support for `f64`s as typed values ([#1522])

Thanks to @Folyd and @jsgf for contributing to this release!

[#1548]: #1548
[#1378]: #1378
[#1522]: #1524
hawkw added a commit that referenced this pull request Sep 13, 2021
# 0.1.27 (September 13, 2021)

This release adds a new [`Span::or_current`] method to aid in
efficiently propagating span contexts to spawned threads or tasks.
Additionally, it updates the [`tracing-core`] version to [0.1.20] and
the [`tracing-attributes`] version to [0.1.16], ensuring that a number
of new features in those crates are present.

### Fixed

- **instrument**: Added missing `WithSubscriber` implementations for
  futures and other types ([#1424])

### Added

- `Span::or_current` method, to help with efficient span context
  propagation ([#1538])
- **attributes**: add `skip_all` option to `#[instrument]` ([#1548])
- **attributes**: record primitive types as primitive values rather than
  as `fmt::Debug` ([#1378])
- **core**: `NoSubscriber`, a no-op `Subscriber` implementation
  ([#1549])
- **core**: Added `Visit::record_f64` and support for recording
  floating-point values ([#1507], [#1522])
- A large number of documentation improvements and fixes ([#1369],
  [#1398], [#1435], [#1442], [#1524], [#1556])

Thanks to new contributors @dzvon and @mbergkvist, as well as @teozkr,
@maxburke, @LukeMathWalker, and @jsgf, for contributing to this release!

[`Span::or_current`]: https://docs.rs/tracing/0.1.27/tracing/struct.Span.html#method.or_current
[`tracing-core`]: https://crates.io/crates/tracing-core
[`tracing-attributes`]: https://crates.io/crates/tracing-attributes
[`tracing-core`]: https://crates.io/crates/tracing-core
[0.1.20]: https://github.com/tokio-rs/tracing/releases/tag/tracing-core-0.1.20
[0.1.16]: https://github.com/tokio-rs/tracing/releases/tag/tracing-attributes-0.1.16
[#1424]: #1424
[#1538]: #1538
[#1548]: #1548
[#1378]: #1378
[#1507]: #1507
[#1522]: #1522
[#1369]: #1369
[#1398]: #1398
[#1435]: #1435
[#1442]: #1442
hawkw added a commit that referenced this pull request Sep 13, 2021
# 0.1.27 (September 13, 2021)

This release adds a new [`Span::or_current`] method to aid in
efficiently propagating span contexts to spawned threads or tasks.
Additionally, it updates the [`tracing-core`] version to [0.1.20] and
the [`tracing-attributes`] version to [0.1.16], ensuring that a number
of new features in those crates are present.

### Fixed

- **instrument**: Added missing `WithSubscriber` implementations for
  futures and other types (#1424)

### Added

- `Span::or_current` method, to help with efficient span context
  propagation (#1538)
- **attributes**: add `skip_all` option to `#[instrument]` (#1548)
- **attributes**: record primitive types as primitive values rather than
  as `fmt::Debug` (#1378)
- **core**: `NoSubscriber`, a no-op `Subscriber` implementation
  (#1549)
- **core**: Added `Visit::record_f64` and support for recording
  floating-point values (#1507, #1522)
- A large number of documentation improvements and fixes (#1369,
  #1398, #1435, #1442, #1524, #1556)

Thanks to new contributors @dzvon and @mbergkvist, as well as @teozkr,
@maxburke, @LukeMathWalker, and @jsgf, for contributing to this release!

[`Span::or_current`]: https://docs.rs/tracing/0.1.27/tracing/struct.Span.html#method.or_current
[`tracing-core`]: https://crates.io/crates/tracing-core
[`tracing-attributes`]: https://crates.io/crates/tracing-attributes
[`tracing-core`]: https://crates.io/crates/tracing-core
[0.1.20]: https://github.com/tokio-rs/tracing/releases/tag/tracing-core-0.1.20
[0.1.16]: https://github.com/tokio-rs/tracing/releases/tag/tracing-attributes-0.1.16
@CAD97
Copy link
Contributor

CAD97 commented Oct 3, 2021

I believe this was a sneakily breaking change on 0.1.X, in that instrumented argument captures of &str changed from matching field filter regexes of "value" to matching value.

kaffarell added a commit to kaffarell/tracing that referenced this pull request Oct 30, 2023
There was an error when backporting tokio-rs#1378 (here: tokio-rs#1418) and a trailing
dot (.) was forgotten (which was breaking the link). Fixed also the link to
`std::fmt::Debug`.
kaffarell added a commit to kaffarell/tracing that referenced this pull request Oct 30, 2023
There was an error when backporting tokio-rs#1378 (here: tokio-rs#1418) and a trailing
dot (.) was forgotten (which was breaking the link). Fixed also the link to
`std::fmt::Debug`.
davidbarsky pushed a commit that referenced this pull request Oct 30, 2023
There was an error when backporting #1378 (here: #1418) and a trailing
dot (.) was forgotten (which was breaking the link). Fixed also the link to
`std::fmt::Debug`.
kaffarell pushed a commit to kaffarell/tracing that referenced this pull request May 22, 2024
# 0.1.16 (September 13, 2021)

This release adds a new `#[instrument(skip_all)]` option to skip
recording *all* arguments to an instrumented function as fields.
Additionally, it adds support for recording arguments that are `tracing`
primitive types as typed values, rather than as `fmt::Debug`.

### Added

- add `skip_all` option to `#[instrument]` ([tokio-rs#1548])
- record primitive types as primitive values rather than as `fmt::Debug`
  ([tokio-rs#1378])
- added support for `f64`s as typed values ([tokio-rs#1522])

Thanks to @Folyd and @jsgf for contributing to this release!

[tokio-rs#1548]: tokio-rs#1548
[tokio-rs#1378]: tokio-rs#1378
[tokio-rs#1522]: tokio-rs#1524
kaffarell pushed a commit to kaffarell/tracing that referenced this pull request May 22, 2024
# 0.1.27 (September 13, 2021)

This release adds a new [`Span::or_current`] method to aid in
efficiently propagating span contexts to spawned threads or tasks.
Additionally, it updates the [`tracing-core`] version to [0.1.20] and
the [`tracing-attributes`] version to [0.1.16], ensuring that a number
of new features in those crates are present.

### Fixed

- **instrument**: Added missing `WithSubscriber` implementations for
  futures and other types (tokio-rs#1424)

### Added

- `Span::or_current` method, to help with efficient span context
  propagation (tokio-rs#1538)
- **attributes**: add `skip_all` option to `#[instrument]` (tokio-rs#1548)
- **attributes**: record primitive types as primitive values rather than
  as `fmt::Debug` (tokio-rs#1378)
- **core**: `NoSubscriber`, a no-op `Subscriber` implementation
  (tokio-rs#1549)
- **core**: Added `Visit::record_f64` and support for recording
  floating-point values (tokio-rs#1507, tokio-rs#1522)
- A large number of documentation improvements and fixes (tokio-rs#1369,
  tokio-rs#1398, tokio-rs#1435, tokio-rs#1442, tokio-rs#1524, tokio-rs#1556)

Thanks to new contributors @dzvon and @mbergkvist, as well as @teozkr,
@maxburke, @LukeMathWalker, and @jsgf, for contributing to this release!

[`Span::or_current`]: https://docs.rs/tracing/0.1.27/tracing/struct.Span.html#method.or_current
[`tracing-core`]: https://crates.io/crates/tracing-core
[`tracing-attributes`]: https://crates.io/crates/tracing-attributes
[`tracing-core`]: https://crates.io/crates/tracing-core
[0.1.20]: https://github.com/tokio-rs/tracing/releases/tag/tracing-core-0.1.20
[0.1.16]: https://github.com/tokio-rs/tracing/releases/tag/tracing-attributes-0.1.16
kaffarell added a commit to kaffarell/tracing that referenced this pull request May 22, 2024
There was an error when backporting tokio-rs#1378 (here: tokio-rs#1418) and a trailing
dot (.) was forgotten (which was breaking the link). Fixed also the link to
`std::fmt::Debug`.
davidbarsky pushed a commit that referenced this pull request Oct 2, 2024
There was an error when backporting #1378 (here: #1418) and a trailing
dot (.) was forgotten (which was breaking the link). Fixed also the link to
`std::fmt::Debug`.
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.

4 participants