feat: Add status to transactions (getsentry#465)

untitaker · web-flow · commit eb947ef57106 · 2019-08-14T10:12:42.000+02:00
* feat: Add status to transactions

* fix: Fix celery tests

* fix: Revert added tags

* fix: Fix tests

* fix: Fix tests
diff --git a/sentry_sdk/hub.py b/sentry_sdk/hub.py
@@ -448,8 +448,6 @@ def span(
         except Exception:
             span.set_failure()
             raise
-        else:
-            span.set_success()
         finally:
             try:
                 span.finish()
diff --git a/sentry_sdk/integrations/celery.py b/sentry_sdk/integrations/celery.py
@@ -185,6 +185,10 @@ def _capture_exception(task, exc_info):
 
     hub.capture_event(event, hint=hint)
 
+    with capture_internal_exceptions():
+        with hub.configure_scope() as scope:
+            scope.span.set_failure()
+
 
 def _patch_worker_exit():
     # Need to flush queue before worker shutdown because a crashing worker will
diff --git a/sentry_sdk/integrations/sqlalchemy.py b/sentry_sdk/integrations/sqlalchemy.py
@@ -49,7 +49,6 @@ def _before_cursor_execute(
     span = ctx_mgr.__enter__()
 
     if span is not None:
-        span.set_success()  # might be overwritten later
         conn._sentry_sql_span = span
 
 
diff --git a/sentry_sdk/integrations/wsgi.py b/sentry_sdk/integrations/wsgi.py
@@ -1,3 +1,4 @@
+import functools
 import sys
 
 from sentry_sdk.hub import Hub, _should_send_default_pii
@@ -16,9 +17,14 @@
     from typing import Any
     from typing import Tuple
     from typing import Optional
+    from typing import TypeVar
 
     from sentry_sdk.utils import ExcInfo
 
+    T = TypeVar("T")
+    U = TypeVar("U")
+    E = TypeVar("E")
+
 
 if PY2:
 
@@ -90,15 +96,31 @@ def __call__(self, environ, start_response):
             span.op = "http.server"
             span.transaction = "generic WSGI request"
 
-            with hub.span(span):
+            with hub.span(span) as span:
                 try:
-                    rv = self.app(environ, start_response)
+                    rv = self.app(
+                        environ,
+                        functools.partial(_sentry_start_response, start_response, span),
+                    )
                 except BaseException:
                     reraise(*_capture_exception(hub))
 
         return _ScopedResponse(hub, rv)
 
 
+def _sentry_start_response(
+    old_start_response, span, status, response_headers, exc_info=None
+):
+    # type: (Callable[[str, U, Optional[E]], T], Span, str, U, Optional[E]) -> T
+    with capture_internal_exceptions():
+        status_int = int(status.split(" ", 1)[0])
+        span.set_tag("http.status_code", status_int)
+        if 500 <= status_int < 600:
+            span.set_failure()
+
+    return old_start_response(status, response_headers, exc_info)
+
+
 def _get_environ(environ):
     # type: (Dict[str, str]) -> Iterator[Tuple[str, str]]
     """
diff --git a/sentry_sdk/tracing.py b/sentry_sdk/tracing.py
@@ -192,18 +192,21 @@ def set_data(self, key, value):
         self._data[key] = value
 
     def set_failure(self):
-        self.set_tag("error", True)
+        self.set_tag("status", "failure")
 
     def set_success(self):
-        self.set_tag("error", False)
+        self.set_tag("status", "success")
+
+    def is_success(self):
+        return self._tags.get("status") in (None, "success")
 
     def finish(self):
         self.timestamp = datetime.now()
         if self._finished_spans is not None:
             self._finished_spans.append(self)
 
     def to_json(self):
-        return {
+        rv = {
             "trace_id": self.trace_id,
             "span_id": self.span_id,
             "parent_span_id": self.parent_span_id,
@@ -217,15 +220,22 @@ def to_json(self):
             "data": self._data,
         }
 
+        return rv
+
     def get_trace_context(self):
-        return {
+        rv = {
             "trace_id": self.trace_id,
             "span_id": self.span_id,
             "parent_span_id": self.parent_span_id,
             "op": self.op,
             "description": self.description,
         }
 
+        if "status" in self._tags:
+            rv["status"] = self._tags["status"]
+
+        return rv
+
 
 def _format_sql(cursor, sql):
     # type: (Any, str) -> Optional[str]
@@ -296,7 +306,7 @@ def record_http_request(hub, url, method):
 def maybe_create_breadcrumbs_from_span(hub, span):
     if span.op == "redis":
         hub.add_breadcrumb(type="redis", category="redis", data=span._tags)
-    elif span.op == "http" and not span._tags.get("error"):
+    elif span.op == "http" and span.is_success():
         hub.add_breadcrumb(
             type="http",
             category="httplib",
diff --git a/tests/integrations/celery/test_celery.py b/tests/integrations/celery/test_celery.py
@@ -123,6 +123,11 @@ def dummy_task(x, y):
     assert execution_event["contexts"]["trace"]["trace_id"] == span.trace_id
     assert submission_event["contexts"]["trace"]["trace_id"] == span.trace_id
 
+    if task_fails:
+        assert execution_event["contexts"]["trace"]["status"] == "failure"
+    else:
+        assert "status" not in execution_event["contexts"]["trace"]
+
     assert execution_event["spans"] == []
     assert submission_event["spans"] == [
         {
@@ -133,7 +138,7 @@ def dummy_task(x, y):
             u"same_process_as_parent": True,
             u"span_id": submission_event["spans"][0]["span_id"],
             u"start_timestamp": submission_event["spans"][0]["start_timestamp"],
-            u"tags": {u"error": False},
+            u"tags": {},
             u"timestamp": submission_event["spans"][0]["timestamp"],
             u"trace_id": text_type(span.trace_id),
         }
diff --git a/tests/integrations/flask/test_flask.py b/tests/integrations/flask/test_flask.py
@@ -557,6 +557,50 @@ def zerodivision(e):
     assert not events
 
 
+def test_tracing_success(sentry_init, capture_events, app):
+    sentry_init(traces_sample_rate=1.0, integrations=[flask_sentry.FlaskIntegration()])
+
+    events = capture_events()
+
+    with app.test_client() as client:
+        response = client.get("/message")
+        assert response.status_code == 200
+
+    message_event, transaction_event = events
+
+    assert transaction_event["type"] == "transaction"
+    assert transaction_event["transaction"] == "hi"
+    assert "status" not in transaction_event["contexts"]["trace"]
+
+    assert message_event["message"] == "hi"
+    assert message_event["transaction"] == "hi"
+
+
+def test_tracing_error(sentry_init, capture_events, app):
+    sentry_init(traces_sample_rate=1.0, integrations=[flask_sentry.FlaskIntegration()])
+
+    events = capture_events()
+
+    @app.route("/error")
+    def error():
+        1 / 0
+
+    with pytest.raises(ZeroDivisionError):
+        with app.test_client() as client:
+            response = client.get("/error")
+            assert response.status_code == 500
+
+    error_event, transaction_event = events
+
+    assert transaction_event["type"] == "transaction"
+    assert transaction_event["transaction"] == "error"
+    assert transaction_event["contexts"]["trace"]["status"] == "failure"
+
+    assert error_event["transaction"] == "error"
+    exception, = error_event["exception"]["values"]
+    assert exception["type"] == "ZeroDivisionError"
+
+
 def test_class_based_views(sentry_init, app, capture_events):
     sentry_init(integrations=[flask_sentry.FlaskIntegration()])
     events = capture_events()
diff --git a/tests/integrations/redis/test_redis.py b/tests/integrations/redis/test_redis.py
@@ -18,7 +18,7 @@ def test_basic(sentry_init, capture_events):
 
     assert crumb == {
         "category": "redis",
-        "data": {"error": False, "redis.key": "foobar"},
+        "data": {"redis.key": "foobar"},
         "timestamp": crumb["timestamp"],
         "type": "redis",
     }
diff --git a/tests/test_tracing.py b/tests/test_tracing.py
@@ -25,10 +25,10 @@ def test_basic(sentry_init, capture_events, sample_rate):
 
         span1, span2 = event["spans"]
         parent_span = event
-        assert span1["tags"]["error"]
+        assert span1["tags"]["status"] == "failure"
         assert span1["op"] == "foo"
         assert span1["description"] == "foodesc"
-        assert not span2["tags"]["error"]
+        assert "status" not in span2["tags"]
         assert span2["op"] == "bar"
         assert span2["description"] == "bardesc"
         assert parent_span["transaction"] == "hi"

Original file line number	Diff line number	Diff line change
`@@ -18,7 +18,7 @@ def test_basic(sentry_init, capture_events):`
`18`	`18`
`19`	`19`	`assert crumb == {`
`20`	`20`	`"category": "redis",`
`21`		`- "data": {"error": False, "redis.key": "foobar"},`
	`21`	`+ "data": {"redis.key": "foobar"},`
`22`	`22`	`"timestamp": crumb["timestamp"],`
`23`	`23`	`"type": "redis",`
`24`	`24`	`}`