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

ref(AM): Make theory explanations clearer, merge in glossary #1667

Merged
merged 12 commits into from
May 13, 2020

Conversation

lobsterkatie
Copy link
Member

@lobsterkatie lobsterkatie commented May 7, 2020

Disclaimer: The Setting Up Tracing section (everything SDK-specific) is slated for work in the next phase of this project, and has for the most part not been touched here. So in terms of feedback, I'm primarily looking for folks to read and comment on everything before that. If the Setting Up section has any glaring inconsistencies with the top half (the half this PR is about), please do point them out, but know that most of that section is going to get similarly reworked next week.


This PR has three major aims:

  • Rewrite and expand the first portion of the doc (everything which isn't about implementing AM in a specific language) to make it clearer for users who may or may not be familiar with other tracing tools.

  • Take the information which has previously lived in the glossary and add it into the main document, so that information isn't missed as a result of people being lazy and not wanting to click through to a separate page.

And most importantly:

  • Be very clear about the distinction between transactions and spans, and the relationship between spans in one service and their children in another service.

This final goal was the inspiration for and driving force behind this project.

Motivation

Currently, we use the terms "transaction," "span," and "transaction span." In the first case, "transaction" is a noun, and in the third it's an adjective (modifying the actual noun, which is "span"). And to be fair, in the current doc we're pretty good about not mixing up those usages. Nonetheless, depending on context, the noun "transaction" may mean:

  • The unit of information which gets sent to Sentry (in other words, a collection of spans).
  • The span which represents the overall operation, to which other spans are attached. Mostly we call this a "transaction span," but there are places where we say "transaction" and it's unclear whether we're talking about this one span or the overall group of spans.
  • The name of the transaction, as passed to the SDK and then used by the UI to identify the transaction.

Further, we say things like, "To start a transaction, call startSpan()..." but then also say things like, "To start a span, call startSpan()...".

For someone familiar with how we've built our AM system, this all makes sense. For someone who isn't, however, it's very easy to get confused: Is a transaction a span, or a group of spans? Is a transaction span the same thing as a transaction? Is a transaction span a transaction, or a span? How is it that you call startSpan() to start both a transaction and a span? Wait, when I'm passing a transaction argument to startSpan(), it's a description of the span rather than that span's transaction? But how is that different from description? And so on.

This is especially true because though "trace" and "span" are terms used in other tracing tools, "transaction" is one unique to Sentry, so even people coming in with prior knowledge of common tracing concepts won't know what it means.

Clarifying Changes

Therefore, this PR introduces two tiny but important changes in the language we use to describe these ideas. First, it emphasizes the tree-like structure of a transaction, to leverage a concept with which all readers are likely very familiar. Second, and more importantly, it stops talking about a "transaction span" and instead talks about a transaction's "root span." These changes accomplish a few things:

  • First and foremost, they fully decouple the terms "transaction" and "span." They are two different ideas, represented consistently by two different words.

    • Transaction: Tree-like structure, in which the nodes of the tree are spans. The unit of information which gets sent to Sentry and which is searchable in Discover. Generally represents an entire process on a given service. Always has a root span. May have a parent span in another process.
    • Span: Node of the transaction tree. Generally represents a sub-process, though each transaction has one span which represents the entire process. Not sent to Sentry on its own and not searchable in Discover. May have one or more child spans in its process, and one child transaction/child root span in a different process.
  • They make it clearer that each transaction has a single span from which all others descend. It's not a transaction span, it's the root span.

  • They make it easier to be very clear about the fact that, say, a front end XHR span is not the same span as the span which serves as the root of the request transaction on the back end, by letting us say that the XHR span is the request transaction's parent, rather than the root's parent. It's that, too, of course, but by inserting the idea of the transaction in between those two spans, it makes it super clear they're different.

Other Changes

In addition to the above (and some general wordsmithing), this PR includes a few other specific changes:

  • The organization is a little different, mostly as a result of pulling in the glossary stuff.

  • A note about the difference between profiling and tracing was added.

  • A detailed example was added, along with two other moderately detailed examples.

  • The diagrams were modified slightly (and a second version of one of the diagrams was added) to support the extended example and the new organization of information.

  • A "tracing data model" section was added, detailing what information is stored where. In this, the names of two fields were aliased for clarity: timestamp is called end_timestamp and transaction is called transaction_name. To be very clear: This does not change the underlying schema in any way. Under the hood, the fields retain their original names. The new names are only aliases, the handling of which will be done by the SDKs. (As part of this larger project, there will be future PRs, both to make the SDKs handle this correctly, and to change the SDK-specific parts of this doc to match.)

  • A section on the transaction list view (Discover results) was added.

  • A few additional available metrics were documented.

  • Two features in the transaction detail view (jumping to child transactions, and searching by trace id to find all transactions in a trace) were noted as behaving differently on Team than on Business.

@lobsterkatie lobsterkatie requested review from dashed, markstory, PeloWriter and a team May 7, 2020 21:30
@lobsterkatie
Copy link
Member Author

@dashed and @markstory - would especially love your help checking the stuff I wrote about discover and the metrics there, as that's the part of this with which I was least familiar.

@lobsterkatie
Copy link
Member Author

lobsterkatie commented May 7, 2020

EDIT: Added this as a disclaimer to the PR description.

(As part of this larger project, there will be future PRs, both to make the SDKs handle this correctly, and to change the SDK-specific parts of this doc to match.)

Just wanted to call this out, since I know that the top half (non-language-specific stuff) doesn't totally match the bottom half (language-specific stuff) all that well right now. It will, but in the interests of getting this out the door, I didn't touch that second section, save to take out the links to the glossary.

@dashed dashed requested a review from a team May 7, 2020 21:56
Copy link
Contributor

@PeloWriter PeloWriter left a comment

Choose a reason for hiding this comment

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

Katie - Thank you for all of the work here.

Copy link
Member

@HazAT HazAT left a comment

Choose a reason for hiding this comment

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

The explanation about traces/transactions & spans is soo good, this should make it very clear how we understand these concepts.

And generally a very good job of explaining how everything works with good examples.

🥇

Copy link
Contributor

@rhcarvalho rhcarvalho left a comment

Choose a reason for hiding this comment

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

@lobsterkatie, very well done! I absolutely love your work here. I did take the time to read and review and thus have a few suggestions and some style-guide-like questions as I may learn from them :-)

