trace: Improve documentation #1148

hawkw · 2019-06-13T22:25:24Z

Motivation

Currently, some parts of the tokio-trace documentation is unclear, and
some concepts are not described adequately. In addition, the
organization of the docs can be improved and made less redundant. For
example, the lib-level documentation has examples of the shorthand
syntax for fields that is repeated in the span macro's documentation.
This could be changed so that the lib and module-level documentation
provides an overview and the individual items go into greater detail.

Solution

This branch reorganizes the documentation and adds better explanations
of some concepts that were previously unclear. In particular, I've
added:

examples of setting metadata and overriding span parents in the
macros
an explanation of what targets are,
added an explanation of the span follows-from relationship, and how
it differs from parent/child relationships

I've also fixed up some typos and broken links.

Fixes #1063
Fixes #1064
Fixes #1065
Fixes #985
Fixes #907

hawkw · 2019-06-13T22:26:07Z

@jonhoo and @LucioFranco, since a lot of the additions here are from improvements you suggested, I'd love to get your feedback on this when you have the chance.

tokio-trace/src/span.rs

jonhoo · 2019-06-14T12:32:23Z

tokio-trace/src/lib.rs

@@ -27,9 +27,9 @@
 //! The core of `tokio-trace`'s API is composed of `Event`s, `Span`s, and
 //! `Subscriber`s. We'll cover these in turn.


nit: maybe make the order here the same as the order they're explained in below?

jonhoo · 2019-06-14T12:34:27Z

tokio-trace/src/lib.rs

@@ -49,153 +49,7 @@
 //! # }


I think if we're not going to give any more stuff here on spans, it'd be good to make this example a little more expansive. I would also avoid using the word "context" here, because as far as I've gathered, it's entirely up to the subscriber whether or not work done below .enter() is considered "in the context of" the span. The span only serves to build a pair of start and end events, and does not say anything about events "contained" within that.

I think if we're not going to give any more stuff here on spans, it'd be good to make this example a little more expansive

I thought the docs would be more readable if more of the content in the lib-level docs was pulled out into module-level docs, so the reader isn't immediately presented with a massive wall of text. The hope was that the lib docs would serve as an overview/table of contents, and most of the actual documentation would live in the appropriate module. I'd love to know if you think this is actually a good way to structure things, or if it's not working for you?

That said, I can definitely add a more complete example here. I wasn't sure about including an example that shows a lot of concepts that haven't been introduced yet in the documentation, though?

I would also avoid using the word "context" here, because as far as I've gathered, it's entirely up to the subscriber whether or not work done below .enter() is considered "in the context of" the span. The span only serves to build a pair of start and end events, and does not say anything about events "contained" within that.

I think there is an expectation that there's some concept of "context", but the policy of what a "context" means is up to the subscriber. I'll try and edit these docs to make it clearer that this is provided by the subscriber implementation.

Oh, sorry, let me rephrase: I think it's great to move things out of the top-level docs the way you have. I meant more that given that you're going to do that, the examples that are left behind should probably be slightly more elaborate than the examples that were there before. I don't think it's too problematic to have more complex concepts in the example here, since the user will know that to get a detailed explanation they should go to the module-level docs.

jonhoo · 2019-06-14T12:35:29Z

tokio-trace/src/lib.rs

-//! create a single span around the entire loop; whereas if we wanted to know how
-//! much time was spent in each individual iteration, we would enter a new span
-//! on every iteration.
+//! The [`span` module]'s documentation provides further details on how to use spans.
 //!
 //! ## Events


This section reinforces the belief that a span establishes a "context", which again does not match my understand of what Spans actually mean?

Further down, this text also says:

Events are represented as a special case of spans — they are created, they may have fields added, and then they close immediately, without being entered.

Is that true? I thought events weren't just "immediate spans"?

Is that true? I thought events weren't just "immediate spans"?

That sentence is no longer correct, I think it was written when this was the case and then it was never removed. I must've missed that while working on this branch, since I was mostly just looking at the span docs. Thanks for catching that!

jonhoo · 2019-06-14T12:38:31Z

Reading through it now!

As a note for a possibly later PR, I think the "In libraries" is one of the most important and difficult sections, and it is currently basically empty.

tokio-trace/src/macros.rs

jonhoo · 2019-06-14T12:42:01Z

tokio-trace/src/macros.rs

+/// ```
+///
+/// By default, the module path to the current Rust module will be used
+/// as the target, and the parent will be [determined contextually].


I don't think that what this links to is useful. Is "contextual spans" explained anywhere else (like in the Span docs)?

tokio-trace/src/macros.rs

jonhoo · 2019-06-14T12:48:28Z

tokio-trace/src/span.rs

@@ -1,6 +1,50 @@
-//! Spans represent periods of time in the execution of a program.
+//! Spans represent periods of time in which a program was executing in a
+//! particular context.


I thought a span explicitly did not establish context, and was only a period of time? Anything else is up to the subscriber, no?

jonhoo · 2019-06-14T12:49:02Z

tokio-trace/src/span.rs

 //!
-//! # Entering a Span
+//! A span consists of [fields], user-defined key-value pairs of arbitrary data
+//! that describe the context the span represents, and [metadata], a fixed set


Other docs refer to these just as attributes. Should we use "metadata" everywhere, or "attributes" everywhere?

Actually, aren't the two distinct? Is there a difference between Attributes and Metadata? What is it? Which do you mean here?

Is Metadata attributes and fields maybe?

tokio-trace/src/span.rs

jonhoo · 2019-06-14T12:52:05Z

tokio-trace/src/span.rs

+//!
+//! The [`Attributes`] type contains data associated with a span, and is
+//! provided to the [`Subscriber`] when a new span is created. It contains
+//! the span's metadata, the ID of the span's parent if one was explicitly set,


