ref: Next iteration of span API (getsentry#466)

untitaker · web-flow · commit 6253e288b597 · 2019-08-16T14:53:01.000+02:00
* ref: Next iteration of span API

* fix: Do not allow user to finish a span twice

* fix: Fix tests

* fix: Fix aiohttp integration

* doc: Add documentation and shortcut method
diff --git a/mypy.ini b/mypy.ini
@@ -22,9 +22,6 @@ warn_redundant_casts = True
 
 ; Relaxations:
 
-[mypy-sentry_sdk.tracing]
-disallow_untyped_defs = False
-
 [mypy-sentry_sdk._compat]
 disallow_untyped_defs = False
 
diff --git a/sentry_sdk/api.py b/sentry_sdk/api.py
@@ -15,6 +15,7 @@
     from typing import ContextManager
 
     from sentry_sdk._types import Event, Hint, Breadcrumb, BreadcrumbHint
+    from sentry_sdk.tracing import Span
 
     T = TypeVar("T")
     F = TypeVar("F", bound=Callable[..., Any])
@@ -34,6 +35,7 @@ def overload(x):
     "push_scope",
     "flush",
     "last_event_id",
+    "start_span",
 ]
 
 
@@ -179,3 +181,15 @@ def last_event_id():
     if hub is not None:
         return hub.last_event_id()
     return None
+
+
+@hubmethod
+def start_span(
+    span=None,  # type: Optional[Span]
+    **kwargs  # type: Any
+):
+    # type: (...) -> Span
+
+    # TODO: All other functions in this module check for
+    # `Hub.current is None`. That actually should never happen?
+    return Hub.current.start_span(span=span, **kwargs)
diff --git a/sentry_sdk/hub.py b/sentry_sdk/hub.py
@@ -10,7 +10,7 @@
 from sentry_sdk._compat import with_metaclass
 from sentry_sdk.scope import Scope
 from sentry_sdk.client import Client
