ref(am): Introduce start_transaction #715
Conversation
There was a problem hiding this comment.
Thanks for doing it @untitaker. I have a few notes if they can be of any use for improvement. Generally this is looking good.
| @@ -462,17 +461,56 @@ def start_span( | |||
| else: | |||
| span = Span(**kwargs) | |||
|
|
|||
| if span.sampled is None and span.transaction is not None: | |||
| elif isinstance(span, Transaction): | |||
| raise ValueError("Pass transactions to start_transaction instead") | |||
There was a problem hiding this comment.
In JS there is a fallback that calls startTransaction in this case, to ease transition of existing code.
| with pytest.raises(TypeError): | ||
| start_span(name="foo") |
There was a problem hiding this comment.
Does this mean a runtime error for user code? Should we be more lenient?
There was a problem hiding this comment.
You always get runtime errors when calling a function with the wrong args. If we want to do a transition path here I think we need to accept transaction as kwarg. Is that what we want?
There was a problem hiding this comment.
IMO yes, I'd avoid breaking beta users during the upgrade.
Include a clear note of the API change in the change log / release notes, but support the old code path with a warning (message when debug=True) for at least one more release.
| if span and isinstance(span, Transaction) and value: | ||
| span.name = value |
There was a problem hiding this comment.
Would it make sense to store the whole transaction in self._transaction?
Code reading it that expects a string (e.g. to set Event.transaction would read from scope._transaction.name, and code that needs the transaction would read from scope._transaction, removing the need for a scope._span that contains an instance of Transaction.
There was a problem hiding this comment.
The current state at JS is that we have a single slot for a "span" that can be a transaction or a span. Let's leave this for later.
| # type: (**Any) -> Span | ||
| """ | ||
| Start a sub-span from the current span or transaction. |
There was a problem hiding this comment.
This feels wrong, parent classes (Span) normally do not refer to their subclasses (Transaction).
This would be easy to fix by omitting the "from the current ...", as the context makes it rather clear who is the parent of the new span that is being started.
| Start a sub-span from the current span or transaction. | |
| Start a child span. |
Co-authored-by: Rodolfo Carvalho <rodolfo.carvalho@sentry.io>
| # 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 sentry_sdk.Hub.start_span(..)` on a | ||
| # non-transaction span and later decided to make it a transaction. | ||
| if self.sampled is None: | ||
| logger.warning("Discarding transaction Span without sampling decision") |
There was a problem hiding this comment.
| # 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 sentry_sdk.Hub.start_span(..)` on a | |
| # non-transaction span and later decided to make it a transaction. | |
| if self.sampled is None: | |
| logger.warning("Discarding transaction Span without sampling decision") | |
| # At this point a `sampled = None` should have already been | |
| # resolved to a concrete decision. If `sampled` is `None`, it's | |
| # likely that somebody created a transaction without setting the sampling decision. | |
| # When using `sentry_sdk.Hub.start_transaction(..)` the sampling decision will always be set. | |
| if self.sampled is None: | |
| logger.warning("Discarding transaction without sampling decision") |
| @@ -194,25 +189,45 @@ def __exit__(self, ty, value, tb): | |||
| self.finish(hub) | |||
| scope.span = old_span | |||
|
|
|||
| def new_span(self, **kwargs): | |||
| def start_child(self, **kwargs): | |||
There was a problem hiding this comment.
Isn't this a breaking change for manual instrumentation?
In JS we made sure it's not breaking even if old API is used.
Hub.current is never None. Fixing other occurrences is left for a separate commit.
Not possible to start a transaction from a span, let's not confuse the API more.
In favor of replicating start_span's signature.
Example usages:
Hub.start_transaction(transaction)
or
Hub.start_transaction(name="...", ...)
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.
Asserting that each start_transaction context contributes with one and only one new event makes the overall test more robust -- though far from perfect, e.g. we make no assertions about the event itself.
|
Closing in favor of #747. |
Refactor the API to be similar to getsentry/sentry-javascript#2600