ref(tracing): Pre-traces_sampler documentation additions (getsentry#865)

lobsterkatie · web-flow · commit e873bdb07114 · 2020-10-16T09:29:43.000-07:00
Comments and docstrings, expanded __repr__s for Span and Transaction, a few variable name changes. No behavior change.
diff --git 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
@@ -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
@@ -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
@@ -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.
@@ -166,8 +171,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,
@@ -200,8 +207,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)
 
@@ -227,6 +235,14 @@ def continue_from_environ(
         **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
+        Transaction.
+
+        If the 'sentry-trace' header is malformed or missing, just create and
+        return a Transaction instance with the given params.
+        """
         if cls is Span:
             logger.warning(
                 "Deprecated: use Transaction.continue_from_environ "
@@ -241,16 +257,25 @@ def continue_from_headers(
         **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.
+
+        If the 'sentry-trace' header is malformed or missing, just create and
+        return a Transaction instance with the given params.
+        """
         if cls is Span:
             logger.warning(
                 "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]
@@ -263,6 +288,13 @@ def from_traceparent(
         **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.
+
+        If the header value is malformed or missing, just create and return a
+        Transaction instance with the given params.
+        """
         if cls is Span:
             logger.warning(
                 "Deprecated: use Transaction.from_traceparent "
@@ -279,20 +311,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):
@@ -436,16 +471,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):
@@ -454,7 +487,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
@@ -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
@@ -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
@@ -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
@@ -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"