-from sentry_sdk.tracing import Span, maybe_create_breadcrumbs_from_span
+from sentry_sdk.tracing import Span
 from sentry_sdk.utils import (
     exc_info_from_error,
     event_from_exception,
@@ -128,17 +128,6 @@ def main(self):
         return GLOBAL_HUB
 
 
-class _HubManager(object):
-    def __init__(self, hub):
-        # type: (Hub) -> None
-        self._old = Hub.current
-        _local.set(hub)
-
-    def __exit__(self, exc_type, exc_value, tb):
-        # type: (Any, Any, Any) -> None
-        _local.set(self._old)
-
-
 class _ScopeManager(object):
     def __init__(self, hub):
         # type: (Hub) -> None
@@ -429,44 +418,27 @@ def add_breadcrumb(
         while len(scope._breadcrumbs) > max_breadcrumbs:
             scope._breadcrumbs.popleft()
 
-    @contextmanager
-    def span(
-        self,
-        span=None,  # type: Optional[Span]
-        **kwargs  # type: Any
-    ):
-        # type: (...) -> Generator[Span, None, None]
-        # TODO: Document
-        span = self.start_span(span=span, **kwargs)
-
-        _, scope = self._stack[-1]
-        old_span = scope.span
-        scope.span = span
-
-        try:
-            yield span
-        except Exception:
-            span.set_failure()
-            raise
-        finally:
-            try:
-                span.finish()
-                maybe_create_breadcrumbs_from_span(self, span)
-                self.finish_span(span)
-            except Exception:
-                self._capture_internal_exception(sys.exc_info())
-            scope.span = old_span
-
     def start_span(
         self,
         span=None,  # type: Optional[Span]
         **kwargs  # type: Any
     ):
         # type: (...) -> Span
-        # TODO: Document
+        """
+        Create a new span whose parent span is the currently active
+        span, if any. The return value is the span object that can
+        be used as a context manager to start and stop timing.
+
+        Note that you will not see any span that is not contained
+        within a transaction. Create a transaction with
+        ``start_span(transaction="my transaction")`` if an
+        integration doesn't already do this for you.
+        """
 
         client, scope = self._stack[-1]
 
+        kwargs.setdefault("hub", self)
+
         if span is None:
             if scope.span is not None:
                 span = scope.span.new_span(**kwargs)
@@ -482,48 +454,6 @@ def start_span(
 
         return span
 
-    def finish_span(
-        self, span  # type: Span
-    ):
-        # type: (...) -> Optional[str]
-        # TODO: Document
-        if span.timestamp is None:
-            # This transaction is not yet finished so we just finish it.
-            span.finish()
-
-        if span.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.
-            return None
-
-        if self.client is None:
-            # We have no client and therefore nowhere to send this transaction
-            # event.
-            return None
-
-        if not span.sampled:
-            # At this point a `sampled = None` should have already been
-            # resolved to a concrete decision. If `sampled` is `None`, it's
-            # likely that somebody used `with Hub.span(..)` on a
-            # non-transaction span and later decided to make it a transaction.
-            assert (
-                span.sampled is not None
-            ), "Need to set transaction when entering span!"
-            return None
-
-        return self.capture_event(
-            {
-                "type": "transaction",
-                "transaction": span.transaction,
-                "contexts": {"trace": span.get_trace_context()},
-                "timestamp": span.timestamp,
-                "start_timestamp": span.start_timestamp,
-                "spans": [
-                    s.to_json() for s in (span._finished_spans or ()) if s is not span
-                ],
-            }
-        )
-
     @overload  # noqa
     def push_scope(
         self, callback=None  # type: Optional[None]
diff --git a/sentry_sdk/integrations/aiohttp.py b/sentry_sdk/integrations/aiohttp.py
@@ -65,7 +65,7 @@ async def inner():
 
                     # If this transaction name makes it to the UI, AIOHTTP's
                     # URL resolver did not find a route or died trying.
-                    with hub.span(transaction="generic AIOHTTP request"):
+                    with hub.start_span(transaction="generic AIOHTTP request"):
                         try:
                             response = await old_handle(self, request)
                         except HTTPException:
diff --git a/sentry_sdk/integrations/celery.py b/sentry_sdk/integrations/celery.py
@@ -79,7 +79,7 @@ def apply_async(*args, **kwargs):
             if headers is not None:
                 kwargs["headers"] = headers
 
-            with hub.span(op="celery.submit", description=task.name):
+            with hub.start_span(op="celery.submit", description=task.name):
                 return f(*args, **kwargs)
         else:
             return f(*args, **kwargs)
@@ -114,7 +114,7 @@ def _inner(*args, **kwargs):
                 # something such as attribute access can fail.
                 span.transaction = task.name
 
-            with hub.span(span):
+            with hub.start_span(span):
                 return f(*args, **kwargs)
 
     return _inner
diff --git a/sentry_sdk/integrations/redis.py b/sentry_sdk/integrations/redis.py
@@ -32,7 +32,7 @@ def sentry_patched_execute_command(self, name, *args, **kwargs):
 
                 description = " ".join(description_parts)
 
-            with hub.span(op="redis", description=description) as span:
+            with hub.start_span(op="redis", description=description) as span:
                 if name and args and name.lower() in ("get", "set", "setex", "setnx"):
                     span.set_tag("redis.key", args[0])
 
diff --git a/sentry_sdk/integrations/rq.py b/sentry_sdk/integrations/rq.py
@@ -55,7 +55,7 @@ def sentry_patched_perform_job(self, job, *args, **kwargs):
                 with capture_internal_exceptions():
                     span.transaction = job.func_name
 
-                with hub.span(span):
+                with hub.start_span(span):
                     rv = old_perform_job(self, job, *args, **kwargs)
 
             if self.is_horse:
diff --git a/sentry_sdk/integrations/stdlib.py b/sentry_sdk/integrations/stdlib.py
@@ -181,7 +181,7 @@ def sentry_patched_popen_init(self, *a, **kw):
                 env = _init_argument(a, kw, "env", 10, lambda x: dict(x or os.environ))
             env["SUBPROCESS_" + k.upper().replace("-", "_")] = v
 
-        with hub.span(op="subprocess", description=description) as span:
+        with hub.start_span(op="subprocess", description=description) as span:
             span.set_data("subprocess.cwd", cwd)
 
             return old_popen_init(self, *a, **kw)
diff --git a/sentry_sdk/integrations/wsgi.py b/sentry_sdk/integrations/wsgi.py
@@ -96,7 +96,7 @@ def __call__(self, environ, start_response):
             span.op = "http.server"
             span.transaction = "generic WSGI request"
 
-            with hub.span(span) as span:
+            with hub.start_span(span) as span:
                 try:
                     rv = self.app(
                         environ,
diff --git a/sentry_sdk/tracing.py b/sentry_sdk/tracing.py
diff --git a/tests/integrations/celery/test_celery.py b/tests/integrations/celery/test_celery.py
diff --git a/tests/integrations/stdlib/test_subprocess.py b/tests/integrations/stdlib/test_subprocess.py
diff --git a/tests/test_tracing.py b/tests/test_tracing.py
