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

core: impl field::Value for Option<T> #1585

Merged
merged 7 commits into from
Sep 24, 2021
Merged

Conversation

bryanburgers
Copy link
Contributor

I'm very interested in #1393 getting merged. Yep, I'm aware that the Valuable work may supersede this, but in the mean time this would be a huge improvement for how we use tracing.

Based on this comment #1393 (review) by @hawkw it looks like the only thing holding up #1393 is some tests? If so, here is one test. Let me know what other tests you'd like to see and I'll get them added.

I wasn't quite sure how to assert that the mock did not record fields none_str, none_bool, etc.

@bryanburgers
Copy link
Contributor Author

Ah yeah, I also rebased it onto master, if that matters.

@hawkw
Copy link
Member

hawkw commented Sep 21, 2021

Ugh...the CI failure isn't your fault, it looks like a recent release of a dependency (http) broke MSRV compatibility...

@bryanburgers
Copy link
Contributor Author

Ugh...the CI failure isn't your fault, it looks like a recent release of a dependency (http) broke MSRV compatibility...

OK, I won't worry about that one. (The force push was to fix a legitimate style test failure.)

@bryanburgers
Copy link
Contributor Author

Actually, now that I think about it, does it make more sense to PR this against the v0.1.x branch?

I'm guessing master will turn into 0.2? So yeah, probably makes sense to use the 0.1 branch.

If that makes sense, I'll rebase tomorrow morning.

@CAD97
Copy link
Contributor

CAD97 commented Sep 22, 2021

Ugh...the CI failure isn't your fault, it looks like a recent release of a dependency (http) broke MSRV compatibility...

hyperium/http#481, includes context on (deliberate) MSRV bump

DCjanus and others added 2 commits September 22, 2021 07:03
Add tests for making an `Option<T> where T: Value` a value in itself.
@bryanburgers
Copy link
Contributor Author

Rebased to PR off of v0.1.x because this change seems appropriate to get into the 0.1.x line, but may be completely unnecessary in the 0.2.x line if 0.2.x comes with full Valuable support.

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, this looks good to me overall!

Rebased to PR off of v0.1.x because this change seems appropriate to get into the 0.1.x line, but may be completely unnecessary in the 0.2.x line if 0.2.x comes with full Valuable support.

Yes, I think basing this on v0.1.x seems correct; the valuable support on 0.2 will make this unnecessary (and may, in fact, result in a conflicting impl, since I believe Options already have Valuable impls in valuable).

Based on this comment #1393 (review) by @hawkw it looks like the only thing holding up #1393 is some tests? If so, here is one test. Let me know what other tests you'd like to see and I'll get them added.

Yeah, the tests were kind of a blocker because I thought it was important that code with expected uses of this API actually compiles and works as expected. :) I think we probably also want to add tests for references to Options, for that reason.

A couple other things that would be nice to do, but can be done in follow-up branches:

  • it's probably worth adding examples of recording Option values in the docs, maybe in this section?
    //! ### Recording Fields
  • Also, we'll want to add support for Option values in the instrument attribute, somewhere around here:
    /// Array of primitive types which should be recorded as [RecordType::Value].
    const TYPES_FOR_VALUE: &'static [&'static str] = &[
    "bool",
    "str",
    "u8",
    "i8",
    "u16",
    "i16",
    "u32",
    "i32",
    "u64",
    "i64",
    "f32",
    "f64",
    "usize",
    "isize",
    "NonZeroU8",
    "NonZeroI8",
    "NonZeroU16",
    "NonZeroI16",
    "NonZeroU32",
    "NonZeroI32",
    "NonZeroU64",
    "NonZeroI64",
    "NonZeroUsize",
    "NonZeroIsize",
    "Wrapping",
    ];
    /// Parse `RecordType` from [syn::Type] by looking up
    /// the [RecordType::TYPES_FOR_VALUE] array.
    fn parse_from_ty(ty: &syn::Type) -> Self {
    match ty {
    syn::Type::Path(syn::TypePath { path, .. })
    if path
    .segments
    .iter()
    .last()
    .map(|path_segment| {
    let ident = path_segment.ident.to_string();
    Self::TYPES_FOR_VALUE.iter().any(|&t| t == ident)
    })
    .unwrap_or(false) =>
    {
    RecordType::Value
    }
    syn::Type::Reference(syn::TypeReference { elem, .. }) => {
    RecordType::parse_from_ty(&*elem)
    }
    _ => RecordType::Debug,
    }
    }
    That way, a function with #[instrument] can record any Option arguments as typed Option values when the value inside the option implements Value.

