Skip to content

Make sure to use the default decimal context in our code #4231

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

Make sure to use the default decimal context in our code #4231

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d19053e
Make sure to use the default decimal context in our code
antonpirker Apr 2, 2025
72c7d3d
Merge branch 'master' into antonpirker/fix/modified-decimal-context
antonpirker Apr 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions sentry_sdk/tracing.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
from decimal import Decimal
import uuid
import warnings
from datetime import datetime, timedelta, timezone
Expand Down Expand Up @@ -1187,10 +1188,8 @@ def _set_initial_sampling_decision(self, sampling_context):
self.sampled = False
return

# Now we roll the dice. self._sample_rand is inclusive of 0, but not of 1,
# so strict < is safe here. In case sample_rate is a boolean, cast it
# to a float (True becomes 1.0 and False becomes 0.0)
self.sampled = self._sample_rand < self.sample_rate
# Now we roll the dice.
self.sampled = self._sample_rand < Decimal.from_float(self.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.

Why do we need this? Just wondering if this can also potentially fail due to unforeseen circumstances 🙃

Copy link
Member Author

Choose a reason for hiding this comment

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

when people set the FloatOperation trap (like the user in the linked issue did) then the old comparison fails, because then comparing Decimals to floats is not allowed anymore. I guess we also could add this around the comparison:

 with localcontext(DefaultContext) as ctx:
     ctx.prec = 6
     self.sampled = self._sample_rand < self.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.

I don't really have a preference, but maybe wrapping stuff in a localcontext might be good, just to possibly avoid other unforeseen errors? I'm kinda afraid of the whole decimal API now so I'd make this as defensive as possible.


if self.sampled:
logger.debug(
Expand Down
13 changes: 8 additions & 5 deletions sentry_sdk/tracing_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
import sys
from collections.abc import Mapping
from datetime import timedelta
from decimal import ROUND_DOWN, Context, Decimal
from decimal import ROUND_DOWN, Decimal, DefaultContext, localcontext
from functools import wraps
from random import Random
from urllib.parse import quote, unquote
Expand Down Expand Up @@ -872,10 +872,13 @@ def _generate_sample_rand(

# Round down to exactly six decimal-digit precision.
# Setting the context is needed to avoid an InvalidOperation exception
# in case the user has changed the default precision.
return Decimal(sample_rand).quantize(
Decimal("0.000001"), rounding=ROUND_DOWN, context=Context(prec=6)
)
# in case the user has changed the default precision or set traps.
with localcontext(DefaultContext) as ctx:
Copy link
Member

Choose a reason for hiding this comment

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

Small readability nit, I would import decimal then change this to

Suggested change
with localcontext(DefaultContext) as ctx:
with decimal.localcontext(DefaultContext) as ctx:

Otherwise, from just reading this code, it is unclear that the localcontext is a decimal-related thing

ctx.prec = 6
return Decimal(sample_rand).quantize(
Decimal("0.000001"),
rounding=ROUND_DOWN,
)


def _sample_rand_range(parent_sampled, sample_rate):
Expand Down
10 changes: 9 additions & 1 deletion tests/tracing/test_sample_rand.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import decimal
from decimal import Inexact, FloatOperation
from unittest import mock

import pytest
Expand Down Expand Up @@ -58,14 +59,19 @@ def test_transaction_uses_incoming_sample_rand(

def test_decimal_context(sentry_init, capture_events):
"""
Ensure that having a decimal context with a precision below 6
Ensure that having a user altered decimal context with a precision below 6
does not cause an InvalidOperation exception.
"""
sentry_init(traces_sample_rate=1.0)
events = capture_events()

old_prec = decimal.getcontext().prec
old_inexact = decimal.getcontext().traps[Inexact]
old_float_operation = decimal.getcontext().traps[FloatOperation]

decimal.getcontext().prec = 2
decimal.getcontext().traps[Inexact] = True
decimal.getcontext().traps[FloatOperation] = True

try:
with mock.patch(
Expand All @@ -77,5 +83,7 @@ def test_decimal_context(sentry_init, capture_events):
)
finally:
decimal.getcontext().prec = old_prec
decimal.getcontext().traps[Inexact] = old_inexact
decimal.getcontext().traps[FloatOperation] = old_float_operation

assert len(events) == 1
Loading