core: impl field::Value for Option<T> by bryanburgers · Pull Request #1585 · tokio-rs/tracing

bryanburgers · 2021-09-21T21:07:08Z

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 · 2021-09-21T21:07:57Z

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

hawkw · 2021-09-21T22:03:06Z

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

bryanburgers · 2021-09-21T22:13:48Z

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 · 2021-09-22T00:42:53Z

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 · 2021-09-22T02:47:27Z

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

Add tests for making an `Option<T> where T: Value` a value in itself.

bryanburgers · 2021-09-22T12:15:54Z

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.

hawkw

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?

tracing/tracing/src/lib.rs

Line 242 in e455367

//! ### Recording Fields

Also, we'll want to add support for Option values in the instrument attribute, somewhere around here:

tracing/tracing-attributes/src/lib.rs

Lines 1091 to 1142 in e455367

    
               /// 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

This adds tests for references to options, and adds `.only()` to the expected fields in the tests.

hawkw

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

hahaha lol never edit code in the github webapp suggestions UI. i have some regrets.

tracing/tests/event.rs

@BrianBurgers

# 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

@BrianBurgers

# 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 · 2021-10-03T20:33:07Z

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

@BrianBurgers

# 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

@BrianBurgers

# 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

@BrianBurgers

# 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

## 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>

@BrianBurgers

# 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

@BrianBurgers

# 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

bryanburgers requested review from a team, carllerche, davidbarsky and hawkw as code owners September 21, 2021 21:07

bryanburgers force-pushed the option-value branch from 67c1246 to 001b384 Compare September 21, 2021 22:00

DCjanus and others added 2 commits September 22, 2021 07:03

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

7e59a00

Add tests for impl Value for Option<V: Value>

d8e87d0

Add tests for making an `Option<T> where T: Value` a value in itself.

bryanburgers force-pushed the option-value branch from 001b384 to d8e87d0 Compare September 22, 2021 12:13

bryanburgers requested review from jtescher and yaahc as code owners September 22, 2021 12:13

bryanburgers changed the base branch from master to v0.1.x September 22, 2021 12:14

Merge branch 'v0.1.x' into option-value

d56a600

hawkw requested changes Sep 24, 2021

View reviewed changes

tracing/tests/event.rs Outdated Show resolved Hide resolved

tracing/tests/event.rs Show resolved Hide resolved

Apply test suggestions from code review

049e3de

This adds tests for references to options, and adds `.only()` to the expected fields in the tests.

hawkw approved these changes Sep 24, 2021

View reviewed changes

hawkw reviewed Sep 24, 2021

View reviewed changes

tracing/tests/event.rs Outdated Show resolved Hide resolved

fix my stupid typo

f6d159d

hahaha lol never edit code in the github webapp suggestions UI. i have some regrets.

hawkw reviewed Sep 24, 2021

View reviewed changes

tracing/tests/event.rs Outdated Show resolved Hide resolved

hawkw added 2 commits September 24, 2021 12:07

haha whoops i broke rustfmt lol

600e850

gahh accidentally removed too much whitespace ;__;

718d4c5

hawkw merged commit f250f6f into tokio-rs:v0.1.x Sep 24, 2021

This was referenced Sep 24, 2021

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

Closed

core: consider Value implementation for Option<T> where T: Value #824

Closed

Value implementation for `Option<T> where T: Value #878

Closed

hawkw mentioned this pull request Oct 1, 2021

core: prepare to release 0.1.21 #1610

Merged

CAD97 mentioned this pull request Oct 3, 2021

tracing::instrument args by autoref specialization #1612

Open

hawkw mentioned this pull request Oct 5, 2021

tracing: prepare to release v0.1.29 #1623

Merged

davidbarsky mentioned this pull request Sep 26, 2023

chore: backport roughly a year's worth of changes #2728

Merged

AmoleR mentioned this pull request Oct 10, 2023

Value is not Implemented for Option<T> #2745

Closed

AmoleR added a commit to Random-Math/tracing that referenced this pull request Oct 10, 2023

impl<T: Value> Value for Option<T> from tokio-rs#1585

17eb586

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

core: impl field::Value for Option<T>#1585
hawkw merged 7 commits intotokio-rs:v0.1.xfrom
bryanburgers:option-value

bryanburgers commented Sep 21, 2021

Uh oh!

bryanburgers commented Sep 21, 2021

Uh oh!

hawkw commented Sep 21, 2021

Uh oh!

bryanburgers commented Sep 21, 2021

Uh oh!

bryanburgers commented Sep 22, 2021

Uh oh!

CAD97 commented Sep 22, 2021 •

edited

Loading

Uh oh!

bryanburgers commented Sep 22, 2021

Uh oh!

hawkw left a comment

Uh oh!

Uh oh!

Uh oh!

hawkw left a comment

Uh oh!

Uh oh!

Uh oh!

CAD97 commented Oct 3, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	/// 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,
	}
	}

Conversation

bryanburgers commented Sep 21, 2021

Uh oh!

bryanburgers commented Sep 21, 2021

Uh oh!

hawkw commented Sep 21, 2021

Uh oh!

bryanburgers commented Sep 21, 2021

Uh oh!

bryanburgers commented Sep 22, 2021

Uh oh!

CAD97 commented Sep 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bryanburgers commented Sep 22, 2021

Uh oh!

hawkw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hawkw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

CAD97 commented Oct 3, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

CAD97 commented Sep 22, 2021 •

edited

Loading