But, those can both be done in subsequent changes, so we can get this merged and move forward!

tracing/tests/event.rs Outdated Show resolved Hide resolved
tracing/tests/event.rs Show resolved Hide resolved
This adds tests for references to options, and adds `.only()` to the expected fields in the tests.
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.

i went ahead and applied my test suggestions, and once CI passes, we can merge this, and make the follow-up changes in other branches!

tracing/tests/event.rs Outdated Show resolved Hide resolved
hahaha lol never edit code in the github webapp suggestions UI. i have some regrets.
tracing/tests/event.rs Outdated Show resolved Hide resolved
hawkw added a commit that referenced this pull request Oct 1, 2021
# 0.1.21 (October 1, 2021)

This release adds support for recording `Option<T> where T: Value` as
typed `tracing` field values.

### Added

- **field**: `Value` impl for `Option<T> where T: Value` ([#1585])

### Fixed

- Fixed deprecation warnings when building with `default-features`
  disabled ([#1603], [#1606])
- Documentation fixes and improvements ([#1595], [#1601])

Thanks to @BrianBurgers, @DCjanus, and @matklad for contributing to this
release!

[#1585]: #1585
[#1595]: #1595
[#1601]: #1601
[#1603]: #1603
[#1606]: #1606
hawkw added a commit that referenced this pull request Oct 1, 2021
# 0.1.21 (October 1, 2021)

This release adds support for recording `Option<T> where T: Value` as
typed `tracing` field values.

### Added

- **field**: `Value` impl for `Option<T> where T: Value` ([#1585])

### Fixed

- Fixed deprecation warnings when building with `default-features`
  disabled ([#1603], [#1606])
- Documentation fixes and improvements ([#1595], [#1601])

Thanks to @BrianBurgers, @DCjanus, and @matklad for contributing to this
release!

[#1585]: #1585
[#1595]: #1595
[#1601]: #1601
[#1603]: #1603
[#1606]: #1606
@CAD97
Copy link
Contributor

CAD97 commented Oct 3, 2021

Would it make sense to use Empty for the None case? Or does that just end up with the same result?

hawkw added a commit that referenced this pull request Oct 5, 2021
# 0.1.29 (October 5th, 2021

This release adds support for recording `Option<T> where T: Value` as
typed `tracing` field values. It also includes significant performance
improvements for functions annotated with the `#[instrument]` attribute
when the generated span is disabled.

### Changed

- `tracing-core`: updated to v0.1.21
- `tracing-attributes`: updated to v0.1.19

### Added

- **field**: `Value` impl for `Option<T> where T: Value` ([#1585])
- **attributes**: - improved performance when skipping
  `#[instrument]`-generated spans below the max level ([#1600], [#1605],
  [#1614], [#1616], [#1617])

### Fixed

- **instrument**: added missing `Future` implementation for
  `WithSubscriber`, making the `WithDispatch` extension trait actually
  useable ([#1602])
- Documentation fixes and improvements ([#1595], [#1601], [#1597])

Thanks to @BrianBurgers, @mattiast, @DCjanus, @oli-obk, and @matklad for
contributing to this release!

[#1585]: #1585
[#1595]: #1596
[#1597]: #1597
[#1600]: #1600
[#1601]: #1601
[#1602]: #1602
[#1614]: #1614
[#1616]: #1616
[#1617]: #1617
hawkw added a commit that referenced this pull request Oct 5, 2021
# 0.1.29 (October 5th, 2021

This release adds support for recording `Option<T> where T: Value` as
typed `tracing` field values. It also includes significant performance
improvements for functions annotated with the `#[instrument]` attribute
when the generated span is disabled.

### Changed

- `tracing-core`: updated to v0.1.21
- `tracing-attributes`: updated to v0.1.19

### Added

- **field**: `Value` impl for `Option<T> where T: Value` ([#1585])
- **attributes**: - improved performance when skipping
  `#[instrument]`-generated spans below the max level ([#1600], [#1605],
  [#1614], [#1616], [#1617])

### Fixed

- **instrument**: added missing `Future` implementation for
  `WithSubscriber`, making the `WithDispatch` extension trait actually
  useable ([#1602])
- Documentation fixes and improvements ([#1595], [#1601], [#1597])

Thanks to @BrianBurgers, @mattiast, @DCjanus, @oli-obk, and @matklad for
contributing to this release!

[#1585]: #1585
[#1595]: #1596
[#1597]: #1597
[#1600]: #1600
[#1601]: #1601
[#1602]: #1602
[#1614]: #1614
[#1616]: #1616
[#1617]: #1617
hawkw added a commit that referenced this pull request Oct 6, 2021
# 0.1.29 (October 5th, 2021

This release adds support for recording `Option<T> where T: Value` as
typed `tracing` field values. It also includes significant performance
improvements for functions annotated with the `#[instrument]` attribute
when the generated span is disabled.

### Changed

- `tracing-core`: updated to v0.1.21
- `tracing-attributes`: updated to v0.1.19

### Added

- **field**: `Value` impl for `Option<T> where T: Value` ([#1585])
- **attributes**: - improved performance when skipping
  `#[instrument]`-generated spans below the max level ([#1600], [#1605],
  [#1614], [#1616], [#1617])

### Fixed

- **instrument**: added missing `Future` implementation for
  `WithSubscriber`, making the `WithDispatch` extension trait actually
  useable ([#1602])
- Documentation fixes and improvements ([#1595], [#1601], [#1597])

Thanks to @BrianBurgers, @mattiast, @DCjanus, @oli-obk, and @matklad for
contributing to this release!

[#1585]: #1585
[#1595]: #1596
[#1597]: #1597
[#1600]: #1600
[#1601]: #1601
[#1602]: #1602
[#1605]: #1605
[#1614]: #1614
[#1616]: #1616
[#1617]: #1617
AmoleR added a commit to Random-Math/tracing that referenced this pull request Oct 10, 2023
kaffarell pushed a commit to kaffarell/tracing that referenced this pull request May 22, 2024
## Motivation

Building a span with some field might be `None` is bother now.

Before:
```rust
let span = info!("example", data = Empty );
if let Some(data) = foo() {
    span.record("data", &data);
}
```

After:
```rust
let span = info!("example", data = foo() );
```

Co-authored-by: DCjanus <DCjanus@dcjanus.com>
Co-authored-by: Eliza Weisman <eliza@buoyant.io>
kaffarell pushed a commit to kaffarell/tracing that referenced this pull request May 22, 2024
# 0.1.21 (October 1, 2021)

This release adds support for recording `Option<T> where T: Value` as
typed `tracing` field values.

### Added

- **field**: `Value` impl for `Option<T> where T: Value` ([tokio-rs#1585])

### Fixed

- Fixed deprecation warnings when building with `default-features`
  disabled ([tokio-rs#1603], [tokio-rs#1606])
- Documentation fixes and improvements ([tokio-rs#1595], [tokio-rs#1601])

Thanks to @BrianBurgers, @DCjanus, and @matklad for contributing to this
release!

[tokio-rs#1585]: tokio-rs#1585
[tokio-rs#1595]: tokio-rs#1595
[tokio-rs#1601]: tokio-rs#1601
[tokio-rs#1603]: tokio-rs#1603
[tokio-rs#1606]: tokio-rs#1606
kaffarell pushed a commit to kaffarell/tracing that referenced this pull request May 22, 2024
# 0.1.29 (October 5th, 2021

This release adds support for recording `Option<T> where T: Value` as
typed `tracing` field values. It also includes significant performance
improvements for functions annotated with the `#[instrument]` attribute
when the generated span is disabled.

### Changed

- `tracing-core`: updated to v0.1.21
- `tracing-attributes`: updated to v0.1.19

### Added

- **field**: `Value` impl for `Option<T> where T: Value` ([tokio-rs#1585])
- **attributes**: - improved performance when skipping
  `#[instrument]`-generated spans below the max level ([tokio-rs#1600], [tokio-rs#1605],
  [tokio-rs#1614], [tokio-rs#1616], [tokio-rs#1617])

### Fixed

- **instrument**: added missing `Future` implementation for
  `WithSubscriber`, making the `WithDispatch` extension trait actually
  useable ([tokio-rs#1602])
- Documentation fixes and improvements ([tokio-rs#1595], [tokio-rs#1601], [tokio-rs#1597])

Thanks to @BrianBurgers, @mattiast, @DCjanus, @oli-obk, and @matklad for
contributing to this release!

[tokio-rs#1585]: tokio-rs#1585
[tokio-rs#1595]: tokio-rs#1596
[tokio-rs#1597]: tokio-rs#1597
[tokio-rs#1600]: tokio-rs#1600
[tokio-rs#1601]: tokio-rs#1601
[tokio-rs#1602]: tokio-rs#1602
[tokio-rs#1605]: tokio-rs#1605
[tokio-rs#1614]: tokio-rs#1614
[tokio-rs#1616]: tokio-rs#1616
[tokio-rs#1617]: tokio-rs#1617
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