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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs: document span.in_scope() at top-level #1344

Merged
merged 2 commits into from Apr 30, 2021

Conversation

Fishrock123
Copy link
Contributor

span.in_scope() had a link def in the main tracing docs which was unused, this function is quite handy to know about and I almost re-implemented something similar to it.

Motivation

I almost reimplemented this as a macro. 馃槄

Solution

Document it at top-level!

@Fishrock123 Fishrock123 requested review from davidbarsky, hawkw and a team as code owners April 1, 2021 17:59
@Fishrock123 Fishrock123 force-pushed the top-level-docsrs-span.in_scope branch from e5b7f2f to 3ada5de Compare April 1, 2021 18:05
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.

A few docs suggestions. The most important one is that I think we need to fix a few things in the example so that it actually compiles.

tracing/src/lib.rs Outdated Show resolved Hide resolved
tracing/src/lib.rs Outdated Show resolved Hide resolved
tracing/src/lib.rs Show resolved Hide resolved
//! [`in_scope()` function][`in_scope`] which can be used to easily wrap synchonous
//! code in a span.
//!
//! For example:
Copy link
Member

Choose a reason for hiding this comment

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

take it or leave it: maybe we ought to also have an example showing that in_scope can be called multiple times on the same span, like

let span = tracing::info_span!(...);

let something = span.in_scope(|| some_function());
let other_thing = span.in_scope(|| other_function());

or whatever?

On the other hand, maybe this is better left in the function-level docs for in_scope?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should leave that in the in_scope() docs. Even just mentioning it here would probably be enough, but I think the serde example will be quite nice.

@hawkw
Copy link
Member

hawkw commented Apr 9, 2021

@Fishrock123 have you had a chance to look at my suggested changes yet? It would definitely be nice to get this merged.

@Fishrock123 Fishrock123 force-pushed the top-level-docsrs-span.in_scope branch from 811bb0d to 164d65f Compare April 15, 2021 23:54
@Fishrock123
Copy link
Contributor Author

Fishrock123 commented Apr 15, 2021

Sorry this took so long - originally I had tests pass locally so I didn't think anything of it. Maybe I ran them incorrectly. Thanks for the detailed suggestions, I hadn't thought of fully mocking out the serde function in a hidden way.

@Fishrock123 Fishrock123 force-pushed the top-level-docsrs-span.in_scope branch 2 times, most recently from 5ff480f to 33049a0 Compare April 16, 2021 00:10
@Fishrock123
Copy link
Contributor Author

Ok I'm not sure why the test still fails with no-std, I put a config there to exclude it under no-std...

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.

Ok I'm not sure why the test still fails with no-std, I put a config there to exclude it under no-std...

I don't actually think the issue is related to the std feature flag. Instead, it looks like the example doesn't compile because the info_span! macro is never imported?

//!
//! For example:
//! ```rust
//! # #[cfg(not(feature = "no-std"))]
Copy link
Member

Choose a reason for hiding this comment

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

tracing doesn't have a feature called no-std; the standard library is disabled by disabling the std feature. So, I would be

//! # #[cfg(not(feature = "std"))]

but, I don't actually think that will work. Feature flags are not passed to the compiler when building doctests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

aaah sorry It was late in the day and I read the Cargo.toml wrong, lol, no-std is a category here not a feature...

tracing/src/lib.rs Show resolved Hide resolved
@Fishrock123 Fishrock123 force-pushed the top-level-docsrs-span.in_scope branch from 9c6c718 to f6f5807 Compare April 19, 2021 16:42
`span.in_scope()` had a link def in the main tracing docs which was unused,
this function is quite handy to know about and I almost re-implemented something similar to it.
@Fishrock123 Fishrock123 force-pushed the top-level-docsrs-span.in_scope branch from f6f5807 to 0f93f0e Compare April 19, 2021 16:43
tracing/src/lib.rs Outdated Show resolved Hide resolved
@Fishrock123 Fishrock123 changed the title docs: document span.in_scope() at to-level docs: document span.in_scope() at top-level Apr 19, 2021
@hawkw hawkw merged commit b4569fb into tokio-rs:master Apr 30, 2021
hawkw added a commit that referenced this pull request Apr 30, 2021
## Motivation

`span.in_scope()` had a link def in the main tracing docs which was
unused, this function is quite handy to know about and I almost
re-implemented something similar to it.

I almost reimplemented this as a macro.


## Solution

Document it at top-level!

Co-authored-by: Eliza Weisman <eliza@buoyant.io>
@hawkw hawkw mentioned this pull request Apr 30, 2021
hawkw added a commit that referenced this pull request Apr 30, 2021
## Motivation

`span.in_scope()` had a link def in the main tracing docs which was
unused, this function is quite handy to know about and I almost
re-implemented something similar to it.

I almost reimplemented this as a macro.


## Solution

Document it at top-level!

Co-authored-by: Eliza Weisman <eliza@buoyant.io>
hawkw added a commit that referenced this pull request Apr 30, 2021
# 0.1.26 (April 30, 2021)

### Fixed

- **attributes**: Compatibility between `#[instrument]` and `async-trait`
  v0.1.43 and newer ([#1228])
- Several documentation fixes ([#1305], [#1344])
### Added

- `Subscriber` impl for `Box<dyn Subscriber + Send + Sync + 'static>`
  ([#1358])
- `Subscriber` impl for `Arc<dyn Subscriber + Send + Sync + 'static>`
  ([#1374])
- Symmetric `From` impls for existing `Into` impls on `span::Current`,
  `Span`, and `Option<Id>` ([#1335], [#1338])
- `From<EnteredSpan>` implementation for `Option<Id>`, allowing
  `EnteredSpan` to be used in a `span!` macro's `parent:` field ([#1325])
- `Attributes::fields` accessor that returns the set of fields defined
  on a span's `Attributes` ([#1331])

Thanks to @Folyd, @nightmared, and new contributors @rmsc and @Fishrock123 for
contributing to this release!

[#1227]: #1228
[#1305]: #1305
[#1325]: #1325
[#1338]: #1338
[#1344]: #1344
[#1358]: #1358
[#1374]: #1374
[#1335]: #1335
[#1331]: #1331
hawkw added a commit that referenced this pull request Apr 30, 2021
# 0.1.26 (April 30, 2021)

### Fixed

- **attributes**: Compatibility between `#[instrument]` and `async-trait`
  v0.1.43 and newer ([#1228])
- Several documentation fixes ([#1305], [#1344])
### Added

- `Subscriber` impl for `Box<dyn Subscriber + Send + Sync + 'static>`
  ([#1358])
- `Subscriber` impl for `Arc<dyn Subscriber + Send + Sync + 'static>`
  ([#1374])
- Symmetric `From` impls for existing `Into` impls on `span::Current`,
  `Span`, and `Option<Id>` ([#1335], [#1338])
- `From<EnteredSpan>` implementation for `Option<Id>`, allowing
  `EnteredSpan` to be used in a `span!` macro's `parent:` field ([#1325])
- `Attributes::fields` accessor that returns the set of fields defined
  on a span's `Attributes` ([#1331])

Thanks to @Folyd, @nightmared, and new contributors @rmsc and @Fishrock123 for
contributing to this release!

[#1227]: #1228
[#1305]: #1305
[#1325]: #1325
[#1338]: #1338
[#1344]: #1344
[#1358]: #1358
[#1374]: #1374
[#1335]: #1335
[#1331]: #1331
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.

None yet

2 participants