getsentry
is_valid_sample_rate may be too strict
How do you use Sentry?
Sentry Saas (sentry.io)
Version
1.5.6
Steps to Reproduce
One day sentry stopped sending transactions to sentry.io. After much digging I found the issue to be here: https://github.com/getsentry/sentry-python/blob/0ba75fef404f877f3c7fc1afcc6013eb9c4b986c/sentry_sdk/tracing.py#L668
We have a custom traces_sampler function that reads in from a simple JSON file and sets the sample rate based on the endpoint. An example of our JSON:
{
"^$": 0.05,
"default_sample_rate": 1.0
}
We limit our root path /, because it gets polled by our load balancer once a minute. And the endpoint is blindingly simple and uninteresting, so we don't want to track all events and blow through our quota.
The problem came about when I switch from the json package to parse the json, to the json_encoder package, which generally is more robust json parser. The issue is that the package json will create a type of float when it deserializes the json, json_encoder will create a type of Decimal
Expected Result
- sample_rate with type Decimal should be sufficient for a return type of the traces_sampler function
- sentry documentation should state the exact allowable types for the traces_sampler function
Actual Result
transactions will be skipped if the type returned by traces_sampler function is not of type float.
Hey @timharsch !
Thanks for writing in. A really nice solution you have there for having dynamic sample rates for your endpoints.
I think there would be an easy fix by just adding a check for decimal here: https://github.com/getsentry/sentry-python/blob/master/sentry_sdk/tracing_utils.py#L183
You can supply a pull request with a fix, if you need this change fast.
I have put this in the internal backlog at low priority so it will take quite some time until we can tackle this, because we have a lot on our plates.
Hmm.. not sure why you are suggesting a change in the record_sql_queries function. It doesn't seem to relate.
I believe the fix should be to go from this code on line 132 https://github.com/getsentry/sentry-python/blob/0ba75fef404f877f3c7fc1afcc6013eb9c4b986c/sentry_sdk/tracing_utils.py#L132
if not isinstance(rate, Real) or math.isnan(rate):
to
if not isinstance(rate, Real) or not isinstance(rate, Decimal) or math.isnan(rate):
The float cast on line 141 should cast the Decimal value to float.
@timharsch - Can you please review - https://github.com/getsentry/sentry-python/pull/1672
