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

The SDK assumes a trace_sample_rate of 0 means we don't want to trace #595

Closed
AAllport opened this issue Oct 14, 2022 · 6 comments · Fixed by #600
Closed

The SDK assumes a trace_sample_rate of 0 means we don't want to trace #595

AAllport opened this issue Oct 14, 2022 · 6 comments · Fixed by #600
Assignees
Milestone

Comments

@AAllport
Copy link

Environment

When running v2.13, setting the SENTRY_TRACES_SAMPLE_RATE to 0.0 will disable the tracing integration.
Whilst this is understandable when serving the frontend via Laravel, this does not work well when using Laravel as an API only.

Our use case has a VueJs frontend, and a Mobile client.
We are enabling performance monitoring and initially hoped to have the sampling decision set on the VueJs frontend for this initial phase.

Due to how couldHavePerformanceTracingEnabled is implemented, a 0 value causes tracing not to be loaded.

protected function couldHavePerformanceTracingEnabled(): bool
{
$config = $this->getUserConfig();
return !empty($config['traces_sample_rate']) || !empty($config['traces_sampler']);
}

Steps to Reproduce

  1. Enable the SDK with a DSN and 0 Sample rate
  2. Have a front end with a positive sampling decision to make a request to the Laravel application.

Expected Result

A backend trace should be sent to Sentry with its parent set to the frontend id

Actual Result

The backend tracing integration was not enabled, and no trace was sent

@stayallive
Copy link
Collaborator

For reference this issue was sparked by a conversation in the Sentry Discord: https://discord.com/channels/621778831602221064/621964286952210432/1030461140913897482.

This is assumed to be undefined or at the very least unclear behaviour of the SDK and we should decide how to proceed because it would still be nice to have the SDK not do all the tracing bits if the user is never going to use it, but our very naive heuristic at the moment is obviously also not ideal or correct behaviour.

A workaround for now could be defining your own trace_sampler callback:

'traces_sampler' => function (\Sentry\Tracing\SamplingContext $context): float { return 0.0; },

However this is not ideal of course but does workaround the issue until we figure out how to fix it correctly.

@cleptric
Copy link
Member

Just confirmed, we have to respect the sampling decision of the HEAD SDK, so we should instrument this regardless.

@stayallive
Copy link
Collaborator

stayallive commented Oct 14, 2022

@cleptric how do you want to "fix" this? Want to do a bugfix or make this change in 3.x?

@cleptric
Copy link
Member

We should do this right away in a bug-fix release.

@cleptric
Copy link
Member

@AAllport First of all, thanks for reporting this. We actually discovered the underlying issue to be in our generic PHP SDK and will need to fix this there first.

@AAllport
Copy link
Author

Oh wow, didn't realise there was an issue with both of the SDKs.
I can totally understand how this happened, and it's somewhat of a non-issue for us now we've decided to just enable the sampling decision on all of the backend requests!

My point is, there's no rush from our end!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants