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: support adding arbitrary fields to the instrument macro #596

Merged
merged 3 commits into from
Feb 24, 2020

Conversation

Kobzol
Copy link
Contributor

@Kobzol Kobzol commented Feb 21, 2020

This PR adds support for adding arbitrary key/value pairs to be used as fields to tracing::instrument.

Current syntax:

#[instrument(fields(key = "value", v = 1, b = true, empty))]
  • Empty keys are supported
  • If a key is not a single identifier, it's value is not a string/int/bool (or missing), is repeated or shares a name with a parameter, an error is reported

Remarks:

  • I noticed that the Value trait is implemented only for integers, booleans, strings and Debug/Empty values. I wonder why floating point numbers are not supported?
  • The testing infrastructure (MockSpan etc.) is awesome!
  • Field keys in spans can be nested (for example parent.child = 1). We might achieve something similar here by transforming a path (parent::child) into a nested key (parent.child). However, I think that it would quite complicate the implementation (it's already pretty convoluted). And I'm not sure if we could quote! these transformed names properly, I had problems with quoting strings and had to use Ident for the keys (but I didn't spend too much time on this).

Fixes: #573

@Kobzol Kobzol requested a review from hawkw February 21, 2020 23:37
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.

This is great, thanks so much! I commented on a couple of minor nits, but it looks really good overall and I'll be very happy to merge it when the nits are addressed.

* Field keys in spans can be nested (for example `parent.child = 1`). We might achieve something similar here by transforming a path (`parent::child`) into a nested key (`parent.child`).

Ideally, I think it would be nice to be able to support nested field names here as well, and I'd really prefer to use the same syntax we do in the function-like macros (read: dotted names rather than paths). However, AFAICT, the only way to do this would be to stop parsing the macro input as NestedMeta, and write our own parser from scratch using syn's primitives...this might be a lot of work. So, for now, I'm happy to merge this without support for nested field names, as long as we open an issue to track adding that eventually with some details.

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 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
@hawkw
Copy link
Member

hawkw commented Feb 21, 2020

Also: looks like CI is complaining about unnecessary parentheses; should be pretty easy to fix.

@Kobzol
Copy link
Contributor Author

Kobzol commented Feb 23, 2020

I changed the error messages and fixed clippy lint. After merge I'll add check for #562 in a new PR.

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.

Okay, this looks good, modulo one missing piece I forgot to mention earlier: can you add a brief note in the docs documenting the fields argument and how to use it (including the requirements that values be string, bool, or integer literals, and that dotted field names aren't currently supported)? An example or two in the docs would be great too, if you have the time.

Thanks!

@Kobzol
Copy link
Contributor Author

Kobzol commented Feb 24, 2020

Added docs and examples with basic usage. Let me know if that's good enough :-)

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.

this looks great; i had a couple last suggestions that you're free to disregard. i'll happily merge this when you feel like it's ready!

tracing-attributes/src/lib.rs Outdated Show resolved Hide resolved
tracing-attributes/src/lib.rs Outdated Show resolved Hide resolved
@Kobzol
Copy link
Contributor Author

Kobzol commented Feb 24, 2020

I hope that I understood your comments right :) Reformatted the documentation.

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.

Looks great! Thanks again!

@hawkw hawkw merged commit a6a2434 into tokio-rs:master Feb 24, 2020
@Kobzol Kobzol deleted the instrument-fields branch February 24, 2020 22:40
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.

Allow extra fields for span created by the #[instrument] macro
2 participants