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 1 commit
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
15 changes: 14 additions & 1 deletion sentry_sdk/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,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 @@ -38,6 +38,7 @@ def overload(x):
"flush",
"last_event_id",
"start_span",
"start_transaction",
"set_tag",
"set_context",
"set_extra",
Expand Down Expand Up @@ -255,3 +256,15 @@ def start_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)


@hubmethod
def start_transaction(
span_or_name=None, # type: Optional[Union[Span, str]]
**kwargs # type: Any
):
# type: (...) -> Transaction

# TODO: All other functions in this module check for
# `Hub.current is None`. That actually should never happen?
return Hub.current.start_transaction(span_or_name=span_or_name, **kwargs)
58 changes: 48 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,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")
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,
span_or_name=None, # type: Optional[Union[Span, str]]
**kwargs # type: Any
):
# type: (...) -> Transaction
"""
Create a new transaction detached from the current span. The return
value is the `Transaction` object which for the most part works like a
span.
Copy link
Contributor

Choose a reason for hiding this comment

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

a new transaction detached from the current span

Here we say "current span", in start_span we refer to the same thing as "currently active span".
All of those refer to "the span (actually an instance of Transaction) stored in the (current) Scope".

Do we even need to talk about "detached from"? When I call a function my usual expectation is that it is not causing unexpected side effects. It is useful to document side effects, strange to document the lack of side effects, unless we're making a direct comparison with something else.

which for the most part works like a span

I feel this is confusing because it does not explain what the difference is. We were trying to help make this distinction more clear for users. Start transaction returns a transaction. Transactions contain spans. Transactions are sent to Sentry when finished.

If we want to go into more details of what a Transaction is, we should do so in the docstring for Transaction.

Here it would be more relevant to explain when someone would want to use this function. In JS we describe that this is the entry point for doing manual instrumentation.

https://github.com/getsentry/sentry-javascript/blob/d66e2d7f76492b2e403f64b047ffaf7bdddabb0a/packages/minimal/src/index.ts#L179-L195

"""

kwargs.setdefault("hub", self)

if isinstance(span_or_name, str):
if "name" in kwargs:
raise ValueError("Cannot specify transaction name twice.")
kwargs["name"] = span_or_name
transaction = Transaction(**kwargs)

elif span_or_name is None and "name" in kwargs:
transaction = Transaction(**kwargs)

elif isinstance(span_or_name, Transaction):
transaction = span_or_name

elif span_or_name is None and "span" in kwargs:
transaction = kwargs.pop("span")

else:
raise ValueError("transaction object or name required.")

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
16 changes: 9 additions & 7 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 @@ -85,13 +85,15 @@ 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"
span = 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(span):
try:
response = await old_handle(self, request)
except HTTPException as e:
Expand Down
15 changes: 8 additions & 7 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,17 @@ 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)
span = Transaction.continue_from_headers(
dict(scope["headers"]),
name=_DEFAULT_TRANSACTION_NAME,
op="{}.server".format(ty),
)
else:
span = Span()
span.op = "asgi.server"
span = Transaction(name=_DEFAULT_TRANSACTION_NAME, op="asgi.server")

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

with hub.start_span(span) as span:
with hub.start_transaction(span):
# 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
14 changes: 8 additions & 6 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"
span = 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")

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
span.name = task.name

with hub.start_span(span):
with hub.start_transaction(span):
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 {}
span = 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
span.name = job.func_name

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

if self.is_horse:
Expand Down
12 changes: 6 additions & 6 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,11 +113,11 @@ 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"
span = Transaction.continue_from_environ(
environ, op="http.server", name="generic WSGI request"
)

with hub.start_span(span) as span:
with hub.start_transaction(span) as span:
try:
rv = self.app(
environ,
Expand All @@ -133,7 +133,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