fix: Do not capture SQL params for now (getsentry#503)

untitaker · web-flow · commit 36b88ff0c3dc · 2019-09-19T16:40:42.000+02:00
* fix: Do not capture SQL params for now

* fix: Discard entire span tree if too large

* ref: Send partial span tree instead of clearing it

* fix: Update tests

* feat: Add experiment to get back params
diff --git a/sentry_sdk/consts.py b/sentry_sdk/consts.py
@@ -49,6 +49,7 @@ def __init__(
         # DO NOT ENABLE THIS RIGHT NOW UNLESS YOU WANT TO EXCEED YOUR EVENT QUOTA IMMEDIATELY
         traces_sample_rate=0.0,  # type: float
         traceparent_v2=False,  # type: bool
+        _experiments={},  # type: Dict[str, Any]
     ):
         # type: (...) -> None
         pass
diff --git a/sentry_sdk/hub.py b/sentry_sdk/hub.py
@@ -450,7 +450,10 @@ def start_span(
             span.sampled = random.random() < sample_rate
 
         if span.sampled:
-            span.init_finished_spans()
+            max_spans = (
+                client and client.options["_experiments"].get("max_spans") or 1000
+            )
+            span.init_finished_spans(maxlen=max_spans)
 
         return span
 
diff --git a/sentry_sdk/tracing.py b/sentry_sdk/tracing.py
@@ -64,6 +64,31 @@ def __iter__(self):
             yield k[len(self.prefix) :]
 
 
+class _SpanRecorder(object):
+    __slots__ = ("maxlen", "finished_spans", "open_span_count")
+
+    def __init__(self, maxlen):
+        # type: (int) -> None
+        self.maxlen = maxlen
+        self.open_span_count = 0  # type: int
+        self.finished_spans = []  # type: List[Span]
+
+    def start_span(self, span):
+        # type: (Span) -> None
+
+        # This is just so that we don't run out of memory while recording a lot
+        # of spans. At some point we just stop and flush out the start of the
+        # trace tree (i.e. the first n spans with the smallest
+        # start_timestamp).
+        self.open_span_count += 1
+        if self.open_span_count > self.maxlen:
+            span._span_recorder = None
+
+    def finish_span(self, span):
+        # type: (Span) -> None
+        self.finished_spans.append(span)
+
+
 class Span(object):
     __slots__ = (
         "trace_id",
@@ -78,7 +103,7 @@ class Span(object):
         "timestamp",
         "_tags",
         "_data",
-        "_finished_spans",
+        "_span_recorder",
         "hub",
         "_context_manager_state",
     )
@@ -107,16 +132,18 @@ def __init__(
         self.hub = hub
         self._tags = {}  # type: Dict[str, str]
         self._data = {}  # type: Dict[str, Any]
-        self._finished_spans = None  # type: Optional[List[Span]]
         self.start_timestamp = datetime.now()
 
         #: End timestamp of span
         self.timestamp = None  # type: Optional[datetime]
 
-    def init_finished_spans(self):
-        # type: () -> None
-        if self._finished_spans is None:
-            self._finished_spans = []
+        self._span_recorder = None  # type: Optional[_SpanRecorder]
+
+    def init_finished_spans(self, maxlen):
+        # type: (int) -> None
+        if self._span_recorder is None:
+            self._span_recorder = _SpanRecorder(maxlen)
+        self._span_recorder.start_span(self)
 
     def __repr__(self):
         # type: () -> str
@@ -162,7 +189,8 @@ def new_span(self, **kwargs):
             sampled=self.sampled,
             **kwargs
         )
-        rv._finished_spans = self._finished_spans
+
+        rv._span_recorder = self._span_recorder
         return rv
 
     @classmethod
@@ -252,11 +280,13 @@ def finish(self, hub=None):
 
         self.timestamp = datetime.now()
 
-        if self._finished_spans is not None:
-            self._finished_spans.append(self)
-
         _maybe_create_breadcrumbs_from_span(hub, self)
 
+        if self._span_recorder is None:
+            return None
+
+        self._span_recorder.finish_span(self)
+
         if self.transaction is None:
             # If this has no transaction set we assume there's a parent
             # transaction for this span that would be flushed out eventually.
@@ -285,7 +315,9 @@ def finish(self, hub=None):
                 "timestamp": self.timestamp,
                 "start_timestamp": self.start_timestamp,
                 "spans": [
-                    s.to_json() for s in (self._finished_spans or ()) if s is not self
+                    s.to_json()
+                    for s in self._span_recorder.finished_spans
+                    if s is not self
                 ],
             }
         )
