Browse Source

feat(dynamic-sampling): Use sliding window sample rate for low volume transactions bias (#49533)

Riccardo Busetti 1 year ago
parent
commit
36ecca570f

+ 1 - 1
src/sentry/dynamic_sampling/rules/base.py

@@ -43,7 +43,7 @@ def get_guarded_blended_sample_rate(organization: Organization, project: Project
         # whereas if we don't find a value in cache, we just sample at 100% under the assumption that the project
         # has just been created.
         sample_rate = get_sliding_window_sample_rate(
-            project=project, error_sample_rate_fallback=sample_rate
+            org_id=organization.id, project_id=project.id, error_sample_rate_fallback=sample_rate
         )
     else:
         # In case we use the prioritise by project, we want to fall back to the original sample rate in case there are

+ 4 - 7
src/sentry/dynamic_sampling/rules/helpers/sliding_window.py

@@ -1,15 +1,12 @@
 from calendar import IllegalMonthError, monthrange
 from datetime import datetime
-from typing import TYPE_CHECKING, Optional
+from typing import Optional
 
 import pytz
 
 from sentry import options
 from sentry.dynamic_sampling.rules.utils import get_redis_client_for_ds
 
-if TYPE_CHECKING:
-    from sentry.models import Project
-
 # In case a misconfiguration happens on the server side which makes the option invalid, we want to define a fallback
 # sliding window size, which in this case will be 24 hours.
 FALLBACK_SLIDING_WINDOW_SIZE = 24
@@ -23,13 +20,13 @@ def generate_sliding_window_cache_key(org_id: int) -> str:
 
 
 def get_sliding_window_sample_rate(
-    project: "Project", error_sample_rate_fallback: float
+    org_id: int, project_id: int, error_sample_rate_fallback: float
 ) -> Optional[float]:
     redis_client = get_redis_client_for_ds()
-    cache_key = generate_sliding_window_cache_key(project.organization.id)
+    cache_key = generate_sliding_window_cache_key(org_id=org_id)
 
     try:
-        value = redis_client.hget(cache_key, project.id)
+        value = redis_client.hget(cache_key, project_id)
         # In case we had an explicit error, we want to fetch the blended sample rate to avoid oversampling.
         if value == SLIDING_WINDOW_CALCULATION_ERROR:
             return error_sample_rate_fallback

+ 15 - 2
src/sentry/dynamic_sampling/tasks.py

@@ -20,6 +20,7 @@ from sentry.dynamic_sampling.recalibrate_transactions import (
     OrganizationDataVolume,
     fetch_org_volumes,
 )
+from sentry.dynamic_sampling.rules.base import is_sliding_window_enabled
 from sentry.dynamic_sampling.rules.helpers.prioritise_project import _generate_cache_key
 from sentry.dynamic_sampling.rules.helpers.prioritize_transactions import (
     set_transactions_resampling_rates,
@@ -30,6 +31,7 @@ from sentry.dynamic_sampling.rules.helpers.sliding_window import (
     generate_sliding_window_cache_key,
     generate_sliding_window_org_cache_key,
     get_sliding_window_org_sample_rate,
+    get_sliding_window_sample_rate,
     get_sliding_window_size,
 )
 from sentry.dynamic_sampling.rules.utils import (
@@ -191,7 +193,6 @@ def process_transaction_biases(project_transactions: ProjectTransactions) -> Non
     A task that given a project relative transaction counts calculates rebalancing
     sampling rates based on the overall desired project sampling rate.
     """
-
     org_id = project_transactions["org_id"]
     project_id = project_transactions["project_id"]
     total_num_transactions = project_transactions.get("total_num_transactions")
@@ -200,7 +201,19 @@ def process_transaction_biases(project_transactions: ProjectTransactions) -> Non
         DSElement(id=id, count=count) for id, count in project_transactions["transaction_counts"]
     ]
 
-    sample_rate = quotas.get_blended_sample_rate(organization_id=org_id)
+    try:
+        organization = Organization.objects.get_from_cache(id=org_id)
+    except Organization.DoesNotExist:
+        organization = None
+
+    if organization is not None and is_sliding_window_enabled(organization):
+        sample_rate = get_sliding_window_sample_rate(
+            org_id=org_id,
+            project_id=project_id,
+            error_sample_rate_fallback=quotas.get_blended_sample_rate(organization_id=org_id),
+        )
+    else:
+        sample_rate = quotas.get_blended_sample_rate(organization_id=org_id)
 
     if sample_rate is None or sample_rate == 1.0:
         # no sampling => no rebalancing

+ 94 - 50
tests/sentry/dynamic_sampling/test_tasks.py

@@ -10,6 +10,10 @@ from sentry.dynamic_sampling.rules.biases.recalibration_bias import Recalibratio
 from sentry.dynamic_sampling.rules.helpers.prioritize_transactions import (
     get_transactions_resampling_rates,
 )
+from sentry.dynamic_sampling.rules.helpers.sliding_window import (
+    SLIDING_WINDOW_CALCULATION_ERROR,
+    generate_sliding_window_cache_key,
+)
 from sentry.dynamic_sampling.rules.utils import RuleType, generate_cache_key_rebalance_factor
 from sentry.dynamic_sampling.tasks import (
     prioritise_projects,
@@ -293,33 +297,66 @@ class TestPrioritiseTransactionsTask(BaseMetricsLayerTestCase, TestCase, SnubaTe
         }
         return idx + counts[name]
 
+    def set_sliding_window_cache_entry(self, org_id: int, project_id: int, value: str):
+        redis = get_redis_client_for_ds()
+        cache_key = generate_sliding_window_cache_key(org_id=org_id)
+        redis.hset(cache_key, project_id, value)
+
+    def set_sliding_window_sample_rate(self, org_id: int, project_id: int, sample_rate: float):
+        self.set_sliding_window_cache_entry(org_id, project_id, str(sample_rate))
+
+    def set_sliding_window_error(self, org_id: int, project_id: int):
+        # We want also to test for this case in order to verify the fallback to the `get_blended_sample_rate`.
+        self.set_sliding_window_cache_entry(org_id, project_id, SLIDING_WINDOW_CALCULATION_ERROR)
+
+    def set_sliding_window_error_for_all(self):
+        for org in self.orgs_info:
+            org_id = org["org_id"]
+            for project_id in org["project_ids"]:
+                self.set_sliding_window_error(org_id, project_id)
+
+    def set_sliding_window_sample_rate_for_all(self, sample_rate: float):
+        for org in self.orgs_info:
+            org_id = org["org_id"]
+            for project_id in org["project_ids"]:
+                self.set_sliding_window_sample_rate(org_id, project_id, sample_rate)
+
     @patch("sentry.dynamic_sampling.rules.base.quotas.get_blended_sample_rate")
     def test_prioritise_transactions_simple(self, get_blended_sample_rate):
         """
         Create orgs projects & transactions and then check that the task creates rebalancing data
         in Redis
         """
-        get_blended_sample_rate.return_value = 0.25
-
-        with self.options(
-            {
-                "dynamic-sampling.prioritise_transactions.load_rate": 1.0,
-            }
-        ):
-            with self.tasks():
-                prioritise_transactions()
+        BLENDED_RATE = 0.25
+        get_blended_sample_rate.return_value = BLENDED_RATE
 
-        # now redis should contain rebalancing data for our projects
-        for org in self.orgs_info:
-            org_id = org["org_id"]
-            for proj_id in org["project_ids"]:
-                tran_rate, global_rate = get_transactions_resampling_rates(
-                    org_id=org_id, proj_id=proj_id, default_rate=0.1
-                )
-                for transaction_name in ["ts1", "ts2", "tm3", "tl4", "tl5"]:
-                    assert (
-                        transaction_name in tran_rate
-                    )  # check we have some rate calculated for each transaction
+        for (sliding_window_error, used_sample_rate) in ((True, BLENDED_RATE), (False, 0.5)):
+            if sliding_window_error:
+                self.set_sliding_window_error_for_all()
+            else:
+                self.set_sliding_window_sample_rate_for_all(used_sample_rate)
+
+            with self.options(
+                {
+                    "dynamic-sampling.prioritise_transactions.load_rate": 1.0,
+                }
+            ):
+                with self.feature("organizations:ds-sliding-window"):
+                    with self.tasks():
+                        prioritise_transactions()
+
+            # now redis should contain rebalancing data for our projects
+            for org in self.orgs_info:
+                org_id = org["org_id"]
+                for proj_id in org["project_ids"]:
+                    tran_rate, global_rate = get_transactions_resampling_rates(
+                        org_id=org_id, proj_id=proj_id, default_rate=0.1
+                    )
+                    for transaction_name in ["ts1", "ts2", "tm3", "tl4", "tl5"]:
+                        assert (
+                            transaction_name in tran_rate
+                        )  # check we have some rate calculated for each transaction
+                    assert global_rate == used_sample_rate
 
     @patch("sentry.dynamic_sampling.rules.base.quotas.get_blended_sample_rate")
     def test_prioritise_transactions_partial(self, get_blended_sample_rate):
@@ -333,36 +370,43 @@ class TestPrioritiseTransactionsTask(BaseMetricsLayerTestCase, TestCase, SnubaTe
         BLENDED_RATE = 0.25
         get_blended_sample_rate.return_value = BLENDED_RATE
 
-        with self.options(
-            {
-                "dynamic-sampling.prioritise_transactions.load_rate": 1.0,
-                "dynamic-sampling.prioritise_transactions.num_explicit_large_transactions": 1,
-                "dynamic-sampling.prioritise_transactions.num_explicit_small_transactions": 1,
-                "dynamic-sampling.prioritise_transactions.rebalance_intensity": 0.7,
-            }
-        ):
-            with self.tasks():
-                prioritise_transactions()
-
-        # now redis should contain rebalancing data for our projects
-        for org in self.orgs_info:
-            org_id = org["org_id"]
-            for proj_id in org["project_ids"]:
-                tran_rate, implicit_rate = get_transactions_resampling_rates(
-                    org_id=org_id, proj_id=proj_id, default_rate=0.1
-                )
-                # explicit transactions
-                for transaction_name in ["ts1", "tl5"]:
-                    assert (
-                        transaction_name in tran_rate
-                    )  # check we have some rate calculated for each transaction
-                # implicit transactions
-                for transaction_name in ["ts2", "tm3", "tl4"]:
-                    assert (
-                        transaction_name not in tran_rate
-                    )  # check we have some rate calculated for each transaction
-                # we do have some different rate for implicit transactions
-                assert implicit_rate != BLENDED_RATE
+        for (sliding_window_error, used_sample_rate) in ((True, BLENDED_RATE), (False, 0.5)):
+            if sliding_window_error:
+                self.set_sliding_window_error_for_all()
+            else:
+                self.set_sliding_window_sample_rate_for_all(used_sample_rate)
+
+            with self.options(
+                {
+                    "dynamic-sampling.prioritise_transactions.load_rate": 1.0,
+                    "dynamic-sampling.prioritise_transactions.num_explicit_large_transactions": 1,
+                    "dynamic-sampling.prioritise_transactions.num_explicit_small_transactions": 1,
+                    "dynamic-sampling.prioritise_transactions.rebalance_intensity": 0.7,
+                }
+            ):
+                with self.feature("organizations:ds-sliding-window"):
+                    with self.tasks():
+                        prioritise_transactions()
+
+            # now redis should contain rebalancing data for our projects
+            for org in self.orgs_info:
+                org_id = org["org_id"]
+                for proj_id in org["project_ids"]:
+                    tran_rate, implicit_rate = get_transactions_resampling_rates(
+                        org_id=org_id, proj_id=proj_id, default_rate=0.1
+                    )
+                    # explicit transactions
+                    for transaction_name in ["ts1", "tl5"]:
+                        assert (
+                            transaction_name in tran_rate
+                        )  # check we have some rate calculated for each transaction
+                    # implicit transactions
+                    for transaction_name in ["ts2", "tm3", "tl4"]:
+                        assert (
+                            transaction_name not in tran_rate
+                        )  # check we have some rate calculated for each transaction
+                    # we do have some different rate for implicit transactions
+                    assert implicit_rate != BLENDED_RATE
 
 
 @freeze_time(MOCK_DATETIME)