Skip to content
This repository was archived by the owner on Aug 14, 2024. It is now read-only.

Conversation

@rhcarvalho
Copy link
Contributor

@rhcarvalho rhcarvalho commented Jul 21, 2021

Follows up on #356 and #381.

Documents the current Sentry span ingestion model, compare the model to alternatives, lists and explains several identified issues and wraps up with a summary for all identified issues (scope propagation and ingestion model).

@vercel
Copy link

vercel bot commented Jul 21, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/sentry/develop/BacxBbYohnrw9WCvCRrF7VVc1QrS
✅ Preview: https://develop-git-rhcarvalho-performance-evolution-issues-inge-27eabe.sentry.dev

@rhcarvalho
Copy link
Contributor Author

Starting with an example trace to explain the current ingestion model and how it differs from OpenTelemetry and others.

Next, will go into specific issues.

Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

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

Might be better suited for another section, but can we add a note how opentelemetry propogates contextual data? (span events/attributes) vs. Sentry's tags/contexts/extra on the transaction event.

The numbers seemed fine when there were just 2 items, but when there
will be several items under "span ingestion model", numbering all of
them is silly and error prone.
@rhcarvalho rhcarvalho marked this pull request as ready for review July 23, 2021 10:13
@rhcarvalho
Copy link
Contributor Author

Might be better suited for another section, but can we add a note how opentelemetry propogates contextual data? (span events/attributes) vs. Sentry's tags/contexts/extra on the transaction event.

@AbhiPrasad I saw this suggestion, but not sure how to fit it in the current document.

Copy link
Member

@lobsterkatie lobsterkatie 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 great, @rhcarvalho! 🙂

I've made a whole boatload of suggestions, but the majority of them are just copy editing - in descending LOGAF order, they consist of grammar fixes, changes to make the language more idiomatic, and generalized wordsmithing. In the places where I made larger changes to sentences, I tried to explain why, but also obviously feel free to ask if anything is unclear.

There are also a small handful of legit questions or comments, but overall I think the content here is really good!


The implementation of the actual `trace` function is relatively simple (see [a PR which has an example implementation](https://github.com/getsentry/sentry-javascript/pull/3697/files#diff-f5bf6e0cdf7709e5675fcdc3b4ff254dd68f3c9d1a399c8751e0fa1846fa85dbR158)), however, knowing what is the current span in async code and global integrations is a challenge yet to be overcome.

The following two examples synthesize the scope propagation issues.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The following two examples synthesize the scope propagation issues.
The following two examples demonstrate the scope propagation issues.

Copy link
Contributor Author

@rhcarvalho rhcarvalho Jul 26, 2021

Choose a reason for hiding this comment

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

Hmm here I meant a bit more than just "demonstrate". I used "synthesize" because, I my view at the time, the two examples fully cover and define the issues.

But perhaps we don't need such precision, and "demonstrate" is a simpler word, possibly easier to understand, without much loss in meaning.

Copy link
Member

Choose a reason for hiding this comment

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

Hmmm - I'm trying to come up with a better word than "demonstrate," then.

I replaced "synthesize" because it needs something or someone to be the subject, doing the synthesis of the examples/ideas/etc (the object) into some coherent whole. What about "encapsulate?"


We have learned a lot with the current tracing implementation at Sentry. This document is an attempt to capture many of the known limitations to serve as basis for future improvement.

Tracing is a complex subject, and taming that complexity is no easy feat.
Copy link
Member

Choose a reason for hiding this comment

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

I really like this sentence. I feel like I should steal it for the top of the nextjs SDK docs. "Next.js is a complex framework, and taming that complexity is no easy feat. We're working on it - be patient."

@rhcarvalho
Copy link
Contributor Author

Thanks for the review, @lobsterkatie!

@rhcarvalho
Copy link
Contributor Author

All feedback applied, thanks again @lobsterkatie!

@rhcarvalho rhcarvalho requested review from jan-auer and mitsuhiko July 29, 2021 13:54
@rhcarvalho
Copy link
Contributor Author

@jan-auer or @mitsuhiko could you give a quick pass on this before we merge it? I'd like to get your agreement at least with regards to the high level issues documented here.

Copy link
Contributor

@vladanpaunovic vladanpaunovic left a comment

Choose a reason for hiding this comment

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

Great post @rhcarvalho! Thanks to this documentation I learned so much more about how our spans work. Thank you.

@rhcarvalho
Copy link
Contributor Author

Considering summer and this topic has been open and shared with relevant folks for some time now, we're opting to merge it as is and any further feedback or concerns can be addressed through issues or follow up PRs.

1 similar comment
@rhcarvalho
Copy link
Contributor Author

Considering summer and this topic has been open and shared with relevant folks for some time now, we're opting to merge it as is and any further feedback or concerns can be addressed through issues or follow up PRs.

@rhcarvalho rhcarvalho merged commit 7e79679 into master Aug 4, 2021
@rhcarvalho rhcarvalho deleted the rhcarvalho/performance-evolution-issues-ingestion-model branch August 4, 2021 12:16
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.

5 participants