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

tracing: add record_all! macro for recording multiple values in one call #3227

Merged

Conversation

matildasmeds
Copy link
Contributor

Motivation

Currently, Span.record_all() is part of the public API and accepts ValueSet as a parameter.
However, constructing a ValueSet is both verbose and undocumented, making it not so practical.

Solution

To make recording multiple values easier, we introduce a new macro: record_all!, which wraps the Span.record_all() function. As we don't intend anyone to call Span.record_all() directly, we hide it from the documentation. We reference the new macro from Span.record() doc comment instead.

The new record_all! macro supports optional formatting sigils % and ?, ensuring a consistent DevEx with the other value-recording macros.

@matildasmeds matildasmeds requested review from hawkw, davidbarsky and a team as code owners March 11, 2025 19:02
Copy link
Contributor

@hds hds left a comment

Choose a reason for hiding this comment

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

Looks good. Thank you very much!

Just one small change

matildasmeds and others added 4 commits March 25, 2025 07:07
Currently, Span.record_all() is part of the public API and accepts
ValueSet as a parameter. However, constructing a ValueSet is both
verbose and undocumented, making it not so practical.

To make recording multiple values easier, we introduce a new macro:
record_all!, which wraps the Span.record_all() function.
As we don't intend anyone to call Span.record_all() directly, we hide
it from the documentation. We reference the new macro from Span.record()
doc comment instead.

The new record_all! macro supports optional formatting sigils % and ?,
ensuring a consistent DevEx with the other value-recording macros.
Co-authored-by: Hayden Stainsby <[email protected]>
When developing the test, I realized that the previous iteration would
not support plain primitive values, such as integers or booleans. Given
that span! macro supports that, I thought it to be reasonable for record_all!
to do so as well. This version does. It is also simpler than the original.

Looking forward to feedback!
@matildasmeds matildasmeds force-pushed the matildasmeds/wrap-record-all-call-in-macro branch from 656fc70 to f80475b Compare March 25, 2025 06:14
@matildasmeds
Copy link
Contributor Author

Rebased against master, to include the lint fixes from #3202.

Copy link
Contributor

@hds hds left a comment

Choose a reason for hiding this comment

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

Couple of small comments.

Copy link
Contributor

@hds hds left a comment

Choose a reason for hiding this comment

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

Fantastic, thanks for all your work!

@hds hds changed the title Add record_all! macro for recording multiple values in one call tracing: add record_all! macro for recording multiple values in one call Mar 27, 2025
@hds hds merged commit d6505ca into tokio-rs:master Mar 27, 2025
55 checks passed
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.

2 participants