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

docs: add example for using instrument macro with pinned future #3045

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

ziutech
Copy link

@ziutech ziutech commented Jul 31, 2024

This change is motivated by a stackoverflow question.

@ziutech ziutech requested review from hawkw, davidbarsky and a team as code owners July 31, 2024 21:43
@ziutech ziutech force-pushed the docs-instrument-pinned-future branch from 735e405 to 3a2bbb6 Compare July 31, 2024 21:52
@ziutech ziutech force-pushed the docs-instrument-pinned-future branch from 3a2bbb6 to 3cc1b7f Compare July 31, 2024 21:52
tracing-attributes/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@kaffarell kaffarell left a comment

Choose a reason for hiding this comment

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

Just two small nits:

  • I think we can hide the use tracing::Instrument import as well.
  • Instead of referring to std::pin, maybe std::pin::Pin (the struct) would be better.

@ziutech
Copy link
Author

ziutech commented Aug 29, 2024

I think we can hide the use tracing::Instrument import as well.

If you think it's not important I can remove it, but in my opinion it would be better to mention where in_current_span comes from, because this would be the only place the trait Instrument is mentioned on this doc page, and it will be easier to read for someone who doesn't know this trait exists.

Of course if the person reading copy-pastes/duplicates thit example into their code they will get an error to include the trait, so for this case it probably does not matter.

Instead of referring to std::pin, maybe std::pin::Pin (the struct) would be better.

My initial choice for the pin module was that it's "more thorough" (as the docs for std::pin::Pin say), but maybe that does not mean it's better.

Tell me what you think @kaffarell, and I'll change it to whatever you think is better.

@kaffarell
Copy link
Contributor

If you think it's not important I can remove it, but in my opinion it would be better to mention where in_current_span comes from, because this would be the only place the trait Instrument is mentioned on this doc page, and it will be easier to read for someone who doesn't know this trait exists.

Of course if the person reading copy-pastes/duplicates thit example into their code they will get an error to include the trait, so for this case it probably does not matter.

No hard feelings here, so I'm also good with leaving it.

My initial choice for the pin module was that it's "more thorough" (as the docs for std::pin::Pin say), but maybe that does not mean it's better.

Actually std::pin only contains the std::pin::Pin struct (and a macro) and the description is more complete. So I am fine with leaving this as it is as well!

LGTM.

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