Users are gonna love the improvement!!!

You deserve a 🏆 and a 🎖️

lobsterkatie added a commit that referenced this pull request May 11, 2020
As pointed out in #1667 (comment), actual headings have many advantages over text made to look like a heading by bolding it and putting it on its own line.

In markdown, it's the difference between `#### Dogs Rule` and `**Dogs Rule**`. In HTML it means anchor links, consistent styling, and better accessibility (vs. lack of all of those things).

So, a clear win for headings, right? It would be, except that with our current CSS, it's next to impossible to tell an `h4` (`#### Dogs Rule`) from an `h3` (`### Dogs Rule`), because the difference is only a 4% reduction in size. Since using `h4`s breaks the document's visual hierarchy, folks in many cases choose to use the fake heading in spite of its drawbacks, because it _is_ visually distinct from an `h3`.

This shrinks the size of `h4`s so that you get the look of the fake heading, with all of the advantages of the real one.
Copy link
Contributor

@rhcarvalho rhcarvalho left a comment

Choose a reason for hiding this comment

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

This is a great improvement over the current state of the docs. @lobsterkatie you're awesome!

Some discussions for future improvements are tracked as issues, I think we should merge this in.

@lobsterkatie lobsterkatie changed the title ref(AM): Make theory explanations clearer, merge glossary into main doc ref(AM): Make theory explanations clearer, merge in glossary May 13, 2020
@lobsterkatie lobsterkatie merged commit 52d15a2 into master May 13, 2020
@lobsterkatie lobsterkatie deleted the kmclb-apm-overview-beginners-mind branch May 13, 2020 20:20
@github-actions github-actions bot locked and limited conversation to collaborators Dec 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants