feat(metrics): Stronger recursion protection (#2426)

Author: mitsuhiko

sentry_sdk/metrics.py

@@ -7,6 +7,7 @@
 import zlib
 from functools import wraps, partial
 from threading import Event, Lock, Thread
+from contextlib import contextmanager
 
 from sentry_sdk._compat import text_type
 from sentry_sdk.hub import Hub
@@ -26,6 +27,7 @@
     from typing import Iterable
     from typing import Callable
     from typing import Optional
+    from typing import Generator
     from typing import Tuple
 
     from sentry_sdk._types import BucketKey
@@ -53,21 +55,33 @@
 )
 
 
+@contextmanager
+def recursion_protection():
+    # type: () -> Generator[bool, None, None]
+    """Enters recursion protection and returns the old flag."""
+    try:
+        in_metrics = _thread_local.in_metrics
+    except AttributeError:
+        in_metrics = False
+    _thread_local.in_metrics = True
+    try:
+        yield in_metrics
+    finally:
+        _thread_local.in_metrics = in_metrics
+
+
 def metrics_noop(func):
     # type: (Any) -> Any
+    """Convenient decorator that uses `recursion_protection` to
+    make a function a noop.
+    """
+
     @wraps(func)
     def new_func(*args, **kwargs):
         # type: (*Any, **Any) -> Any
-        try:
-            in_metrics = _thread_local.in_metrics
-        except AttributeError:
-            in_metrics = False
-        _thread_local.in_metrics = True
-        try:
+        with recursion_protection() as in_metrics:
             if not in_metrics:
                 return func(*args, **kwargs)
-        finally:
-            _thread_local.in_metrics = in_metrics
 
     return new_func
 
@@ -449,7 +463,16 @@ def _emit(
         encoded_metrics = _encode_metrics(flushable_buckets)
         metric_item = Item(payload=encoded_metrics, type="statsd")
         envelope = Envelope(items=[metric_item])
-        self._capture_func(envelope)
+
+        # A malfunctioning transport might create a forever loop of metric
+        # emission when it emits a metric in capture_envelope.  We still
+        # allow the capture to take place, but interior metric incr calls
+        # or similar will be disabled.  In the background thread this can
+        # never happen, but in the force flush case which happens in the
+        # foreground we might make it here unprotected.
+        with recursion_protection():
+            self._capture_func(envelope)
+
         return envelope
 
     def _serialize_tags(
@@ -495,8 +518,10 @@ def _get_aggregator_and_update_tags(key, tags):
 
     callback = client.options.get("_experiments", {}).get("before_emit_metric")
     if callback is not None:
-        if not callback(key, updated_tags):
-            return None, updated_tags
+        with recursion_protection() as in_metrics:
+            if not in_metrics:
+                if not callback(key, updated_tags):
+                    return None, updated_tags
 
     return client.metrics_aggregator, updated_tags
 

tests/test_metrics.py

@@ -418,6 +418,8 @@ def before_emit(key, tags):
             return False
         tags["extra"] = "foo"
         del tags["release"]
+        # this better be a noop!
+        metrics.incr("shitty-recursion")
         return True
 
     sentry_init(
@@ -501,3 +503,32 @@ def test_tag_serialization(sentry_init, capture_envelopes):
         "release": "fun-release",
         "environment": "not-fun-env",
     }
+
+
+def test_flush_recursion_protection(sentry_init, capture_envelopes, monkeypatch):
+    sentry_init(
+        release="fun-release",
+        environment="not-fun-env",
+        _experiments={"enable_metrics": True},
+    )
+    envelopes = capture_envelopes()
+    test_client = Hub.current.client
+
+    real_capture_envelope = test_client.transport.capture_envelope
+
+    def bad_capture_envelope(*args, **kwargs):
+        metrics.incr("bad-metric")
+        return real_capture_envelope(*args, **kwargs)
+
+    monkeypatch.setattr(test_client.transport, "capture_envelope", bad_capture_envelope)
+
+    metrics.incr("counter")
+
+    # flush twice to see the inner metric
+    Hub.current.flush()
+    Hub.current.flush()
+
+    (envelope,) = envelopes
+    m = parse_metrics(envelope.items[0].payload.get_bytes())
+    assert len(m) == 1
+    assert m[0][1] == "counter@none"