Skip to content

Set traces_sample_rate to null by default #616

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

Merged
merged 4 commits into from
Nov 22, 2022

Conversation

stayallive
Copy link
Collaborator

@stayallive stayallive commented Nov 21, 2022

(missing sentry/sentry release is failing the tests)

@cleptric cleptric marked this pull request as ready for review November 22, 2022 11:32
@cleptric cleptric added this to the 3.2.0 milestone Nov 22, 2022
@cleptric cleptric enabled auto-merge (squash) November 22, 2022 15:02
@cleptric cleptric merged commit 77029df into master Nov 22, 2022
@cleptric cleptric deleted the feature/traces_sample_rate branch November 22, 2022 15:03
@cleptric cleptric modified the milestones: 3.2.0, 3.1.2 Nov 23, 2022
@negoziator
Copy link

negoziator commented Nov 23, 2022

@stayallive I really like this change, since the default behavior is now not to send trace samples ( which can get pretty expensive quickly.... )

However .. i'm just curious why it wouldn't be enough to just do it like this instead of the explicit type check for null` ?

'traces_sample_rate' => env('SENTRY_TRACES_SAMPLE_RATE') ? (float)env('SENTRY_TRACES_SAMPLE_RATE') : null 

I'm maybe overseeing something, but by doing it like this - it seems to me like there's a reason for being very explicit ( and also more complex to read )

@stayallive
Copy link
Collaborator Author

stayallive commented Nov 23, 2022

To be clear, there were never any traces sent by default, except when you had a front-end library instructing to send traces which caught some users off guard (understandably). This is why this was changed, but we didn't send traces by default without any input (from the front-end or setting this to > 0). Just to be clear to future readers 😃

Some would argue your example is harder to read, that is code style preference (especially your earlier edit with ?;). However, as you suspected there is an functional difference too. Your earlier example doesn't check for null but for truthy values, a string or true or something else entirely is also truthy and the PHP SDK will fail on that so we check a little more explicit since only null and int or float is acceptable (other types throw a exception which we rather avoid).

Edit: Feel free to change the example provided with whatever works and reads best for you of course 👌

@stayallive
Copy link
Collaborator Author

stayallive commented Nov 23, 2022

So my comment became a bit of a mess because you changed your example (I answered based on the GitHub notification e-mail) sorry about that :)

Your current example doesn't allow setting the traces_sample_rate to 0 since that would result in null which is different from setting it to 0. Simply put, you want to be able to set the traces_sample_rate anywhere from 0.0 to 1.0 and null so that's why I've chosen to implement it like it is in the PR.

@negoziator
Copy link

I get your point. Thanks for taking your time to elaborate.

It actually makes sense, that

if you set it to 0 - then you want the value to be 0.0

In my way - when you set it to 0 - that would elaborate to null - i get it, that's weird behavior.

Alright. Thank you.

And yes -- sorry for the confusion about the default behavior! 😄

@stayallive
Copy link
Collaborator Author

It's not incorrect but also not something we can fix since the Laravel env helper returns mixed although in reality it would only return a number or null if setup correctly for this key. The old previous value we had should have the same problem.

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.

3 participants