ref(tracing): Various tweaks to tracestate header handling

sentry_sdk/client.py

@@ -346,7 +346,7 @@ def capture_event(
                 "sent_at": format_timestamp(datetime.utcnow()),
             }
 
-            tracestate_data = reinflate_tracestate(
+            tracestate_data = raw_tracestate and reinflate_tracestate(
                 raw_tracestate.replace("sentry=", "")
             )
             if tracestate_data:

sentry_sdk/scope.py

@@ -150,19 +150,13 @@ def transaction(self):
         if self._span is None:
             return None
 
-        # the span on the scope is itself a transaction
-        if isinstance(self._span, Transaction):
-            return self._span
-
-        # the span on the scope isn't a transaction but belongs to one
-        if self._span._containing_transaction:
-            return self._span._containing_transaction
-
-        # there's a span (not a transaction) on the scope, but it was started on
-        # its own, not as the descendant of a transaction (this is deprecated
-        # behavior, but as long as the start_span function exists, it can still
-        # happen)
-        return None
+        # there is an orphan span on the scope
+        if self._span.containing_transaction is None:
+            return None
+
+        # there is either a transaction (which is its own containing
+        # transaction) or a non-orphan span on the scope
+        return self._span.containing_transaction
 
     @transaction.setter
     def transaction(self, value):

sentry_sdk/tracing.py

@@ -73,8 +73,6 @@ class Span(object):
         "_span_recorder",
         "hub",
         "_context_manager_state",
-        # TODO: rename this "transaction" once we fully and truly deprecate the
-        # old "transaction" attribute (which was actually the transaction name)?
         "_containing_transaction",
     )
 
@@ -104,6 +102,7 @@ def __init__(
         hub=None,  # type: Optional[sentry_sdk.Hub]
         status=None,  # type: Optional[str]
         transaction=None,  # type: Optional[str] # deprecated
+        containing_transaction=None,  # type: Optional[Transaction]
     ):
         # type: (...) -> None
         self.trace_id = trace_id or uuid.uuid4().hex
