-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
fix(tracing): Clean up sampling decision inheritance #2921
Conversation
size-limit report
|
6279d8f
to
b394cc4
Compare
b394cc4
to
d5cc2bc
Compare
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.
Good job @lobsterkatie! This was much easier to review this time 🎉
Only one thing I could not wrap my head around, the "H:" comment.
}); | ||
|
||
it('returns undefined if the header is malformed', () => { | ||
document.head.innerHTML = `<meta name="sentry-trace" content="12312012-112120121-0">`; |
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.
L:
💡 If we see a meta tag that is half-way there, <meta name="sentry-trace" content="some-invalid-data">
we should log an error / send a usage error to Sentry.
Such invalid tags are most likely coming from a wrong setup in the backend.
ada25c4
to
b6da190
Compare
b6da190
to
62e1701
Compare
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 @lobsterkatie
/me =>😴
Follow up to #2820.
Fixes a number of issues with sampling decision inheritance, some from the above PR, and some predating it.
true
were passing that decision down to child transactionssample
function wasn't adjusted to match.beforeNavigate
was failing to create a transaction ifundefined
was returned, rather than creating a transaction withsampled = false
to pass the negative sampling decision to its child spans/transactions<meta>
tag was getting used for all transactions on the front end, rather than just pageload transactions, with the result that all transactions in the session belonged to the same parent trace rather than each having their own tracesentry-trace
header was still getting attached to outgoing requests, even if tracing was disabled, so there was no way to distinguish that situation from a regularsampled = false
situation.This fixes all of those issues, so that
A transaction is always created, even whenUPDATE: will be fixed in a separate PRsampled
would befalse
(though non-sampled transactions don't collect any spans they spawn)sampled = false
, so that it can pass the decision along to its children (though here, too, the span is not stored for unsampled transactions)startTransaction({name: "...", sampled: true})
over inherited sampling decisions<meta>
tag is ignored for all but pageload transactionsNB: The test coverage here isn't complete, as it proved quite challenging to mock
XMLHttpRequest
,fetch
, and all of the other internals they touch. I was eventually successful with the former, but have yet to quite crack the latter, and it may have to wait for a separate PR. (The work there is on a separate branch.)To-do: