feat: Introduce Transaction and Hub.start_transaction #747

rhcarvalho · 2020-06-26T22:35:13Z

This aligns the tracing implementation with the current JS tracing implementation, up to a certain extent.

Hub.start_transaction or start_transaction are meant to be used when starting transactions, replacing most uses of Hub.start_span / start_span.

Spans are typically created from their parent transactions via transaction.start_child, or start_span relying on the transaction being in the current scope.

It is okay to start a transaction without a name and set it later.
Sometimes the proper name is not known until after the transaction has started.

We could fail the transaction if it has no name when calling the finish method. Instead, set a default name that will prompt users to give a name to their transactions. This is the same behavior as implemented in JS.

rhcarvalho · 2020-06-27T13:14:44Z

I don't know what's wrong with the docs generation job https://travis-ci.com/github/getsentry/sentry-python/jobs/354722815

AbhiPrasad

LGTM. Figured I would start reviewing these PRs to get more familiar with this SDK, but I'll leave Markus to give the final ✅

AbhiPrasad · 2020-06-29T12:03:42Z

sentry_sdk/hub.py

+        # This is for backwards compatibility with releases before
+        # start_transaction existed, to allow for a smoother transition.
+        if isinstance(span, Transaction) or "transaction" in kwargs:
+            deprecation_msg = (


I don't think we need the redundant check here for isinstance(span, Transaction) or "transaction" in kwargs, we should just extract deprecation_msg to be a top level constant

The intention here was to have a self-contained block with the TODO note, so it is clearer what it applies to.

AbhiPrasad · 2020-06-29T12:05:50Z

sentry_sdk/hub.py

+        # appending an existing span to a new transaction created to
+        # contain the span.
+
+        # TODO: consider removing this in a future release.


As an aside for another PR: we should have a standard deprecated comment somewhere like we do in JS, so we can easily see what to remove in each major version.

sentry_sdk/scope.py

sentry_sdk/tracing.py

AbhiPrasad · 2020-06-29T12:38:11Z

sentry_sdk/hub.py

+        #
+        # Would be clearer if Hub was not involved:
+        #     transaction.name = "new_name"
+        #     with transaction:  # __enter__ => start, __exit__ => finish


Wouldn't we still have to attach the transaction to the current hub, even with with transaction:? That way, I'd much prefer to be explicit using hub.start_transaction then hide what it's doing here.

AbhiPrasad · 2020-06-29T12:41:42Z

sentry_sdk/hub.py

+        # XXX: should we always set transaction.hub = self?
+        # In a multi-hub program, what does it mean to write
+        #     hub1.start_transaction(Transaction(hub=hub2))
+        # ? Should the transaction be on hub1 or hub2?


Hmm, this is tricky. I think if we keep our current implementation, the transaction should be on hub1 and we should not allow setting hub through kwargs. If we decide to change the API, we can revisit this.

untitaker

I would shift each of those discussions into github issues if you want to continue them

untitaker · 2020-06-29T13:05:11Z

sentry_sdk/hub.py

+                name = kwargs.pop("transaction")
+                return self.start_transaction(name=name, **kwargs)
+
+        # XXX: this is not very useful because


This is there to be consistent with start_transaction I suppose.

Well, start_transaction doesn't exist outside this PR. In one of its usages, the previous start_span would also just return the input (sampling decision work moved to transactions only).

Keeping it for backwards-compat.

untitaker · 2020-06-29T13:07:13Z

sentry_sdk/hub.py

+        if span is not None:
+            return span.start_child(**kwargs)
+
+        # XXX: returning a detached span here means whatever span tree built


This makes the API simpler to use as the caller does not have to check whether they are in a transaction. I don't really see why you call this not useful.

This can hide an entire instrumented span tree if one is not careful to guarantee a transaction is in the scope when calling hub.start_span(op="...").

You told me today about a use case for intentionally omitting an entire instrumented span tree, I still keeping wrap my head around that. In other parts of the Unified API we explicitly prefer to avoid doing things when not necessary -- e.g. no-op transport/client when SDK has no DSN, callbacks are not called, etc.

I'll move these discussions into issues.

sentry_sdk/scope.py

untitaker · 2020-06-29T13:09:22Z

sentry_sdk/hub.py

+        #     hub1.start_transaction(Transaction(hub=hub2))
+        # ? Should the transaction be on hub1 or hub2?
+
+        # XXX: it is strange that both start_span and start_transaction take


btw i think those discussions should not be done as comments in the sourcecode

rhcarvalho · 2020-06-29T13:32:17Z

I don't know what's wrong with the docs generation job https://travis-ci.com/github/getsentry/sentry-python/jobs/354722815

Circular imports.

Removed the SpanLike type, making the total diff smaller and solving the circular import.

diff --git a/sentry_sdk/_types.py b/sentry_sdk/_types.py
index 7c1aaa6..8e5427c 100644
--- a/sentry_sdk/_types.py
+++ b/sentry_sdk/_types.py
@@ -12,7 +12,7 @@ if MYPY:
     from typing import Optional
     from typing import Tuple
     from typing import Type
-    from typing import Union
+
     from typing_extensions import Literal
 
     ExcInfo = Tuple[
@@ -37,7 +37,3 @@ if MYPY:
     ]
     SessionStatus = Literal["ok", "exited", "crashed", "abnormal"]
     EndpointType = Literal["store", "envelope"]
-
-    from sentry_sdk.tracing import Span, Transaction  # noqa
-
-    SpanLike = Union[Span, Transaction]
diff --git a/sentry_sdk/scope.py b/sentry_sdk/scope.py
index 0c082c4..f928063 100644
--- a/sentry_sdk/scope.py
+++ b/sentry_sdk/scope.py
@@ -26,7 +26,7 @@ if MYPY:
         Type,
     )
 
