feat(dynamic-sampling): Implement dynamic factor function and new factor-based rules [TET-702] #44500
Conversation
github-actions
bot
added
the
Scope: Backend
iambriccardo
changed the title
iambriccardo
force-pushed
the
riccardo/feat/dynamic-factor-function
branch
from
485bb06 to
06e0430
Compare
| "The dynamic factor function requires a sample rate in the interval [0.0, 1.0]." | ||
| ) | ||
|
|
||
| return float(x / x**base_sample_rate) |
There was a problem hiding this comment.
IMO function doc string should demonstrate this formula rather then saying in general def a factor.
There was a problem hiding this comment.
I agree, I will describe it.
| @@ -12,19 +12,23 @@ | |||
| ) | |||
| from sentry.dynamic_sampling.rules.helpers.latest_releases import ProjectBoostedReleases | |||
| from sentry.dynamic_sampling.rules.utils import ( | |||
| RELEASE_BOOST_FACTOR, | |||
| BOOST_LATEST_RELEASES_DECAYED_FACTOR, | |||
| BOOST_LATEST_RELEASES_FACTOR, | |||
There was a problem hiding this comment.
i don't understand the purpose of this rename?
RELEASE_BOOST_FACTOR,
sounds IMO correct, wdyt @RaduW ?
There was a problem hiding this comment.
It's aligned with the file naming, this is the rationale.
| @@ -331,7 +331,7 @@ def test_generate_rules_return_uniform_rules_and_key_transaction_rule_with_many_ | |||
| "op": "or", | |||
| }, | |||
| "id": 1003, | |||
| "samplingValue": {"type": "sampleRate", "value": 0.5}, | |||
| "samplingValue": {"type": "factor", "value": 1.5}, | |||
There was a problem hiding this comment.
please use constants here - we do the same for globs.
| for each rule based on the base_sample_rate of the project. | ||
|
|
||
| The function decreases the factor from x to 1 in the interval [0.0, 1.0] which means that a user with | ||
| base_sample_rate = 0.0 will have a factor of x, whereas with base_sample_rate = 1.0 we will have a factor of 1. |
There was a problem hiding this comment.
i think we will raise an exception if sample rate is 0.
There was a problem hiding this comment.
Yeah, that makes sense. It's not a problem mathematically but it doesn't make sense to compute any factors if that is the case.
There was a problem hiding this comment.
to demonstrate this you can add assert base_sample_rate == 0.0
| base_sample_rate = 0.0 will have a factor of x, whereas with base_sample_rate = 1.0 we will have a factor of 1. | ||
|
|
||
| The high-level idea is that we want to reduce the factor the bigger the base_sample_rate becomes, this is done | ||
| because multiplication will exceed 1 very quickly in case we don't reduce the factor. |
There was a problem hiding this comment.
i think you can keep only this sentence and remove first two.
| @patch("sentry.dynamic_sampling.rules.base.quotas.get_blended_sample_rate") | ||
| def test_generate_rules_return_uniform_rules_and_key_transaction_rule( | ||
| get_blended_sample_rate, default_project, default_team | ||
| get_blended_sample_rate, apply_dynamic_factor, default_project, default_team |
There was a problem hiding this comment.
Why do these tests mock apply_dynamic_factor? Couldn't we just assert on the real return value?
This PR aims at implementing the new dynamic factor function and factor-based rules introduced in Relay with the following PR -> getsentry/relay#1790.