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

fix(tracing): Clean up sampling decision inheritance #2921

Merged
merged 8 commits into from
Oct 1, 2020

Conversation

lobsterkatie
Copy link
Member

@lobsterkatie lobsterkatie commented Sep 18, 2020

Follow up to #2820.

Fixes a number of issues with sampling decision inheritance, some from the above PR, and some predating it.

  • Only transactions with a sampling decision of true were passing that decision down to child transactions
  • In the previous PR, a parent sampling decision was differentiated from a transaction's own decision in the transaction context, but the behavior of the sample function wasn't adjusted to match.
  • beforeNavigate was failing to create a transaction if undefined was returned, rather than creating a transaction with sampled = false to pass the negative sampling decision to its child spans/transactions
  • The sentry-trace <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 trace
  • The sentry-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 regular sampled = false situation.

This fixes all of those issues, so that

  • A transaction is always created, even when sampled would be false (though non-sampled transactions don't collect any spans they spawn) UPDATE: will be fixed in a separate PR
  • A span is always created, even when its parent transaction has sampled = false, so that it can pass the decision along to its children (though here, too, the span is not stored for unsampled transactions)
  • Precedence is given to explicitly-set sampling decisions (i.e. the user calling startTransaction({name: "...", sampled: true}) over inherited sampling decisions
  • The <meta> tag is ignored for all but pageload transactions
  • No headers are attached to outgoing requests if tracing is disabled.

NB: 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:

  • Apply bugfixes to @sentry/apm (in separate PR)

@github-actions
Copy link
Contributor

github-actions bot commented Sep 18, 2020

size-limit report

Path Size
@sentry/browser - CDN Bundle (gzipped) 18.02 KB (0%)
@sentry/browser - Webpack 18.83 KB (0%)
@sentry/react - Webpack 18.83 KB (0%)
@sentry/browser + @sentry/tracing - CDN Bundle (gzipped) 23.88 KB (+0.26% 🔺)

packages/hub/src/scope.ts Outdated Show resolved Hide resolved
packages/tracing/test/hub.test.ts Show resolved Hide resolved
packages/tracing/test/hub.test.ts Show resolved Hide resolved
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.

Good job @lobsterkatie! This was much easier to review this time 🎉

Only one thing I could not wrap my head around, the "H:" comment.

packages/hub/src/scope.ts Outdated Show resolved Hide resolved
packages/tracing/src/browser/browsertracing.ts Outdated Show resolved Hide resolved
packages/tracing/src/browser/browsertracing.ts Outdated Show resolved Hide resolved
});

it('returns undefined if the header is malformed', () => {
document.head.innerHTML = `<meta name="sentry-trace" content="12312012-112120121-0">`;
Copy link
Contributor

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.

@lobsterkatie lobsterkatie force-pushed the kmclb-fix-sampling branch 2 times, most recently from ada25c4 to b6da190 Compare October 1, 2020 17:25
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.

👍 Thanks @lobsterkatie

/me =>😴

packages/tracing/src/browser/request.ts Show resolved Hide resolved
@lobsterkatie lobsterkatie merged commit e39c754 into master Oct 1, 2020
@lobsterkatie lobsterkatie deleted the kmclb-fix-sampling branch October 1, 2020 21:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants