This repository has been archived by the owner on Mar 23, 2021. It is now read-only.
docs: add a step-by-step example for top-level lib documentation #12
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This shouldn't be the place where documentation stops being written, but it might be a good start. adapting [the `basic` example][basic-example] to have local imports (to make the source of symbols abundantly clear) and changing the code's style to be as incremental as possible. [basic-example]: https://github.com/vivint-smarthome/opentelemetry-stackdriver/blob/102ab6242a891798a8316db5f7bfca463c818507/examples/basic.rs
ErichDonGubler
force-pushed
the
example-docs
branch
from
5489c16 to
f2289a4
Compare
djc
reviewed
Nov 7, 2020
Fix a typo where I missed the "l" in "leveraging". Resolves https://github.com/vivint-smarthome/opentelemetry-stackdriver/pull/12/files/f2289a4b51852e20e13f5bdfdc35ed925f045e4d#r519210796.
Fixes some incomplete commentary for the `spawn` variable. Resolves https://github.com/vivint-smarthome/opentelemetry-stackdriver/pull/12/files/f2289a4b51852e20e13f5bdfdc35ed925f045e4d#r519210345.
Move args for `StackDriverExporter::connect` to an order that matches their usage.
Resolves the issue pointed out in https://github.com/vivint-smarthome/opentelemetry-stackdriver/pull/12/files/f2289a4b51852e20e13f5bdfdc35ed925f045e4d#r519210340. It doesn't solve the bigger question underlying the problem noted, but...
Fix up remaining issues that would have been caught if doctests were actually getting compiled right now. :<
New commit message: This shouldn't be the place where documentation stops being written (which is being discussed in vivint-smarthome#7), but it might be a good start. This is very similar to code you can already find in this repo's [`examples/basic.rs`][basic-example] and documentation in other places, like [the doc example for `tracing_opentelemetry::OpenTelemetryLayer::new`][open_telemetry_layer-new]. The focus of these docs, though, is to demonstrate the usage of the API in an incremental style, so that single steps are as easy as possible to understand. [basic-example]: https://github.com/vivint-smarthome/opentelemetry-stackdriver/blob/102ab6242a891798a8316db5f7bfca463c818507/examples/basic.rs [open_telemetry_layer-new]: https://docs.rs/tracing-opentelemetry/0.8.0/tracing_opentelemetry/struct.OpenTelemetryLayer.html#examples
djc
reviewed
Nov 8, 2020
| //! | ||
| //! // An implementation of `futures::task::Spawn`, which allows our `StackDriverExporter` to | ||
| //! // run new tasks in an `async` runtime, as part of its work. | ||
| //! let spawn = { |
There was a problem hiding this comment.
So I think opentelemetry-stackdriver actually includes this under a feature flag. Maybe you should mention that and just use it here, because it's pretty much a distraction from the purpose of this example?
djc
reviewed
Nov 8, 2020
| //! &spawn, | ||
| //! maximum_shutdown_duration, | ||
| //! num_concurrent_requests, | ||
| //! ).await.unwrap(); |
There was a problem hiding this comment.
It would be useful/interesting to point out why this is async in the first place. And also maybe mention explicitly (even if it's alluded to in the part about the Spawner) that this spawns a background task that's actually responsible for schlepping spans off to Stackdriver.
djc
reviewed
Nov 8, 2020
| //! // A `tracing_subscriber::Registry` is a local backing store for span data -- it | ||
| //! let registry = tracing_subscriber::Registry::default(); | ||
| //! | ||
| //! let subscriber = { |
There was a problem hiding this comment.
I find the extra scope introduction distracting here, maybe skip that?
|
More remarks -- I'm a bit of a perfectionist, sorry, but this would definitely make for a large improvement in the overall documentation situation for this crate! |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This shouldn't be the place where documentation stops being written (which is being discussed in #7), but it might be a good start. This is very similar to code you can already find in this repo's
examples/basic.rsand documentation in other places, like the doc example fortracing_opentelemetry::OpenTelemetryLayer::new. The focus of these docs, though, is to demonstrate the usage of the API in an incremental style, so that single steps are as easy as possible to understand.Things to do before merging:
This PR currently uses default
rustfmtstyle, which is not the same as the rest of this project's style configuration. Problem?--autosquashfixup!andsquash!commits.This currently won't be doctested because of this:
opentelemetry-stackdriver/src/lib.rs
Lines 17 to 21 in 38c1e3e
How problematic is this right now?