diff --git a/src/sentry/api/endpoints/project_trace_item_details.py b/src/sentry/api/endpoints/project_trace_item_details.py index f0659087b427..7c9a36372015 100644 --- a/src/sentry/api/endpoints/project_trace_item_details.py +++ b/src/sentry/api/endpoints/project_trace_item_details.py @@ -431,16 +431,29 @@ def serialize_links(attributes: list[dict[str, Any]]) -> list[TraceItemLink] | N if link_attribute is None: return None + value = link_attribute.get("value", {}).get("valStr", None) + if value is None: + return None + return parse_links_json(value) + + +def parse_links_json(raw: str) -> list[TraceItemLink] | None: + """Parse the JSON stored in `sentry.links`, keeping every link that can be serialized.""" try: - value = link_attribute.get("value", {}).get("valStr", None) - if value is not None: - links = json.loads(value) - return [serialize_link(link) for link in links] - else: - return None - except Exception as e: + links = json.loads(raw) + except json.JSONDecodeError as e: sentry_sdk.capture_exception(e) return None + if not isinstance(links, list): + return None + + parsed = [] + for link in links: + try: + parsed.append(serialize_link(link)) + except (AttributeError, KeyError, TypeError) as e: + sentry_sdk.capture_exception(e) + return parsed def serialize_link(link: dict[str, Any]) -> TraceItemLink: diff --git a/src/sentry/seer/agent/tools.py b/src/sentry/seer/agent/tools.py index c213ac146df1..a3cd9e078b8a 100644 --- a/src/sentry/seer/agent/tools.py +++ b/src/sentry/seer/agent/tools.py @@ -17,6 +17,7 @@ from sentry import eventstore, features from sentry.api.client import client from sentry.api.endpoints.organization_events_timeseries import TOP_EVENTS_DATASETS +from sentry.api.endpoints.project_trace_item_details import parse_links_json from sentry.api.serializers.base import serialize from sentry.api.serializers.models.activity import ActivitySerializer, ActivitySerializerResponse from sentry.api.serializers.models.commit import CommitSerializer, CommitSerializerResponse @@ -64,7 +65,7 @@ ) from sentry.search.eap.occurrences.query_utils import build_event_id_in_filter from sentry.search.eap.resolver import SearchResolver -from sentry.search.eap.types import SearchResolverConfig +from sentry.search.eap.types import FieldsACL, SearchResolverConfig from sentry.search.events.constants import ISSUE_ID_ALIAS from sentry.search.events.types import SAMPLING_MODES, SnubaParams from sentry.seer.agent.index_data import UNESCAPED_QUOTE_RE @@ -629,6 +630,22 @@ def execute_trace_table_query( raise +SPAN_LINKS_ATTRIBUTE = "sentry.links" + + +def _parse_span_links(items: Sequence[dict[str, Any]]) -> None: + """Replace the raw `sentry.links` JSON on each span with parsed `links`, in place.""" + for item in items: + attributes = item.get("additional_attributes") + raw_links = ( + attributes.pop(SPAN_LINKS_ATTRIBUTE, None) if isinstance(attributes, dict) else None + ) + # EAP returns "" for spans without the attribute. + if raw_links and isinstance(raw_links, str) and (links := parse_links_json(raw_links)): + item["links"] = links + _parse_span_links(item.get("children", [])) + + def get_trace_waterfall( trace_id: str, organization_id: int, @@ -644,9 +661,11 @@ def get_trace_waterfall( Returns: The spans and errors in the trace, along with the full 32-character trace ID. + Spans with span links carry them in `links`. """ if additional_attributes is None: additional_attributes = ["span.status_code"] + additional_attributes = [*additional_attributes, SPAN_LINKS_ATTRIBUTE] try: organization = Organization.objects.get(id=organization_id) @@ -688,12 +707,16 @@ def get_trace_waterfall( additional_attributes=additional_attributes, referrer=referrer, organization=organization, + # `sentry.links` is private, so only this caller may select it. + config=SearchResolverConfig(fields_acl=FieldsACL(attributes={SPAN_LINKS_ATTRIBUTE})), ) + trace = cast(list[dict[str, Any]], events) + _parse_span_links(trace) return EAPTrace( trace_id=full_trace_id, org_id=organization_id, - trace=events, + trace=trace, ) diff --git a/src/sentry/snuba/spans_rpc.py b/src/sentry/snuba/spans_rpc.py index df8f7c820d5a..820876757094 100644 --- a/src/sentry/snuba/spans_rpc.py +++ b/src/sentry/snuba/spans_rpc.py @@ -143,7 +143,7 @@ def run_trace_query( "app.vitals.ttfd.value", *additional_attributes, ] - resolver = cls.get_resolver(params=params, config=SearchResolverConfig()) + resolver = cls.get_resolver(params=params, config=config) columns, _ = resolver.resolve_attributes(trace_attributes) meta = resolver.resolve_meta(referrer=referrer) request = GetTraceRequest( diff --git a/src/sentry/snuba/trace.py b/src/sentry/snuba/trace.py index ac7633c9b420..80b647d8a4a9 100644 --- a/src/sentry/snuba/trace.py +++ b/src/sentry/snuba/trace.py @@ -900,6 +900,7 @@ def query_trace_data( additional_attributes: list[str] | None = None, include_uptime: bool = False, organization: Organization | None = None, + config: SearchResolverConfig | None = None, ) -> list[SerializedTraceItem]: """Queries span/error data for a given trace""" # This is a hack, long term EAP will store both errors and performance_issues eventually but is not ready @@ -933,7 +934,7 @@ def query_trace_data( trace_id=trace_id, params=snuba_params, referrer=referrer, - config=SearchResolverConfig(), + config=config or SearchResolverConfig(), additional_attributes=all_additional_attributes, ) errors_future = query_thread_pool.submit( diff --git a/tests/sentry/seer/agent/test_tools.py b/tests/sentry/seer/agent/test_tools.py index 9d2a5b75b2fe..8d691aa3c5ac 100644 --- a/tests/sentry/seer/agent/test_tools.py +++ b/tests/sentry/seer/agent/test_tools.py @@ -32,6 +32,7 @@ EVENT_TIMESERIES_RESOLUTIONS, _get_issue_event_timeseries, _get_recommended_event, + _parse_span_links, execute_table_query, execute_timeseries_query, execute_trace_table_query, @@ -873,6 +874,55 @@ def test_get_trace_waterfall_short_id(self) -> None: def test_get_trace_waterfall_full_id(self) -> None: self._test_get_trace_waterfall(use_short_id=False) + def test_get_trace_waterfall_span_links(self) -> None: + trace_id = uuid.uuid4().hex + root = self.create_span( + { + "description": "GET /pricing", + "trace_id": trace_id, + "parent_span_id": None, + "is_segment": True, + "sentry_tags": { + "links": '[{"trace_id":"d099bf9ad5a143cf8f83a98081d0ed3b","span_id":"8873a98879faf06d","sampled":true,"attributes":{"sentry.link.type":{"type":"string","value":"cache_origin"}}}]', + }, + }, + start_ts=self.ten_mins_ago, + ) + child = self.create_span( + { + "description": "no links", + "trace_id": trace_id, + "parent_span_id": root["span_id"], + "is_segment": False, + }, + start_ts=self.ten_mins_ago, + ) + self.store_spans([root, child]) + + with patch( + "sentry.api.endpoints.project_trace_item_details.sentry_sdk.capture_exception" + ) as mock_capture: + result = get_trace_waterfall(trace_id, self.organization.id) + # EAP returns "" for the link-less child, which must not count as a bad payload. + mock_capture.assert_not_called() + + assert isinstance(result, EAPTrace) + spans = [result.trace[0], *result.trace[0]["children"]] + by_id = {span["event_id"]: span for span in spans} + assert by_id[root["span_id"]]["links"] == [ + { + "traceId": "d099bf9ad5a143cf8f83a98081d0ed3b", + "itemId": "8873a98879faf06d", + "sampled": True, + "attributes": [ + {"name": "sentry.link.type", "value": "cache_origin", "type": "str"} + ], + } + ] + assert "sentry.links" not in by_id[root["span_id"]]["additional_attributes"] + assert "links" not in by_id[child["span_id"]] + assert "sentry.links" not in by_id[child["span_id"]]["additional_attributes"] + def test_get_trace_waterfall_wrong_project(self) -> None: transaction_name = "api/users/profile" trace_id = uuid.uuid4().hex @@ -4448,3 +4498,51 @@ def test_legacy_fallback_only_when_gal_is_unavailable(self) -> None: assert self._activity() == [] with self.feature({"projects:issue-action-log-activity": False}): assert [item["type"] for item in self._activity()] == ["set_resolved"] + + +@pytest.mark.parametrize( + "raw_links", + [ + "not json", + '{"trace_id": "d099bf9ad5a143cf8f83a98081d0ed3b"}', + '[{"trace_id": "d099bf9ad5a143cf8f83a98081d0ed3b"}]', + '[{"trace_id": "d099bf9ad5a143cf8f83a98081d0ed3b", "span_id": "8873a98879faf06d", "attributes": ["x"]}]', + ], +) +def test_parse_span_links_skips_malformed_links(raw_links: str) -> None: + linked_child: dict[str, Any] = { + "event_id": "8873a98879faf06d", + "additional_attributes": { + "sentry.links": '[{"trace_id": "d099bf9ad5a143cf8f83a98081d0ed3b", "span_id": "8873a98879faf06d"}]' + }, + "children": [], + } + span: dict[str, Any] = { + "event_id": "a1b2c3d4e5f60718", + "additional_attributes": {"sentry.links": raw_links, "span.status_code": "200"}, + "children": [linked_child], + } + + _parse_span_links([span]) + + assert "links" not in span + assert span["additional_attributes"] == {"span.status_code": "200"} + assert linked_child["links"] == [ + {"traceId": "d099bf9ad5a143cf8f83a98081d0ed3b", "itemId": "8873a98879faf06d"} + ] + + +def test_parse_span_links_keeps_valid_links_next_to_malformed_ones() -> None: + span: dict[str, Any] = { + "event_id": "a1b2c3d4e5f60718", + "additional_attributes": { + "sentry.links": '[{"trace_id": "d099bf9ad5a143cf8f83a98081d0ed3b"}, {"trace_id": "d099bf9ad5a143cf8f83a98081d0ed3b", "span_id": "8873a98879faf06d"}]' + }, + "children": [], + } + + _parse_span_links([span]) + + assert span["links"] == [ + {"traceId": "d099bf9ad5a143cf8f83a98081d0ed3b", "itemId": "8873a98879faf06d"} + ] diff --git a/tests/snuba/api/endpoints/test_organization_trace.py b/tests/snuba/api/endpoints/test_organization_trace.py index 358fc9ad3011..1e32edc3059d 100644 --- a/tests/snuba/api/endpoints/test_organization_trace.py +++ b/tests/snuba/api/endpoints/test_organization_trace.py @@ -610,6 +610,14 @@ def test_with_additional_attributes(self) -> None: assert data[0]["children"][0]["additional_attributes"]["gen_ai.request.model"] == "gpt-4o" assert data[0]["children"][0]["additional_attributes"]["gen_ai.usage.total_tokens"] == 100 + def test_private_span_links_attribute_rejected(self) -> None: + self.load_trace() + with self.feature(self.FEATURES): + response = self.client_get( + data={"timestamp": self.day_ago, "additional_attributes": ["sentry.links"]}, + ) + assert response.status_code == 400, response.content + def test_with_span_description_in_additional_attrs(self) -> None: self.load_trace() with self.feature(self.FEATURES):