fix(tracing) Omit the top level status tag (getsentry#644)

markstory · untitaker · web-flow · commit 909ecaa14423 · 2020-03-13T10:49:49.000-04:00
In the product side we don't really want people to search by this tag as
it is far more expensive than the `transaction.status` property which is
indexed separately. By not emitting this tag and only including it in
the trace context we won't end up with poor performing tags for users to
click on.

To workaround get_trace_context() being called multiple times I needed
an additional non-tags place to store the status. I've had to shim the
status back into tags as non-transaction spans expect to have status as
a tag.

Co-authored-by: Markus Unterwaditzer &lt;markus@unterwaditzer.net&gt;
diff --git a/sentry_sdk/tracing.py b/sentry_sdk/tracing.py
@@ -103,6 +103,7 @@ class Span(object):
         "description",
         "start_timestamp",
         "_start_timestamp_monotonic",
+        "status",
         "timestamp",
         "_tags",
         "_data",
@@ -122,6 +123,7 @@ def __init__(
         op=None,  # type: Optional[str]
         description=None,  # type: Optional[str]
         hub=None,  # type: Optional[sentry_sdk.Hub]
+        status=None,  # type: Optional[str]
     ):
         # type: (...) -> None
         self.trace_id = trace_id or uuid.uuid4().hex
@@ -132,6 +134,7 @@ def __init__(
         self.transaction = transaction
         self.op = op
         self.description = description
+        self.status = status
         self.hub = hub
         self._tags = {}  # type: Dict[str, str]
         self._data = {}  # type: Dict[str, Any]
@@ -183,7 +186,7 @@ def __enter__(self):
     def __exit__(self, ty, value, tb):
         # type: (Optional[Any], Optional[Any], Optional[Any]) -> None
         if value is not None:
-            self._tags.setdefault("status", "internal_error")
+            self.set_status("internal_error")
 
         hub, scope, old_span = self._context_manager_state
         del self._context_manager_state
@@ -272,7 +275,7 @@ def set_data(self, key, value):
 
     def set_status(self, value):
         # type: (str) -> None
-        self.set_tag("status", value)
+        self.status = value
 
     def set_http_status(self, http_status):
         # type: (int) -> None
@@ -309,7 +312,7 @@ def set_http_status(self, http_status):
 
     def is_success(self):
         # type: () -> bool
-        return self._tags.get("status") == "ok"
+        return self.status == "ok"
 
     def finish(self, hub=None):
         # type: (Optional[sentry_sdk.Hub]) -> Optional[str]
@@ -387,6 +390,9 @@ def to_json(self, client):
         if transaction:
             rv["transaction"] = transaction
 
+        if self.status:
+            self._tags["status"] = self.status
+
         tags = self._tags
         if tags:
             rv["tags"] = tags
@@ -406,9 +412,8 @@ def get_trace_context(self):
             "op": self.op,
             "description": self.description,
         }
-
-        if "status" in self._tags:
-            rv["status"] = self._tags["status"]
+        if self.status:
+            rv["status"] = self.status
 
         return rv
 
diff --git a/tests/test_tracing.py b/tests/test_tracing.py
@@ -12,7 +12,8 @@ def test_basic(sentry_init, capture_events, sample_rate):
     sentry_init(traces_sample_rate=sample_rate)
     events = capture_events()
 
-    with Hub.current.start_span(transaction="hi"):
+    with Hub.current.start_span(transaction="hi") as span:
+        span.set_status("ok")
         with pytest.raises(ZeroDivisionError):
             with Hub.current.start_span(op="foo", description="foodesc"):
                 1 / 0
@@ -32,6 +33,8 @@ def test_basic(sentry_init, capture_events, sample_rate):
         assert span2["op"] == "bar"
         assert span2["description"] == "bardesc"
         assert parent_span["transaction"] == "hi"
+        assert "status" not in event["tags"]
+        assert event["contexts"]["trace"]["status"] == "ok"
     else:
         assert not events
 
