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

Allow 0.0 to be set on tracesSampleRate. #1328

Merged
merged 4 commits into from
Mar 19, 2021
Merged

Allow 0.0 to be set on tracesSampleRate. #1328

merged 4 commits into from
Mar 19, 2021

Conversation

maciejwalkowiak
Copy link
Contributor

📜 Description

Allow 0.0 to be set on tracesSampleRate.

💡 Motivation and Context

Fixes #1326.

💚 How did you test it?

📝 Checklist

  • I reviewed the submitted code
  • I added tests to verify the changes
  • I updated the docs if needed
  • No breaking changes

@maciejwalkowiak
Copy link
Contributor Author

The question is what's the point of having null on tracesSampleRate? Shouldn't we default it to 0.0? Or this is actually a bug, and having null on tracesSampleRate should make all startTransaction calls return no-op?

@marandaneto
Copy link
Contributor

The question is what's the point of having null on tracesSampleRate? Shouldn't we default it to 0.0? Or this is actually a bug, and having null on tracesSampleRate should make all startTransaction calls return no-op?

@maciejwalkowiak that was the point.

@bruno-garcia to confirm but if I understood correctly means:

tracesSampleRate and tracesSampler == null -> Tracing is fully disabled, we could do NoOp IMO as discussed in the meeting.

tracesSampleRate == 0.0, means no transaction will be created by this SDK unless an incoming trace has been sampled

@marandaneto
Copy link
Contributor

missing @tests and changelog, let's wait @bruno-garcia to confirm though

@marandaneto
Copy link
Contributor

@bruno-garcia thats all we need right?
@maciejwalkowiak could u work on that, so I can cut a release with it :)

Copy link
Member

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

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

Other than the note from @marandaneto LGTM.
As Manoel already wrote on this PR: null means no transactions should be emitted.

But 0.0 means no chance of random sampling creating transactions, but if an incoming request is sampled, the SDK will honor that decision.

@bruno-garcia
Copy link
Member

@maciejwalkowiak
Copy link
Contributor Author

@bruno-garcia docs updated: getsentry/sentry-docs#3277

Disabling performance feature when tracesSampleRate is null will come in subsequent PR.

@maciejwalkowiak maciejwalkowiak merged commit e9a865b into main Mar 19, 2021
@maciejwalkowiak maciejwalkowiak deleted the gh-1326 branch March 19, 2021 10:03
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.

Allow tracesSampleRate = 0.0
3 participants