I think somewhere needs to talk about the relationship between spans and how it is or is not determined. When reading this sentence, I had several questions:

What if it isn't set?

What happens if I set it to something "wrong"?

What are the implications of setting one span as another's "parent"?

jonhoo · 2019-06-14T12:53:17Z

tokio-trace/src/span.rs

+//! the span's metadata, the ID of the span's parent if one was explicitly set,
+//! and any fields whose values were recorded when the span was constructed.
+//! The subscriber may then choose to cache the data for future use, record
+//! it in some manner, or discard it completely.


This last sentence is pretty vague given that I (currently) have no idea what a subscriber is if I have followed the docs thus far. I think elaborating a bit here would be worthwhile.

jonhoo · 2019-06-14T12:53:43Z

tokio-trace/src/span.rs

+//!
+//! # The Span Lifecycle
+//!
+//! ## Entering a Span
 //!
 //! A thread of execution is said to _enter_ a span when it begins executing,
 //! and _exit_ the span when it switches to another context. Spans may be


What does "switches to another context" mean?

jonhoo · 2019-06-14T12:54:13Z

tokio-trace/src/span.rs

+//!
+//! # The Span Lifecycle
+//!
+//! ## Entering a Span


I think this is missing an explanation of what "entering a span" even means in the first place.

jonhoo · 2019-06-14T12:55:43Z

tokio-trace/src/span.rs

+//!
+//! Spans form a tree structure — unless it is a root span, all spans have a
+//! _parent_, and may have one or more _children_. When a new span is created,
+//! the current span becomes the new span's parent. The total execution time of


This answers some of the questions from above, but raises a few more:

How is the "current" span determined?

What if I have entered multiple spans?

Is this actually guaranteed, or dependent on the subscriber?

jonhoo · 2019-06-14T12:57:16Z

tokio-trace/src/span.rs

+//! the current span becomes the new span's parent. The total execution time of
+//! a span consists of the time spent in that span and in the entire subtree
+//! represented by its children. Thus, a parent span always lasts for at least
+//! as long as the longest-executing span in its subtree.


Is that true? That seems weird. What if I have the following:

let a = trace_span!("a"); let b = trace_span!("b"); let ag = a.enter(); let bg = b.enter(); drop(ag); // what happens to events here? are they in a or not? is this behavior guaranteed? drop(bg);

jonhoo · 2019-06-14T12:58:08Z

tokio-trace/src/span.rs

+//! A child span should typically be considered _part_ of its parent. For
+//! example, if a subscriber is recording the length of time spent in various
+//! spans, it should generally include the time spent in a span's children as
+//! part of that span's duration.


Does that mean that time in children is double-counted? Once in the parent and once in the child?

tokio-trace/src/span.rs

jonhoo · 2019-06-14T13:03:11Z

tokio-trace/tokio-trace-core/src/metadata.rs

@@ -7,8 +7,21 @@ use std::fmt;

 /// Metadata describing a [span] or [event].


The [event] link here is broken.

Sorry, it is broken in tokio-trace::Metadata, not in tokio-trace-core::Metadata

Related to that, is there any way for the two to be the same? Why are they distinct types with different docs (the docs for tokio-trace::Metadata haven't been updated to reflect the change below for example.

They're the same type; tokio-trace::Metadata is a re-export of tokio-trace-core::Metadata. I think the issue is that tokio-trace depends on the crates.io version of tokio-trace-core, rather than the local version, so the changes I've made to the -core docs since the last crates.io release aren't reflected in the tokio-trace docs.

I'll publish a tokio-trace-core point release before publishing tokio-trace 0.2, so I think this should be resolved. In the meantime, you can patch tokio-trace's dependency on tokio-trace-core to see the docs as they should be.

Ohh, I see. No worries, I just generated docs for tokio-trace-core explicitly. I think the [event] link is still broken though.

I'll double check that one!

jonhoo · 2019-06-14T13:05:23Z

The link to "metadata" from Span::metadata is broken (the one from Event::metadata is not).

jonhoo · 2019-06-14T13:09:19Z

Changes look good overall! I left some notes :)

LucioFranco · 2019-06-14T14:37:42Z

I took a quick look through it looks great! Unfortunetly wont have time to review this but thank you for all the work @hawkw!

hawkw · 2019-06-14T18:18:58Z

@jonhoo thanks so much for the extremely thorough review, I really appreciate it!

Fixes #1065 Signed-off-by: Eliza Weisman <eliza@buoyant.io>

Fixes #1064 Signed-off-by: Eliza Weisman <eliza@buoyant.io>

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

Fixes #985 Signed-off-by: Eliza Weisman <eliza@buoyant.io>

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

Co-Authored-By: Jon Gjengset <jon@thesquareplanet.com>

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

hawkw · 2019-06-24T22:07:50Z

@jonhoo in order to prepare for the repo move to tokio-rs/tracing, I'd like to merge these changes as-is (since I think they're better than the previous docs) and make some of your suggestions in a follow-up branch after the move. WDYT?

davidbarsky · 2019-06-25T00:20:34Z

Opened tokio-rs/tracing#93 to track applying the rest of @jonhoo's suggestions.

hawkw added T-docs Topic: documentation tokio-trace labels Jun 13, 2019

hawkw added this to the tokio-trace v0.2 (breaking changes) milestone Jun 13, 2019

hawkw requested review from jonhoo and LucioFranco June 13, 2019 22:25

hawkw self-assigned this Jun 13, 2019

davidbarsky reviewed Jun 13, 2019

View reviewed changes

tokio-trace/src/span.rs Show resolved Hide resolved