Backpressure: only downsample a max of 10 times (getsentry#2347)

sl0thentr0py · web-flow · commit d554a6cf95bd · 2023-09-04T15:19:20.000+02:00
diff --git a/sentry_sdk/monitor.py b/sentry_sdk/monitor.py
@@ -10,6 +10,9 @@
     from typing import Optional
 
 
+MAX_DOWNSAMPLE_FACTOR = 10
+
+
 class Monitor(object):
     """
     Performs health checks in a separate thread once every interval seconds
@@ -25,7 +28,7 @@ def __init__(self, transport, interval=10):
         self.interval = interval  # type: float
 
         self._healthy = True
-        self._downsample_factor = 1  # type: int
+        self._downsample_factor = 0  # type: int
 
         self._thread = None  # type: Optional[Thread]
         self._thread_lock = Lock()
@@ -64,13 +67,14 @@ def run(self):
     def set_downsample_factor(self):
         # type: () -> None
         if self._healthy:
-            if self._downsample_factor > 1:
+            if self._downsample_factor > 0:
                 logger.debug(
                     "[Monitor] health check positive, reverting to normal sampling"
                 )
-            self._downsample_factor = 1
+            self._downsample_factor = 0
         else:
-            self._downsample_factor *= 2
+            if self.downsample_factor < MAX_DOWNSAMPLE_FACTOR:
+                self._downsample_factor += 1
             logger.debug(
                 "[Monitor] health check negative, downsampling with a factor of %d",
                 self._downsample_factor,
diff --git a/sentry_sdk/tracing.py b/sentry_sdk/tracing.py
@@ -595,7 +595,7 @@ def finish(self, hub=None, end_timestamp=None):
             # exclusively based on sample rate but also traces sampler, but
             # we handle this the same here.
             if client.transport and has_tracing_enabled(client.options):
-                if client.monitor and client.monitor.downsample_factor > 1:
+                if client.monitor and client.monitor.downsample_factor > 0:
                     reason = "backpressure"
                 else:
                     reason = "sample_rate"
@@ -758,7 +758,7 @@ def _set_initial_sampling_decision(self, sampling_context):
         self.sample_rate = float(sample_rate)
 
         if client.monitor:
-            self.sample_rate /= client.monitor.downsample_factor
+            self.sample_rate /= 2**client.monitor.downsample_factor
 
         # if the function returned 0 (or false), or if `traces_sample_rate` is
         # 0, it's a sign the transaction should be dropped
diff --git a/tests/test_monitor.py b/tests/test_monitor.py
@@ -37,7 +37,7 @@ def test_monitor_if_enabled(sentry_init):
     assert monitor._thread is None
 
     assert monitor.is_healthy() is True
-    assert monitor.downsample_factor == 1
+    assert monitor.downsample_factor == 0
     assert monitor._thread is not None
     assert monitor._thread.name == "sentry.monitor"
 
@@ -49,11 +49,11 @@ def test_monitor_unhealthy(sentry_init):
     monitor.interval = 0.1
 
     assert monitor.is_healthy() is True
-    monitor.run()
-    assert monitor.is_healthy() is False
-    assert monitor.downsample_factor == 2
-    monitor.run()
-    assert monitor.downsample_factor == 4
+
+    for i in range(15):
+        monitor.run()
+        assert monitor.is_healthy() is False
+        assert monitor.downsample_factor == (i + 1 if i < 10 else 10)
 
 
 def test_transaction_uses_downsampled_rate(
@@ -75,7 +75,7 @@ def test_transaction_uses_downsampled_rate(
     assert monitor.is_healthy() is True
     monitor.run()
     assert monitor.is_healthy() is False
-    assert monitor.downsample_factor == 2
+    assert monitor.downsample_factor == 1
 
     with start_transaction(name="foobar") as transaction:
         assert transaction.sampled is False