-    from sentry_sdk.tracing import SpanLike
+    from sentry_sdk.tracing import Span
     from sentry_sdk.sessions import Session
 
     F = TypeVar("F", bound=Callable[..., Any])
@@ -114,7 +114,7 @@ class Scope(object):
         self.clear_breadcrumbs()
         self._should_capture = True
 
-        self._span = None  # type: Optional[SpanLike]
+        self._span = None  # type: Optional[Span]
         self._session = None  # type: Optional[Session]
         self._force_auto_session_tracking = None  # type: Optional[bool]
 
@@ -181,13 +181,13 @@ class Scope(object):
 
     @property
     def span(self):
-        # type: () -> Optional[SpanLike]
+        # type: () -> Optional[Span]
         """Get/set current tracing span or transaction."""
         return self._span
 
     @span.setter
     def span(self, span):
-        # type: (Optional[SpanLike]) -> None
+        # type: (Optional[Span]) -> None
         self._span = span
         # XXX: this differs from the implementation in JS, there Scope.setSpan
         # does not set Scope._transactionName.
diff --git a/sentry_sdk/tracing.py b/sentry_sdk/tracing.py
index 05abcf6..d390c6b 100644
--- a/sentry_sdk/tracing.py
+++ b/sentry_sdk/tracing.py
@@ -25,9 +25,6 @@ if MYPY:
     from typing import Dict
     from typing import List
     from typing import Tuple
-    from typing import Type
-
-    from sentry_sdk._types import SpanLike
 
 _traceparent_header_format_re = re.compile(
     "^[ \t]*"  # whitespace
@@ -225,7 +222,7 @@ class Span(object):
 
     @classmethod
     def continue_from_environ(
-        cls,  # type: Type[SpanLike]
+        cls,
         environ,  # type: typing.Mapping[str, str]
         **kwargs  # type: Any
     ):
@@ -239,7 +236,7 @@ class Span(object):
 
     @classmethod
     def continue_from_headers(
-        cls,  # type: Type[SpanLike]
+        cls,
         headers,  # type: typing.Mapping[str, str]
         **kwargs  # type: Any
     ):
@@ -261,7 +258,7 @@ class Span(object):
 
     @classmethod
     def from_traceparent(
-        cls,  # type: Type[SpanLike]
+        cls,
         traceparent,  # type: Optional[str]
         **kwargs  # type: Any
     ):

This aligns the tracing implementation with the current JS tracing implementation, up to a certain extent. Hub.start_transaction or start_transaction are meant to be used when starting transactions, replacing most uses of Hub.start_span / start_span. Spans are typically created from their parent transactions via transaction.start_child, or start_span relying on the transaction being in the current scope. It is okay to start a transaction without a name and set it later. Sometimes the proper name is not known until after the transaction has started. We could fail the transaction if it has no name when calling the finish method. Instead, set a default name that will prompt users to give a name to their transactions. This is the same behavior as implemented in JS. Span.continue_from_headers, Span.continue_from_environ, Span.from_traceparent and the equivalent methods on Transaction always return a Transaction and take kwargs to set attributes on the new Transaction. Rename Span.new_span to Span.start_child (and Transaction.start_child)a, aligning with JS / tracing API spec. The old name is kept for backwards compatibility. Co-authored-by: Markus Unterwaditzer <markus@unterwaditzer.net>

rhcarvalho · 2020-06-29T15:06:23Z

Removed some commentary, will move the discussion into new issues where appropriate.

rhcarvalho mentioned this pull request Jun 26, 2020

ref(am): Introduce start_transaction #715

Closed

rhcarvalho force-pushed the rhcarvalho/tracing-transaction branch from c877854 to ecafc4a Compare June 27, 2020 12:20

rhcarvalho marked this pull request as ready for review June 27, 2020 13:14

rhcarvalho requested a review from untitaker June 27, 2020 13:15

AbhiPrasad reviewed Jun 29, 2020

View reviewed changes

untitaker approved these changes Jun 29, 2020

View reviewed changes

rhcarvalho force-pushed the rhcarvalho/tracing-transaction branch from 7c94965 to 9389f49 Compare June 29, 2020 15:05

rhcarvalho merged commit ab3da08 into master Jun 29, 2020

rhcarvalho deleted the rhcarvalho/tracing-transaction branch June 29, 2020 15:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Introduce Transaction and Hub.start_transaction #747

feat: Introduce Transaction and Hub.start_transaction #747

rhcarvalho commented Jun 26, 2020 •

edited

rhcarvalho commented Jun 27, 2020

AbhiPrasad left a comment

AbhiPrasad Jun 29, 2020

rhcarvalho Jun 29, 2020

AbhiPrasad Jun 29, 2020

AbhiPrasad Jun 29, 2020

AbhiPrasad Jun 29, 2020 •

edited

untitaker left a comment

untitaker Jun 29, 2020

rhcarvalho Jun 29, 2020

untitaker Jun 29, 2020

rhcarvalho Jun 29, 2020

untitaker Jun 29, 2020

rhcarvalho commented Jun 29, 2020

rhcarvalho commented Jun 29, 2020

feat: Introduce Transaction and Hub.start_transaction #747

feat: Introduce Transaction and Hub.start_transaction #747

Conversation

rhcarvalho commented Jun 26, 2020 • edited

rhcarvalho commented Jun 27, 2020

AbhiPrasad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AbhiPrasad Jun 29, 2020 • edited

Choose a reason for hiding this comment

untitaker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhcarvalho commented Jun 29, 2020

rhcarvalho commented Jun 29, 2020

rhcarvalho commented Jun 26, 2020 •

edited

AbhiPrasad Jun 29, 2020 •

edited