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(apm): Sampling of traces #2500

Merged
merged 35 commits into from
Mar 19, 2020
Merged

fix(apm): Sampling of traces #2500

merged 35 commits into from
Mar 19, 2020

Conversation

HazAT
Copy link
Member

@HazAT HazAT commented Mar 18, 2020

Reworking the sampling decision, now everything is consistent with it was meant to be.

  • fix sampling of traces work now only depending on the client option tracesSampleRate
  • remove internal forceNoChild parameter from hub.startSpan
  • make constructor of Span internal, only use hub.startSpan

Also set op otherwise span will be discarded by the server
@HazAT HazAT requested a review from kamilogorek as a code owner March 18, 2020 08:00
@HazAT HazAT requested a review from rhcarvalho March 18, 2020 08:01
@getsentry-bot
Copy link
Contributor

getsentry-bot commented Mar 18, 2020

Messages
📖

@sentry/browser bundle gzip'ed minified size: (ES5: 16.9023 kB) (ES6: 15.9434 kB)

📖 ✅ TSLint passed

Generated by 🚫 dangerJS against 106905d

@@ -402,6 +402,8 @@ export class Tracing implements Integration {

const span = hub.startSpan(
{
op: 'operation',
sampled: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

FTR (as we talked over video chat), I believe more users will forget to set this and get surprised and frustrated.
It feels like only the SDK itself should ever set the valued of sampled, never the user.

@HazAT HazAT changed the title fix(apm): Set sampled and op by default fix(apm): Sampling of traces Mar 18, 2020
@HazAT
Copy link
Member Author

HazAT commented Mar 18, 2020

This is the docs pr
getsentry/sentry-docs#1556

const experimentsOptions = (client && client.getOptions()._experiments) || {};
span.initFinishedSpans(experimentsOptions.maxSpans as number);
}
// We always want to record spans independent from the sample rate
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please add a short summary of why?

Co-Authored-By: Rodolfo Carvalho <rodolfo.carvalho@sentry.io>
HazAT and others added 5 commits March 18, 2020 11:40
Co-Authored-By: Rodolfo Carvalho <rodolfo.carvalho@sentry.io>
Co-Authored-By: Rodolfo Carvalho <rodolfo.carvalho@sentry.io>
@@ -187,7 +192,10 @@ export class Span implements SpanInterface, SpanContext {
traceId: this._traceId,
});

span.spanRecorder = this.spanRecorder;
Copy link
Member

Choose a reason for hiding this comment

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

This is already happening in finishSpan

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.

Let's get this in and keep iterating.

@HazAT HazAT merged commit dea97aa into master Mar 19, 2020
@HazAT HazAT deleted the apm/set-sampled branch March 19, 2020 14:25
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.

5 participants