@@ -354,11 +386,19 @@ def record_sql_queries(
     executemany,  # type: bool
 ):
     # type: (...) -> Generator[Span, None, None]
-    if not params_list or params_list == [None]:
-        params_list = None
 
-    if paramstyle == "pyformat":
-        paramstyle = "format"
+    # TODO: Bring back capturing of params by default
+    if hub.client and hub.client.options["_experiments"].get(
+        "record_sql_params", False
+    ):
+        if not params_list or params_list == [None]:
+            params_list = None
+
+        if paramstyle == "pyformat":
+            paramstyle = "format"
+    else:
+        params_list = None
+        paramstyle = None
 
     query = _format_sql(cursor, query)
 
diff --git a/tests/integrations/django/test_basic.py b/tests/integrations/django/test_basic.py
@@ -170,7 +170,9 @@ def test_sql_queries(sentry_init, capture_events, with_integration):
     sentry_init(
         integrations=[DjangoIntegration()] if with_integration else [],
         send_default_pii=True,
+        _experiments={"record_sql_params": True},
     )
+
     from django.db import connection
 
     sql = connection.cursor()
@@ -193,7 +195,11 @@ def test_sql_queries(sentry_init, capture_events, with_integration):
 
 @pytest.mark.django_db
 def test_sql_dict_query_params(sentry_init, capture_events):
-    sentry_init(integrations=[DjangoIntegration()], send_default_pii=True)
+    sentry_init(
+        integrations=[DjangoIntegration()],
+        send_default_pii=True,
+        _experiments={"record_sql_params": True},
+    )
 
     from django.db import connections
 
@@ -230,7 +236,11 @@ def test_sql_dict_query_params(sentry_init, capture_events):
 )
 @pytest.mark.django_db
 def test_sql_psycopg2_string_composition(sentry_init, capture_events, query):
-    sentry_init(integrations=[DjangoIntegration()], send_default_pii=True)
+    sentry_init(
+        integrations=[DjangoIntegration()],
+        send_default_pii=True,
+        _experiments={"record_sql_params": True},
+    )
     from django.db import connections
 
     if "postgres" not in connections:
@@ -254,7 +264,11 @@ def test_sql_psycopg2_string_composition(sentry_init, capture_events, query):
 
 @pytest.mark.django_db
 def test_sql_psycopg2_placeholders(sentry_init, capture_events):
-    sentry_init(integrations=[DjangoIntegration()], send_default_pii=True)
+    sentry_init(
+        integrations=[DjangoIntegration()],
+        send_default_pii=True,
+        _experiments={"record_sql_params": True},
+    )
     from django.db import connections
 
     if "postgres" not in connections:
@@ -499,7 +513,11 @@ def test_does_not_capture_403(sentry_init, client, capture_events, endpoint):
 
 
 def test_middleware_spans(sentry_init, client, capture_events):
-    sentry_init(integrations=[DjangoIntegration()], traces_sample_rate=1.0)
+    sentry_init(
+        integrations=[DjangoIntegration()],
+        traces_sample_rate=1.0,
+        _experiments={"record_sql_params": True},
+    )
     events = capture_events()
 
     _content, status, _headers = client.get(reverse("message"))
diff --git a/tests/integrations/sqlalchemy/test_sqlalchemy.py b/tests/integrations/sqlalchemy/test_sqlalchemy.py
@@ -8,7 +8,9 @@
 
 
 def test_orm_queries(sentry_init, capture_events):
-    sentry_init(integrations=[SqlalchemyIntegration()])
+    sentry_init(
+        integrations=[SqlalchemyIntegration()], _experiments={"record_sql_params": True}
+    )
     events = capture_events()
 
     Base = declarative_base()
diff --git a/tests/test_tracing.py b/tests/test_tracing.py
@@ -127,3 +127,18 @@ def foo():
         gc.collect()
 
         assert len(references) == expected_refcount
+
+
+def test_span_trimming(sentry_init, capture_events):
+    sentry_init(traces_sample_rate=1.0, _experiments={"max_spans": 3})
+    events = capture_events()
+
+    with Hub.current.start_span(transaction="hi"):
+        for i in range(10):
+            with Hub.current.start_span(op="foo{}".format(i)):
+                pass
+
+    event, = events
+    span1, span2 = event["spans"]
+    assert span1["op"] == "foo0"
+    assert span2["op"] == "foo1"