@@ -117,6 +116,7 @@ def __init__(
         self.hub = hub
         self._tags = {}  # type: Dict[str, str]
         self._data = {}  # type: Dict[str, Any]
+        self._containing_transaction = containing_transaction
         self.start_timestamp = datetime.utcnow()
         try:
             # TODO: For Python 3.7+, we could use a clock with ns resolution:
@@ -131,7 +131,6 @@ def __init__(
         self.timestamp = None  # type: Optional[datetime]
 
         self._span_recorder = None  # type: Optional[_SpanRecorder]
-        self._containing_transaction = None  # type: Optional[Transaction]
 
     def init_span_recorder(self, maxlen):
         # type: (int) -> None
@@ -172,6 +171,15 @@ def __exit__(self, ty, value, tb):
         self.finish(hub)
         scope.span = old_span
 
+    @property
+    def containing_transaction(self):
+        # type: () -> Optional[Transaction]
+
+        # this is a getter rather than a regular attribute so that transactions
+        # can return `self` here instead (as a way to prevent them circularly
+        # referencing themselves)
+        return self._containing_transaction
+
     def start_child(self, **kwargs):
         # type: (**Any) -> Span
         """
@@ -184,14 +192,12 @@ def start_child(self, **kwargs):
         kwargs.setdefault("sampled", self.sampled)
 
         child = Span(
-            trace_id=self.trace_id, span_id=None, parent_span_id=self.span_id, **kwargs
+            trace_id=self.trace_id,
+            parent_span_id=self.span_id,
+            containing_transaction=self.containing_transaction,
+            **kwargs
         )
 
-        if isinstance(self, Transaction):
-            child._containing_transaction = self
-        else:
-            child._containing_transaction = self._containing_transaction
-
         child._span_recorder = recorder = self._span_recorder
         if recorder:
             recorder.add(child)
@@ -257,6 +263,10 @@ def iter_headers(self):
         """
         Creates a generator which returns the span's `sentry-trace` and
         `tracestate` headers.
+
+        If the span's containing transaction doesn't yet have a
+        `sentry_tracestate` value, this will cause one to be generated and
+        stored.
         """
         yield "sentry-trace", self.to_traceparent()
 
@@ -304,22 +314,24 @@ def to_tracestate(self):
         Computes the `tracestate` header value using data from the containing
         transaction.
 
+        If the containing transaction doesn't yet have a `sentry_tracestate`
+        value, this will cause one to be generated and stored.
+
+        If there is no containing transaction, a value will be generated but not
+        stored.
+
         Returns None if there's no client and/or no DSN.
         """
 
-        if isinstance(self, Transaction):
-            transaction = self  # type: Optional[Transaction]
-        else:
-            transaction = self._containing_transaction
+        sentry_tracestate = self.get_or_set_sentry_tracestate()
+        third_party_tracestate = (
+            self.containing_transaction._third_party_tracestate
+            if self.containing_transaction
+            else None
+        )
 
-        # we should have the relevant values stored on the transaction, but if
-        # this is an orphan span, make a new value
-        if transaction:
-            sentry_tracestate = transaction._sentry_tracestate
-            third_party_tracestate = transaction._third_party_tracestate
-        else:
-            sentry_tracestate = compute_tracestate_entry(self)
-            third_party_tracestate = None
+        if not sentry_tracestate:
+            return None
 
         header_value = sentry_tracestate
 
@@ -328,6 +340,25 @@ def to_tracestate(self):
 
         return header_value
 
+    def get_or_set_sentry_tracestate(self):
+        # type: (Span) -> Optional[str]
+        """
+        Read sentry tracestate off of the span's containing transaction.
+
+        If the transaction doesn't yet have a `_sentry_tracestate` value,
+        compute one and store it.
+        """
+        transaction = self.containing_transaction
+
+        if transaction:
+            if not transaction._sentry_tracestate:
+                transaction._sentry_tracestate = compute_tracestate_entry(self)
+
+            return transaction._sentry_tracestate
+
+        # orphan span - nowhere to store the value, so just return it
+        return compute_tracestate_entry(self)
+
     def set_tag(self, key, value):
         # type: (str, Any) -> None
         self._tags[key] = value
@@ -434,13 +465,14 @@ def get_trace_context(self):
         if self.status:
             rv["status"] = self.status
 
-        if isinstance(self, Transaction):
-            transaction = self  # type: Optional[Transaction]
-        else:
-            transaction = self._containing_transaction
+        # if the transaction didn't inherit a tracestate value, and no outgoing
+        # requests - whose need for headers would have caused a tracestate value
+        # to be created - were made as part of the transaction, the transaction
+        # still won't have a tracestate value, so compute one now
+        sentry_tracestate = self.get_or_set_sentry_tracestate()
 
-        if transaction:
-            rv["tracestate"] = transaction._sentry_tracestate
+        if sentry_tracestate:
+            rv["tracestate"] = sentry_tracestate
 
         return rv
 
@@ -479,7 +511,10 @@ def __init__(
         Span.__init__(self, **kwargs)
         self.name = name
         self.parent_sampled = parent_sampled
-        self._sentry_tracestate = sentry_tracestate or compute_tracestate_entry(self)
+        # if tracestate isn't inherited and set here, it will get set lazily,
+        # either the first time an outgoing request needs it for a header or the
+        # first time an event needs it for inclusion in the captured data
+        self._sentry_tracestate = sentry_tracestate
         self._third_party_tracestate = third_party_tracestate
 
     def __repr__(self):
@@ -494,6 +529,15 @@ def __repr__(self):
             self.sampled,
         )
 
+    @property
+    def containing_transaction(self):
+        # type: () -> Transaction
+
+        # Transactions (as spans) belong to themselves (as transactions). This
+        # is a getter rather than a regular attribute to avoid having a circular
+        # reference.
+        return self
+
     def finish(self, hub=None):
         # type: (Optional[sentry_sdk.Hub]) -> Optional[str]
         if self.timestamp is not None:

sentry_sdk/tracing_utils.py

@@ -301,7 +301,7 @@ def compute_tracestate_value(data):
 
 
 def compute_tracestate_entry(span):
-    # type: (Span) -> str
+    # type: (Span) -> Optional[str]
     """
     Computes a new sentry tracestate for the span. Includes the `sentry=`.
 
@@ -311,8 +311,6 @@ def compute_tracestate_entry(span):
 
     client = (span.hub or sentry_sdk.Hub.current).client
 
-    # if there's no client and/or no DSN, we're not sending anything anywhere,
-    # so it's fine to not have any tracestate data
     if client and client.options.get("dsn"):
         options = client.options
         data = {
@@ -322,7 +320,9 @@ def compute_tracestate_entry(span):
             "public_key": Dsn(options["dsn"]).public_key,
         }
 
-    return "sentry=" + compute_tracestate_value(data)
+        return "sentry=" + compute_tracestate_value(data)
+
+    return None
 
 
 def reinflate_tracestate(encoded_tracestate):

tests/tracing/test_http_headers.py

@@ -2,6 +2,7 @@
 
 import pytest
 
+import sentry_sdk
 from sentry_sdk.tracing import Transaction, Span
 from sentry_sdk.tracing_utils import (
     compute_tracestate_value,
@@ -31,6 +32,9 @@ def test_tracestate_computation(sentry_init):
         trace_id="12312012123120121231201212312012",
     )
 
+    # force lazy computation to create a value
+    transaction.to_tracestate()
+
     computed_value = transaction._sentry_tracestate.replace("sentry=", "")
     # we have to decode and reinflate the data because we can guarantee that the
     # order of the entries in the jsonified dict will be the same here as when
@@ -45,7 +49,7 @@ def test_tracestate_computation(sentry_init):
     }
 
 
-def test_adds_new_tracestate_to_transaction_when_none_given(sentry_init):
+def test_doesnt_add_new_tracestate_to_transaction_when_none_given(sentry_init):
     sentry_init(
         dsn="https://dogsarebadatkeepingsecrets@squirrelchasers.ingest.sentry.io/12312012",
         environment="dogpark",
@@ -58,9 +62,92 @@ def test_adds_new_tracestate_to_transaction_when_none_given(sentry_init):
         # sentry_tracestate=< value would be passed here >
     )
 
+    assert transaction._sentry_tracestate is None
+
+
+def test_adds_tracestate_to_transaction_when_to_traceparent_called(sentry_init):
+    sentry_init(
+        dsn="https://dogsarebadatkeepingsecrets@squirrelchasers.ingest.sentry.io/12312012",
+        environment="dogpark",
+        release="off.leash.park",
+    )
+
+    transaction = Transaction(
+        name="/interactions/other-dogs/new-dog",
+        op="greeting.sniff",
+    )
+
+    # no inherited tracestate, and none created in Transaction constructor
+    assert transaction._sentry_tracestate is None
+
+    transaction.to_tracestate()
+
     assert transaction._sentry_tracestate is not None
 
 
+def test_adds_tracestate_to_transaction_when_getting_trace_context(sentry_init):
+    sentry_init(
+        dsn="https://dogsarebadatkeepingsecrets@squirrelchasers.ingest.sentry.io/12312012",
+        environment="dogpark",
+        release="off.leash.park",
+    )
+
+    transaction = Transaction(
+        name="/interactions/other-dogs/new-dog",
+        op="greeting.sniff",
+    )
+
+    # no inherited tracestate, and none created in Transaction constructor
+    assert transaction._sentry_tracestate is None
+
+    transaction.get_trace_context()
+
+    assert transaction._sentry_tracestate is not None
+
+
+@pytest.mark.parametrize(
+    "set_by", ["inheritance", "to_tracestate", "get_trace_context"]
+)
+def test_tracestate_is_immutable_once_set(sentry_init, monkeypatch, set_by):
+    monkeypatch.setattr(
+        sentry_sdk.tracing,
+        "compute_tracestate_entry",
+        mock.Mock(return_value="sentry=doGsaREgReaT"),
+    )
+
+    sentry_init(
+        dsn="https://dogsarebadatkeepingsecrets@squirrelchasers.ingest.sentry.io/12312012",
+        environment="dogpark",
+        release="off.leash.park",
+    )
+
+    # for each scenario, get to the point where tracestate has been set
+    if set_by == "inheritance":
+        transaction = Transaction(
+            name="/interactions/other-dogs/new-dog",
+            op="greeting.sniff",
+            sentry_tracestate=("sentry=doGsaREgReaT"),
+        )
+    else:
+        transaction = Transaction(
+            name="/interactions/other-dogs/new-dog",
+            op="greeting.sniff",
+        )
+
+        if set_by == "to_tracestate":
+            transaction.to_tracestate()
+        if set_by == "get_trace_context":
+            transaction.get_trace_context()
+
+    assert transaction._sentry_tracestate == "sentry=doGsaREgReaT"
+
+    # user data would be included in tracestate if it were recomputed at this point
+    sentry_sdk.set_user({"id": 12312013, "segment": "bigs"})
+
+    # value hasn't changed
+    assert transaction._sentry_tracestate == "sentry=doGsaREgReaT"
+
+
 @pytest.mark.parametrize("sampled", [True, False, None])
 def test_to_traceparent(sentry_init, sampled):