From 22acce4f83c39fd57391a92b4454b4a9380103fa Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Tue, 24 Aug 2021 00:30:13 -0700 Subject: [PATCH 1/4] return None for tracestate entry if no client or DSN --- sentry_sdk/client.py | 2 +- sentry_sdk/tracing.py | 3 +++ sentry_sdk/tracing_utils.py | 8 ++++---- 3 files changed, 8 insertions(+), 5 deletions(-) diff --git a/sentry_sdk/client.py b/sentry_sdk/client.py index f4789d5306..6c9eb44939 100644 --- a/sentry_sdk/client.py +++ b/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: diff --git a/sentry_sdk/tracing.py b/sentry_sdk/tracing.py index 7235adbb95..e287130d3f 100644 --- a/sentry_sdk/tracing.py +++ b/sentry_sdk/tracing.py @@ -321,6 +321,9 @@ def to_tracestate(self): sentry_tracestate = compute_tracestate_entry(self) third_party_tracestate = None + if not sentry_tracestate: + return None + header_value = sentry_tracestate if third_party_tracestate: diff --git a/sentry_sdk/tracing_utils.py b/sentry_sdk/tracing_utils.py index d3c53a971e..ec7ad7743f 100644 --- a/sentry_sdk/tracing_utils.py +++ b/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): From 4d739f6dd5ca6e73c8d90b59c070fee30dcd6e6e Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Tue, 24 Aug 2021 00:13:51 -0700 Subject: [PATCH 2/4] let transactions be their own containing transactions --- sentry_sdk/scope.py | 20 +++++++------------- sentry_sdk/tracing.py | 43 ++++++++++++++++++++++++++----------------- 2 files changed, 33 insertions(+), 30 deletions(-) diff --git a/sentry_sdk/scope.py b/sentry_sdk/scope.py index f7070e8a02..fb3bee42f1 100644 --- a/sentry_sdk/scope.py +++ b/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): diff --git a/sentry_sdk/tracing.py b/sentry_sdk/tracing.py index e287130d3f..de18dbf59d 100644 --- a/sentry_sdk/tracing.py +++ b/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) @@ -307,10 +313,7 @@ def to_tracestate(self): 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 + transaction = self.containing_transaction # we should have the relevant values stored on the transaction, but if # this is an orphan span, make a new value @@ -437,10 +440,7 @@ 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 + transaction = self.containing_transaction if transaction: rv["tracestate"] = transaction._sentry_tracestate @@ -497,6 +497,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: From 14bd3d80f72e7e0d24d6ed6abb205f30a8aa9597 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Tue, 24 Aug 2021 00:26:04 -0700 Subject: [PATCH 3/4] switch to lazy creation of tracestate value --- sentry_sdk/tracing.py | 60 +++++++++++++++++++++++------- tests/tracing/test_http_headers.py | 4 ++ 2 files changed, 50 insertions(+), 14 deletions(-) diff --git a/sentry_sdk/tracing.py b/sentry_sdk/tracing.py index de18dbf59d..4a4f8b2e3f 100644 --- a/sentry_sdk/tracing.py +++ b/sentry_sdk/tracing.py @@ -263,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() @@ -310,19 +314,21 @@ 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. """ - transaction = self.containing_transaction - - # 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 + sentry_tracestate = self.get_or_set_sentry_tracestate() + third_party_tracestate = ( + self.containing_transaction._third_party_tracestate + if self.containing_transaction + else None + ) if not sentry_tracestate: return None @@ -334,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 @@ -440,10 +465,14 @@ def get_trace_context(self): if self.status: rv["status"] = self.status - 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 @@ -482,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): diff --git a/tests/tracing/test_http_headers.py b/tests/tracing/test_http_headers.py index 5c0cad320b..66c7165ca0 100644 --- a/tests/tracing/test_http_headers.py +++ b/tests/tracing/test_http_headers.py @@ -31,6 +31,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,6 +48,7 @@ def test_tracestate_computation(sentry_init): } +@pytest.mark.xfail() # TODO kmclb def test_adds_new_tracestate_to_transaction_when_none_given(sentry_init): sentry_init( dsn="https://dogsarebadatkeepingsecrets@squirrelchasers.ingest.sentry.io/12312012", From d5db89c7f07cda785d6f433f3e5388ed16c74c08 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Fri, 27 Aug 2021 14:24:00 -0700 Subject: [PATCH 4/4] add tests --- tests/tracing/test_http_headers.py | 87 +++++++++++++++++++++++++++++- 1 file changed, 85 insertions(+), 2 deletions(-) diff --git a/tests/tracing/test_http_headers.py b/tests/tracing/test_http_headers.py index 66c7165ca0..d86c7bf4d4 100644 --- a/tests/tracing/test_http_headers.py +++ b/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, @@ -48,8 +49,7 @@ def test_tracestate_computation(sentry_init): } -@pytest.mark.xfail() # TODO kmclb -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", @@ -62,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):