-
Notifications
You must be signed in to change notification settings - Fork 552
feat(tracing): Use sample_rand
for sampling decisions
#4044
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
feat(tracing): Use sample_rand
for sampling decisions
#4044
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## szokeasaurusrex/sample_rand-3 #4044 +/- ##
=================================================================
+ Coverage 79.55% 79.59% +0.03%
=================================================================
Files 140 140
Lines 15570 15579 +9
Branches 2641 2643 +2
=================================================================
+ Hits 12387 12400 +13
+ Misses 2343 2341 -2
+ Partials 840 838 -2
|
c3d6d12
to
5a906f6
Compare
af64b16
to
01334d7
Compare
5a906f6
to
548396d
Compare
01334d7
to
7ec8e08
Compare
c4abdc9
to
5f57723
Compare
sentry_sdk/tracing.py
Outdated
if baggage_sample_rand is not None: | ||
self._sample_rand = baggage_sample_rand | ||
else: | ||
self._sample_rand = Random(self.trace_id).random() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In what cases will we have to generate a new _sample_rand
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whenever we are starting a new trace, i.e. if we call start_transaction
without calling continue_trace
first and without manually passing in baggage
5f57723
to
b66c955
Compare
7ec8e08
to
b400fb1
Compare
b66c955
to
dc4c9aa
Compare
b400fb1
to
cf51e51
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just comments on the tests, testing exact float values like that can be non-deterministic so maybe better to just mock out the rng
tests/tracing/test_sample_rand.py
Outdated
|
||
TEST_TRACE_ID_SAMPLE_RANDS = { | ||
"00000000000000000000000000000000": 0.8766381713144122, | ||
"01234567012345670123456701234567": 0.6451742521664413, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also not sure if rng is consistent like that across python versions and so on?
dc4c9aa
to
ffd3ead
Compare
cf51e51
to
0db414f
Compare
ffd3ead
to
2186af6
Compare
0db414f
to
620b617
Compare
2186af6
to
7d8d1f1
Compare
c8820e2
to
64b4d84
Compare
7d8d1f1
to
a2852ba
Compare
a2852ba
to
30769fe
Compare
64b4d84
to
6e56143
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks ok -- please see comments
@@ -636,7 +636,7 @@ async def handler(request): | |||
|
|||
assert ( | |||
resp.request_info.headers["baggage"] | |||
== "custom=value,sentry-trace_id=0123456789012345678901234567890,sentry-environment=production,sentry-release=d08ebdb9309e1b004c6f52202de58a09c2268e42,sentry-transaction=/interactions/other-dogs/new-dog,sentry-sample_rate=1.0,sentry-sampled=true" | |||
== "custom=value,sentry-trace_id=0123456789012345678901234567890,sentry-sample_rand=0.3015579701611357,sentry-environment=production,sentry-release=d08ebdb9309e1b004c6f52202de58a09c2268e42,sentry-transaction=/interactions/other-dogs/new-dog,sentry-sample_rate=1.0,sentry-sampled=true" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't sample_rand
have 6 digits after the decimal point?
tests/tracing/test_sample_rand.py
Outdated
import sentry_sdk | ||
from sentry_sdk.tracing_utils import Baggage | ||
|
||
TEST_TRACE_ID_SAMPLE_RANDS = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These too -- aren't they supposed to have 6 decimal places?
30769fe
to
20bfe5a
Compare
11cfacd
to
f77a9ec
Compare
ad9d103
to
8d4d858
Compare
f77a9ec
to
61ae6e7
Compare
74e7c41
to
5d65dc0
Compare
Use the `sample_rand` value from an incoming trace to make sampling decisions, rather than generating a random value. When we are the head SDK starting a new trace, save our randomly-generated value as the `sample_rand`, and also change the random generation logic so that the `sample_rand` is computed deterministically based on the `trace_id`. Closes #3998
5d65dc0
to
0d49ac2
Compare
Use the
sample_rand
value from an incoming trace to make sampling decisions, rather than generating a random value. When we are the head SDK starting a new trace, save our randomly-generated value as thesample_rand
, and also change the random generation logic so that thesample_rand
is computed deterministically based on thetrace_id
.Depends on:
sample_rand
onPropagationContext
#4038Closes #3998
Thank you for contributing to
sentry-python
! Please add tests to validate your changes, and lint your code usingtox -e linters
.Running the test suite on your PR might require maintainer approval. The AWS Lambda tests additionally require a maintainer to add a special label, and they will fail until this label is added.