Skip to content

ref(am): Introduce start_transaction #715

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 16 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion sentry_sdk/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
from typing import Union

from sentry_sdk._types import Event, Hint, Breadcrumb, BreadcrumbHint, ExcInfo
from sentry_sdk.tracing import Span
from sentry_sdk.tracing import Span, Transaction

T = TypeVar("T")
F = TypeVar("F", bound=Callable[..., Any])
Expand All @@ -37,6 +37,7 @@ def overload(x):
"flush",
"last_event_id",
"start_span",
"start_transaction",
"set_tag",
"set_context",
"set_extra",
Expand Down Expand Up @@ -201,3 +202,12 @@ def start_span(
):
# type: (...) -> Span
return Hub.current.start_span(span=span, **kwargs)


@hubmethod
def start_transaction(
transaction=None, # type: Optional[Transaction]
**kwargs # type: Any
):
# type: (...) -> Transaction
return Hub.current.start_transaction(transaction, **kwargs)
65 changes: 55 additions & 10 deletions sentry_sdk/hub.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,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
from sentry_sdk.tracing import Span, Transaction
from sentry_sdk.sessions import Session
from sentry_sdk.utils import (
exc_info_from_error,
Expand Down Expand Up @@ -445,10 +445,9 @@ def start_span(
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.
Note that you will not see any span that is not contained within a
transaction. Most integrations already do this for you, but create a
transaction with `start_transaction` otherwise.
"""

client, scope = self._stack[-1]
Expand All @@ -462,17 +461,63 @@ 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")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In JS there is a fallback that calls startTransaction in this case, to ease transition of existing code.

https://github.com/getsentry/sentry-javascript/blob/d66e2d7f76492b2e403f64b047ffaf7bdddabb0a/packages/apm/src/hubextensions.ts#L52-L71


return span

def start_transaction(
self,
transaction=None, # type: Optional[Transaction]
**kwargs # type: Any
):
# type: (...) -> Transaction
"""
Start and return a transaction.

Start an existing transaction if given, otherwise create and start a new
transaction with kwargs.
"""
# 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?

# XXX: it is strange that both start_span and start_transaction take
# kwargs, but those are ignored if span/transaction are not None.
# Code such as:
# hub.start_transaction(Transaction(name="foo"), name="bar")
#
# is clearly weird, but not so weird if we intentionally want to rename
# a transaction we got from somewhere else:
# hub.start_transaction(transaction, name="new_name")
#
# Would be clearer if Hub was not involved:
# transaction.name = "new_name"
# with transaction: # __enter__ => start, __exit__ => finish
# with transaction.start_child(...):
# pass
# # alternatively, rely on transaction being in the current scope
# with Span(...):
# pass

if transaction is None:
kwargs.setdefault("hub", self)
transaction = Transaction(**kwargs)

client, scope = self._stack[-1]

if transaction.sampled is None:
sample_rate = client and client.options["traces_sample_rate"] or 0
span.sampled = random.random() < sample_rate
transaction.sampled = random.random() < sample_rate

if span.sampled:
if transaction.sampled:
max_spans = (
client and client.options["_experiments"].get("max_spans") or 1000
)
span.init_finished_spans(maxlen=max_spans)
transaction.init_span_recorder(maxlen=max_spans)

return span
return transaction

@overload # noqa
def push_scope(
Expand Down
22 changes: 12 additions & 10 deletions sentry_sdk/integrations/aiohttp.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
_filter_headers,
request_body_within_bounds,
)
from sentry_sdk.tracing import Span
from sentry_sdk.tracing import Transaction
from sentry_sdk.utils import (
capture_internal_exceptions,
event_from_exception,
Expand Down Expand Up @@ -87,27 +87,29 @@ async def sentry_app_handle(self, request, *args, **kwargs):
scope.clear_breadcrumbs()
scope.add_event_processor(_make_request_processor(weak_request))

span = Span.continue_from_headers(request.headers)
span.op = "http.server"
# If this transaction name makes it to the UI, AIOHTTP's
# URL resolver did not find a route or died trying.
span.transaction = "generic AIOHTTP request"
transaction = Transaction.continue_from_headers(
request.headers,
op="http.server",
# If this transaction name makes it to the UI, AIOHTTP's
# URL resolver did not find a route or died trying.
name="generic AIOHTTP request",
)

with hub.start_span(span):
with hub.start_transaction(transaction):
try:
response = await old_handle(self, request)
except HTTPException as e:
span.set_http_status(e.status_code)
transaction.set_http_status(e.status_code)
raise
except asyncio.CancelledError:
span.set_status("cancelled")
transaction.set_status("cancelled")
raise
except Exception:
# This will probably map to a 500 but seems like we
# have no way to tell. Do not set span status.
reraise(*_capture_exception(hub))

span.set_http_status(response.status)
transaction.set_http_status(response.status)
return response

Application._handle = sentry_app_handle
Expand Down
19 changes: 11 additions & 8 deletions sentry_sdk/integrations/asgi.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
HAS_REAL_CONTEXTVARS,
CONTEXTVARS_ERROR_MESSAGE,
)
from sentry_sdk.tracing import Span
from sentry_sdk.tracing import Transaction

if MYPY:
from typing import Dict
Expand Down Expand Up @@ -123,16 +123,19 @@ async def _run_app(self, scope, callback):
ty = scope["type"]

if ty in ("http", "websocket"):
span = Span.continue_from_headers(dict(scope["headers"]))
span.op = "{}.server".format(ty)
transaction = Transaction.continue_from_headers(
dict(scope["headers"]),
name=_DEFAULT_TRANSACTION_NAME,
op="{}.server".format(ty),
)
else:
span = Span()
span.op = "asgi.server"
transaction = Transaction(
name=_DEFAULT_TRANSACTION_NAME, op="asgi.server"
)

span.set_tag("asgi.type", ty)
span.transaction = _DEFAULT_TRANSACTION_NAME
transaction.set_tag("asgi.type", ty)

with hub.start_span(span) as span:
with hub.start_transaction(transaction):
# XXX: Would be cool to have correct span status, but we
# would have to wrap send(). That is a bit hard to do with
# the current abstraction over ASGI 2/3.
Expand Down
16 changes: 9 additions & 7 deletions sentry_sdk/integrations/celery.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

from sentry_sdk.hub import Hub
from sentry_sdk.utils import capture_internal_exceptions, event_from_exception
from sentry_sdk.tracing import Span
from sentry_sdk.tracing import Transaction
from sentry_sdk._compat import reraise
from sentry_sdk.integrations import Integration, DidNotEnable
from sentry_sdk.integrations.logging import ignore_logger
Expand Down Expand Up @@ -130,19 +130,21 @@ def _inner(*args, **kwargs):
scope.clear_breadcrumbs()
scope.add_event_processor(_make_event_processor(task, *args, **kwargs))

span = Span.continue_from_headers(args[3].get("headers") or {})
span.op = "celery.task"
span.transaction = "unknown celery task"
transaction = Transaction.continue_from_headers(
args[3].get("headers") or {},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see this was already there. What is args[3]? Would it make sense to give it a proper name now that you're touching it?

op="celery.task",
name="unknown celery task",
)

# Could possibly use a better hook than this one
span.set_status("ok")
transaction.set_status("ok")

with capture_internal_exceptions():
# Celery task objects are not a thing to be trusted. Even
# something such as attribute access can fail.
span.transaction = task.name
transaction.name = task.name

with hub.start_span(span):
with hub.start_transaction(transaction):
return f(*args, **kwargs)

return _inner # type: ignore
Expand Down
13 changes: 7 additions & 6 deletions sentry_sdk/integrations/rq.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

from sentry_sdk.hub import Hub
from sentry_sdk.integrations import Integration, DidNotEnable
from sentry_sdk.tracing import Span
from sentry_sdk.tracing import Transaction
from sentry_sdk.utils import capture_internal_exceptions, event_from_exception


Expand Down Expand Up @@ -61,15 +61,16 @@ def sentry_patched_perform_job(self, job, *args, **kwargs):
scope.clear_breadcrumbs()
scope.add_event_processor(_make_event_processor(weakref.ref(job)))

span = Span.continue_from_headers(
job.meta.get("_sentry_trace_headers") or {}
transaction = Transaction.continue_from_headers(
job.meta.get("_sentry_trace_headers") or {},
op="rq.task",
name="unknown RQ task",
)
span.op = "rq.task"

with capture_internal_exceptions():
span.transaction = job.func_name
transaction.name = job.func_name

with hub.start_span(span):
with hub.start_transaction(transaction):
rv = old_perform_job(self, job, *args, **kwargs)

if self.is_horse:
Expand Down
16 changes: 9 additions & 7 deletions sentry_sdk/integrations/wsgi.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
event_from_exception,
)
from sentry_sdk._compat import PY2, reraise, iteritems
from sentry_sdk.tracing import Span
from sentry_sdk.tracing import Transaction
from sentry_sdk.sessions import auto_session_tracking
from sentry_sdk.integrations._wsgi_common import _filter_headers

Expand Down Expand Up @@ -113,15 +113,17 @@ def __call__(self, environ, start_response):
_make_wsgi_event_processor(environ)
)

span = Span.continue_from_environ(environ)
span.op = "http.server"
span.transaction = "generic WSGI request"
transaction = Transaction.continue_from_environ(
environ, op="http.server", name="generic WSGI request"
)

with hub.start_span(span) as span:
with hub.start_transaction(transaction) as transaction:
try:
rv = self.app(
environ,
partial(_sentry_start_response, start_response, span),
partial(
_sentry_start_response, start_response, transaction
),
)
except BaseException:
reraise(*_capture_exception(hub))
Expand All @@ -133,7 +135,7 @@ def __call__(self, environ, start_response):

def _sentry_start_response(
old_start_response, # type: StartResponse
span, # type: Span
span, # type: Transaction
status, # type: str
response_headers, # type: WsgiResponseHeaders
exc_info=None, # type: Optional[WsgiExcInfo]
Expand Down
9 changes: 5 additions & 4 deletions sentry_sdk/scope.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
from sentry_sdk._functools import wraps
from sentry_sdk._types import MYPY
from sentry_sdk.utils import logger, capture_internal_exceptions
from sentry_sdk.tracing import Transaction

if MYPY:
from typing import Any
Expand Down Expand Up @@ -140,8 +141,8 @@ def transaction(self, value):
"""When set this forces a specific transaction name to be set."""
self._transaction = value
span = self._span
if span:
span.transaction = value
if span and isinstance(span, Transaction) and value:
span.name = value
Comment on lines +144 to +145
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


@_attr_setter
def user(self, value):
Expand All @@ -166,8 +167,8 @@ def span(self):
def span(self, span):
# type: (Optional[Span]) -> None
self._span = span
if span is not None:
span_transaction = span.transaction
if isinstance(span, Transaction):
span_transaction = span.name
if span_transaction:
self._transaction = span_transaction

Expand Down
Loading