From ddaf57ce718ea7587740a710c66062c9e3fb5a67 Mon Sep 17 00:00:00 2001 From: Mark Story Date: Mon, 9 Mar 2020 17:15:07 -0400 Subject: [PATCH 1/4] fix(tracing) Omit the top level status tag In the product side we don't really want people to search by this tag as it is far more expensive than the `transaction.status` property which is indexed separately. By not emitting this tag and only including it in the trace context we won't end up with poor performing tags for users to click on. --- sentry_sdk/tracing.py | 2 +- tests/test_tracing.py | 5 ++++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/sentry_sdk/tracing.py b/sentry_sdk/tracing.py index f0c6b873f4..0265e57918 100644 --- a/sentry_sdk/tracing.py +++ b/sentry_sdk/tracing.py @@ -408,7 +408,7 @@ def get_trace_context(self): } if "status" in self._tags: - rv["status"] = self._tags["status"] + rv["status"] = self._tags.pop("status") return rv diff --git a/tests/test_tracing.py b/tests/test_tracing.py index bd1fdcf535..2e7fbf1bea 100644 --- a/tests/test_tracing.py +++ b/tests/test_tracing.py @@ -12,7 +12,8 @@ def test_basic(sentry_init, capture_events, sample_rate): sentry_init(traces_sample_rate=sample_rate) events = capture_events() - with Hub.current.start_span(transaction="hi"): + with Hub.current.start_span(transaction="hi") as span: + span.set_status('ok') with pytest.raises(ZeroDivisionError): with Hub.current.start_span(op="foo", description="foodesc"): 1 / 0 @@ -32,6 +33,8 @@ def test_basic(sentry_init, capture_events, sample_rate): assert span2["op"] == "bar" assert span2["description"] == "bardesc" assert parent_span["transaction"] == "hi" + assert "status" not in event["tags"] + assert event["contexts"]["trace"]["status"] == "ok" else: assert not events From ba6033fbbb334208e854e4994e3ad97cb6bbca15 Mon Sep 17 00:00:00 2001 From: Markus Unterwaditzer Date: Wed, 11 Mar 2020 10:08:05 +0100 Subject: [PATCH 2/4] fix: Formatting --- tests/test_tracing.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_tracing.py b/tests/test_tracing.py index 2e7fbf1bea..237c0e6ebb 100644 --- a/tests/test_tracing.py +++ b/tests/test_tracing.py @@ -13,7 +13,7 @@ def test_basic(sentry_init, capture_events, sample_rate): events = capture_events() with Hub.current.start_span(transaction="hi") as span: - span.set_status('ok') + span.set_status("ok") with pytest.raises(ZeroDivisionError): with Hub.current.start_span(op="foo", description="foodesc"): 1 / 0 From c7067e0b2fb0f922c02338ccde7546e51c62a1e7 Mon Sep 17 00:00:00 2001 From: Mark Story Date: Wed, 11 Mar 2020 13:30:24 -0400 Subject: [PATCH 3/4] Fix status being dropped on inner spans. To workaround get_trace_context() being called multiple times I needed an additional non-tags place to store the status. I've had to shim the status back into tags as non-transaction spans expect to have status as a tag. --- sentry_sdk/tracing.py | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/sentry_sdk/tracing.py b/sentry_sdk/tracing.py index 0265e57918..c926270070 100644 --- a/sentry_sdk/tracing.py +++ b/sentry_sdk/tracing.py @@ -103,6 +103,7 @@ class Span(object): "description", "start_timestamp", "_start_timestamp_monotonic", + "status", "timestamp", "_tags", "_data", @@ -122,6 +123,7 @@ def __init__( op=None, # type: Optional[str] description=None, # type: Optional[str] hub=None, # type: Optional[sentry_sdk.Hub] + status=None, # type: Optional[str] ): # type: (...) -> None self.trace_id = trace_id or uuid.uuid4().hex @@ -132,6 +134,7 @@ def __init__( self.transaction = transaction self.op = op self.description = description + self.status = status self.hub = hub self._tags = {} # type: Dict[str, str] self._data = {} # type: Dict[str, Any] @@ -183,7 +186,7 @@ def __enter__(self): def __exit__(self, ty, value, tb): # type: (Optional[Any], Optional[Any], Optional[Any]) -> None if value is not None: - self._tags.setdefault("status", "internal_error") + self.set_status("internal_error") hub, scope, old_span = self._context_manager_state del self._context_manager_state @@ -272,7 +275,7 @@ def set_data(self, key, value): def set_status(self, value): # type: (str) -> None - self.set_tag("status", value) + self.status = value def set_http_status(self, http_status): # type: (int) -> None @@ -309,7 +312,7 @@ def set_http_status(self, http_status): def is_success(self): # type: () -> bool - return self._tags.get("status") == "ok" + return self.status == "ok" def finish(self, hub=None): # type: (Optional[sentry_sdk.Hub]) -> Optional[str] @@ -387,6 +390,9 @@ def to_json(self, client): if transaction: rv["transaction"] = transaction + if self.status: + self._tags['status'] = self.status + tags = self._tags if tags: rv["tags"] = tags @@ -406,9 +412,8 @@ def get_trace_context(self): "op": self.op, "description": self.description, } - - if "status" in self._tags: - rv["status"] = self._tags.pop("status") + if self.status: + rv["status"] = self.status return rv From be93706e538286912521c2caa1c6a18a02f334f4 Mon Sep 17 00:00:00 2001 From: Mark Story Date: Wed, 11 Mar 2020 15:31:19 -0400 Subject: [PATCH 4/4] Fix black --- sentry_sdk/tracing.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sentry_sdk/tracing.py b/sentry_sdk/tracing.py index c926270070..9293365b83 100644 --- a/sentry_sdk/tracing.py +++ b/sentry_sdk/tracing.py @@ -391,7 +391,7 @@ def to_json(self, client): rv["transaction"] = transaction if self.status: - self._tags['status'] = self.status + self._tags["status"] = self.status tags = self._tags if tags: