-
Notifications
You must be signed in to change notification settings - Fork 739
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
Improve README for first time users #825
Conversation
Start with the simple way of setting up a subscriber rather than diving into the complex ways. Show the preferred method of instrumenting async code before explaining why it is more complex and how it's implemented. Remove yak credit. Does anyone care?
cc @davidbarsky and @yaahc, who almost certainly have docs opinions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on the docs, it's much appreciated! I had a few thoughts that I commented on inline.
A lot of the same examples are reproduced in the tracing
crate's README (https://github.com/tokio-rs/tracing/tree/master/tracing#usage), since that's what's rendered on the crate's crates.io page. Once we finalize the changes to the repository README, we ought to try to ensure the relevant changes are reflected there as well.
``` | ||
|
||
This requires the [`tracing-futures`] crate to be specified as a dependency. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And then maybe put the example with the instrument
combinator here as well, before discussing the reasons for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was an awkward join between the examples still. I've reworked the language here but left the motivation in the middle, as it helps understand the combinator (though you can arrive here without really having taken in what a span is yet).
README.md
Outdated
Note: Libraries should *NOT* call `set_global_default()`, as this will cause | ||
conflicts when executables try to set the default later. | ||
Note: Libraries should *NOT* install a subscriber by using a method than calls | ||
`set_global_default()`, as this will cause conflicts when executables try to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should at least discuss the existence of set_global_default
and what it does before this warning. If the first example is changed to show tracing_subscriber::init
, then users will read this note before even knowing that there is a function called set_global_default
.
Perhaps another example after the first one, explaining that tracing_subscriber::init()
uses set_global_default
, the generalized way of setting a global default subscriber?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some better joining language between the examples to address this. I think one simple example and one complex example is enough?
I think this is a great idea. I don't have any comments on the particular changes being made but I think I'll follow the example set here and make the same changes to My one comment is that I think should be applied to more than just |
The new version of tokio.rs is going live pretty soon, and will have a Also, I think the |
Address @hawkw review comments. Opted for clearer wording in passages joining examples together rather than extra reordering for now.
@davidbarsky have you had a chance to give this a skim yet? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was hoping to get @davidbarsky to give this a skim, but I think it's fine to merge as-is. We can always make more docs improvements in a follow-up.
I haven't had the time; feel free to merge. |
Motivation
The documentation nearly put me off trying to use
tracing
at all. It looks hard to set up and use, and the two problems I first ran into (needing to output to stderr, and instrumenting a function with a non-formattable arg) were not easy to find the right solutions for.Solution
This change rearranges the README a little to emphasise solutions, rather than problems and extra details a first time user probably doesn't need to care about yet.