From 4ecac6fe038b87fb5739b63f260549b5b9a77192 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Thu, 15 Oct 2020 13:01:11 -0700 Subject: [PATCH 1/4] add docstrings and comments --- pytest.ini | 2 +- sentry_sdk/integrations/flask.py | 3 ++- sentry_sdk/scope.py | 2 ++ sentry_sdk/tracing.py | 29 +++++++++++++++++++++-- tests/conftest.py | 2 ++ tests/integrations/stdlib/test_httplib.py | 4 ++++ tests/tracing/test_integration_tests.py | 7 ++++++ tests/tracing/test_misc.py | 6 +++++ 8 files changed, 51 insertions(+), 4 deletions(-) diff --git a/pytest.ini b/pytest.ini index 4e440e2a47..c00b03296c 100644 --- a/pytest.ini +++ b/pytest.ini @@ -2,5 +2,5 @@ DJANGO_SETTINGS_MODULE = tests.integrations.django.myapp.settings addopts = --tb=short markers = - tests_internal_exceptions + tests_internal_exceptions: Handle internal exceptions just as the SDK does, to test it. (Otherwise internal exceptions are recorded and reraised.) only: A temporary marker, to make pytest only run the tests with the mark, similar to jest's `it.only`. To use, run `pytest -v -m only`. diff --git a/sentry_sdk/integrations/flask.py b/sentry_sdk/integrations/flask.py index f6306e5a41..fe630ea50a 100644 --- a/sentry_sdk/integrations/flask.py +++ b/sentry_sdk/integrations/flask.py @@ -104,7 +104,8 @@ def _request_started(sender, **kwargs): with hub.configure_scope() as scope: request = _request_ctx_stack.top.request - # Rely on WSGI middleware to start a trace + # Set the transaction name here, but rely on WSGI middleware to actually + # start the transaction try: if integration.transaction_style == "endpoint": scope.transaction = request.url_rule.endpoint diff --git a/sentry_sdk/scope.py b/sentry_sdk/scope.py index 30bf014068..bc3df8b97b 100644 --- a/sentry_sdk/scope.py +++ b/sentry_sdk/scope.py @@ -77,6 +77,8 @@ class Scope(object): "_level", "_name", "_fingerprint", + # note that for legacy reasons, _transaction is the transaction *name*, + # not a Transaction object (the object is stored in _span) "_transaction", "_user", "_tags", diff --git a/sentry_sdk/tracing.py b/sentry_sdk/tracing.py index 3028284ac3..90353bead6 100644 --- a/sentry_sdk/tracing.py +++ b/sentry_sdk/tracing.py @@ -200,8 +200,9 @@ def start_child(self, **kwargs): """ Start a sub-span from the current span or transaction. - Takes the same arguments as the initializer of :py:class:`Span`. No - attributes other than the sample rate are inherited. + Takes the same arguments as the initializer of :py:class:`Span`. The + trace id, sampling decision, and span recorder are inherited from the + current span/transaction. """ kwargs.setdefault("sampled", self.sampled) @@ -226,6 +227,14 @@ def continue_from_environ( environ, # type: typing.Mapping[str, str] **kwargs # type: Any ): + """ + Create a Transaction with the given params, then add in data pulled from + the 'sentry-trace' header in the environ (if any) before returning the + Transaction. + + If the 'sentry-trace' header is malformed or missing, just create and + return a Transaction instance with the given params. + """ # type: (...) -> Transaction if cls is Span: logger.warning( @@ -240,6 +249,13 @@ def continue_from_headers( headers, # type: typing.Mapping[str, str] **kwargs # type: Any ): + """ + Create a Transaction with the given params, then add in data pulled from + the 'sentry-trace' header (if any) before returning the Transaction. + + If the 'sentry-trace' header is malformed or missing, just create and + return a Transaction instance with the given params. + """ # type: (...) -> Transaction if cls is Span: logger.warning( @@ -262,6 +278,13 @@ def from_traceparent( traceparent, # type: Optional[str] **kwargs # type: Any ): + """ + Create a Transaction with the given params, then add in data pulled from + the given 'sentry-trace' header value before returning the Transaction. + + If the header value is malformed or missing, just create and return a + Transaction instance with the given params. + """ # type: (...) -> Optional[Transaction] if cls is Span: logger.warning( @@ -454,7 +477,9 @@ def finish(self, hub=None): # This transaction is already finished, ignore. return None + # This is a de facto proxy for checking if sampled = False if self._span_recorder is None: + logger.debug("Discarding transaction because sampled = False") return None hub = hub or self.hub or sentry_sdk.Hub.current diff --git a/tests/conftest.py b/tests/conftest.py index 1c368a5b14..d5589238b5 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -48,6 +48,8 @@ def _capture_internal_exception(self, exc_info): @request.addfinalizer def _(): + # rerasise the errors so that this just acts as a pass-through (that + # happens to keep track of the errors which pass through it) for e in errors: reraise(*e) diff --git a/tests/integrations/stdlib/test_httplib.py b/tests/integrations/stdlib/test_httplib.py index a8d9a6a458..ed062761bb 100644 --- a/tests/integrations/stdlib/test_httplib.py +++ b/tests/integrations/stdlib/test_httplib.py @@ -4,13 +4,17 @@ import pytest try: + # py3 from urllib.request import urlopen except ImportError: + # py2 from urllib import urlopen try: + # py2 from httplib import HTTPSConnection except ImportError: + # py3 from http.client import HTTPSConnection from sentry_sdk import capture_message diff --git a/tests/tracing/test_integration_tests.py b/tests/tracing/test_integration_tests.py index 7423e4bd1e..3f5025e41f 100644 --- a/tests/tracing/test_integration_tests.py +++ b/tests/tracing/test_integration_tests.py @@ -51,11 +51,13 @@ def test_continue_from_headers(sentry_init, capture_events, sampled): sentry_init(traces_sample_rate=1.0) events = capture_events() + # make a parent transaction (normally this would be in a different service) with start_transaction(name="hi"): with start_span() as old_span: old_span.sampled = sampled headers = dict(Hub.current.iter_trace_propagation_headers()) + # test that the sampling decision is getting encoded in the header correctly header = headers["sentry-trace"] if sampled is True: assert header.endswith("-1") @@ -64,6 +66,8 @@ def test_continue_from_headers(sentry_init, capture_events, sampled): if sampled is None: assert header.endswith("-") + # child transaction, to prove that we can read 'sentry-trace' header data + # correctly transaction = Transaction.continue_from_headers(headers, name="WRONG") assert transaction is not None assert transaction.sampled == sampled @@ -72,6 +76,9 @@ def test_continue_from_headers(sentry_init, capture_events, sampled): assert transaction.parent_span_id == old_span.span_id assert transaction.span_id != old_span.span_id + # add child transaction to the scope, to show that the captured message will + # be tagged with the trace id (since it happens while the transaction is + # open) with start_transaction(transaction): with configure_scope() as scope: scope.transaction = "ho" diff --git a/tests/tracing/test_misc.py b/tests/tracing/test_misc.py index ce717437ea..8cb4988f2a 100644 --- a/tests/tracing/test_misc.py +++ b/tests/tracing/test_misc.py @@ -14,6 +14,12 @@ def test_span_trimming(sentry_init, capture_events): pass (event,) = events + + # the transaction is its own first span (which counts for max_spans) but it + # doesn't show up in the span list in the event, so this is 1 less than our + # max_spans value + assert len(event["spans"]) == 2 + span1, span2 = event["spans"] assert span1["op"] == "foo0" assert span2["op"] == "foo1" From 15a60b37d28a5d054733a8758eeb968b0c230591 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Thu, 15 Oct 2020 22:24:50 -0700 Subject: [PATCH 2/4] add more data to reprs --- sentry_sdk/tracing.py | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/sentry_sdk/tracing.py b/sentry_sdk/tracing.py index 90353bead6..e7ddb55cc5 100644 --- a/sentry_sdk/tracing.py +++ b/sentry_sdk/tracing.py @@ -166,8 +166,10 @@ def init_span_recorder(self, maxlen): def __repr__(self): # type: () -> str - return "<%s(trace_id=%r, span_id=%r, parent_span_id=%r, sampled=%r)>" % ( + return "<%s(op=%r, description:%r, trace_id=%r, span_id=%r, parent_span_id=%r, sampled=%r)>" % ( self.__class__.__name__, + self.op, + self.description, self.trace_id, self.span_id, self.parent_span_id, @@ -459,16 +461,14 @@ def __init__( def __repr__(self): # type: () -> str - return ( - "<%s(name=%r, trace_id=%r, span_id=%r, parent_span_id=%r, sampled=%r)>" - % ( - self.__class__.__name__, - self.name, - self.trace_id, - self.span_id, - self.parent_span_id, - self.sampled, - ) + return "<%s(name=%r, op=%r, trace_id=%r, span_id=%r, parent_span_id=%r, sampled=%r)>" % ( + self.__class__.__name__, + self.name, + self.op, + self.trace_id, + self.span_id, + self.parent_span_id, + self.sampled, ) def finish(self, hub=None): From d1f9d2537676d3a5d7d0b2c3a2cf77aac6462138 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Thu, 15 Oct 2020 22:25:23 -0700 Subject: [PATCH 3/4] clarify variable names --- sentry_sdk/tracing.py | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/sentry_sdk/tracing.py b/sentry_sdk/tracing.py index e7ddb55cc5..6d6151ef20 100644 --- a/sentry_sdk/tracing.py +++ b/sentry_sdk/tracing.py @@ -264,11 +264,13 @@ def continue_from_headers( "Deprecated: use Transaction.continue_from_headers " "instead of Span.continue_from_headers." ) - parent = Transaction.from_traceparent(headers.get("sentry-trace"), **kwargs) - if parent is None: - parent = Transaction(**kwargs) - parent.same_process_as_parent = False - return parent + transaction = Transaction.from_traceparent( + headers.get("sentry-trace"), **kwargs + ) + if transaction is None: + transaction = Transaction(**kwargs) + transaction.same_process_as_parent = False + return transaction def iter_headers(self): # type: () -> Generator[Tuple[str, str], None, None] @@ -304,20 +306,23 @@ def from_traceparent( if match is None: return None - trace_id, span_id, sampled_str = match.groups() + trace_id, parent_span_id, sampled_str = match.groups() if trace_id is not None: trace_id = "{:032x}".format(int(trace_id, 16)) - if span_id is not None: - span_id = "{:016x}".format(int(span_id, 16)) + if parent_span_id is not None: + parent_span_id = "{:016x}".format(int(parent_span_id, 16)) if sampled_str: - sampled = sampled_str != "0" # type: Optional[bool] + parent_sampled = sampled_str != "0" # type: Optional[bool] else: - sampled = None + parent_sampled = None return Transaction( - trace_id=trace_id, parent_span_id=span_id, sampled=sampled, **kwargs + trace_id=trace_id, + parent_span_id=parent_span_id, + sampled=parent_sampled, + **kwargs ) def to_traceparent(self): From bf5977e9f394d294a3df977a6f73f1e97fcbea1c Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Fri, 16 Oct 2020 00:59:08 -0700 Subject: [PATCH 4/4] fix linting issues --- sentry_sdk/tracing.py | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/sentry_sdk/tracing.py b/sentry_sdk/tracing.py index 6d6151ef20..af256d583e 100644 --- a/sentry_sdk/tracing.py +++ b/sentry_sdk/tracing.py @@ -111,6 +111,11 @@ class Span(object): def __new__(cls, **kwargs): # type: (**Any) -> Any + """ + Backwards-compatible implementation of Span and Transaction + creation. + """ + # TODO: consider removing this in a future release. # This is for backwards compatibility with releases before Transaction # existed, to allow for a smoother transition. @@ -229,6 +234,7 @@ def continue_from_environ( environ, # type: typing.Mapping[str, str] **kwargs # type: Any ): + # type: (...) -> Transaction """ Create a Transaction with the given params, then add in data pulled from the 'sentry-trace' header in the environ (if any) before returning the @@ -237,7 +243,6 @@ def continue_from_environ( If the 'sentry-trace' header is malformed or missing, just create and return a Transaction instance with the given params. """ - # type: (...) -> Transaction if cls is Span: logger.warning( "Deprecated: use Transaction.continue_from_environ " @@ -251,6 +256,7 @@ def continue_from_headers( headers, # type: typing.Mapping[str, str] **kwargs # type: Any ): + # type: (...) -> Transaction """ Create a Transaction with the given params, then add in data pulled from the 'sentry-trace' header (if any) before returning the Transaction. @@ -258,7 +264,6 @@ def continue_from_headers( If the 'sentry-trace' header is malformed or missing, just create and return a Transaction instance with the given params. """ - # type: (...) -> Transaction if cls is Span: logger.warning( "Deprecated: use Transaction.continue_from_headers " @@ -282,6 +287,7 @@ def from_traceparent( traceparent, # type: Optional[str] **kwargs # type: Any ): + # type: (...) -> Optional[Transaction] """ Create a Transaction with the given params, then add in data pulled from the given 'sentry-trace' header value before returning the Transaction. @@ -289,7 +295,6 @@ def from_traceparent( If the header value is malformed or missing, just create and return a Transaction instance with the given params. """ - # type: (...) -> Optional[Transaction] if cls is Span: logger.warning( "Deprecated: use Transaction.from_traceparent "