fix(issue detectors): Fix missing nodestore data on segment-derived occurrences - #123288
Conversation
| "op": span.get("op"), | ||
| "description": trimmed_description, | ||
| "start_timestamp": span.get("start_timestamp"), | ||
| "timestamp": span.get("timestamp"), |
There was a problem hiding this comment.
| "timestamp": span.get("timestamp"), | |
| "timestamp": span.get("timestamp"), | |
| "hash": span.get("hash"), |
I believe this gets used in the UI for DB issues to link to the query
There was a problem hiding this comment.
So this is precisely one of the follow-up issues that claude identified. Apparently hash isn't in the span schema that Relay uses, so it ends up undefined by the time it actually reaches nodestore, because it gets normalized away. There's therefore a follow-up change coming (both FE and BE) to rely on the sentry.group and sentry.category attributes instead. But I will make him check his work and explain it to me again before I push it that change. Otherwise I will add it back in here at that point.
| occurrence_event_data = { | ||
| **event_data, | ||
| "event_id": occurrence_id, | ||
| "spans": occurrence_spans, | ||
| } |
There was a problem hiding this comment.
Bug: A None value for the platform attribute causes schema validation to fail in the occurrence consumer, leading to the event being silently dropped.
Severity: HIGH
Suggested Fix
Ensure the platform attribute is never None when constructing the event_data. A default string value should be provided if the sentry.platform attribute is missing from the segment span to prevent schema validation failures in the occurrence consumer.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: src/sentry/spans/consumers/process_segments/message.py#L476-L480
Potential issue: When `build_shim_event_data` processes a segment span that lacks a
`sentry.platform` attribute, the `attribute_value` function defaults the `platform`
field to `None`. This `None` value is passed to the occurrence consumer, where it fails
schema validation because the `platform` is required to be a non-null string. This
failure raises an `InvalidEventPayloadError`, which is caught, causing the occurrence to
be silently dropped instead of being processed and stored. This affects spans from
services that do not have a platform attribute, such as backend services.
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
This is kind of right and kind of wrong, but regardless is separate from what this PR is fixing. According to Claude:
The premise is wrong
sentry.platform is a Relay-set attribute derived from the SDK. A Python backend sends platform: "python", PHP sends "php". There's nothing about backend services that would omit it — the bot appears to have invented that. It's one of the attributes enrichment.py explicitly propagates from segment to child spans, and the test fixture build_mock_span sets "sentry.platform": {"value": "python"}.
It isn't a regression from this PR
This is the part that settles it as review feedback. The schema validation lives in _get_kwargs, and it runs before the is_buffered_spans branch ever existed — so a None platform failed in exactly the same way on the old path. build_shim_event_data has always set platform from that attribute, and this PR doesn't touch either. The only platform line in the diff is the deletion of the line "platform": event_data.get("platform") inside the create_event() function this PR removes. So whatever the behavior is, it's identical before and after.
Two details in the description are wrong
Worth knowing because they change how you'd detect it:
-
It doesn't raise
InvalidEventPayloadError.jsonschema.ValidationErrorisn't a subclass ofValueErrororKeyError(MRO isValidationError → _Error → Exception), so neither_get_kwargs'sexcept (KeyError, ValueError)nor_process_message's catches it. It propagates asValidationErrorand is finally swallowed by run.py's blanketexcept Exception. -
It isn't silent. Before it propagates, validation failure fires
occurrence_ingest.event_payload_invalid, logslogger.exception("Error validating event payload, falling back to legacy validation"), retries againstLEGACY_EVENT_PAYLOAD_SCHEMA(which also requires a non-empty string platform), then firesoccurrence_ingest.legacy_event_payload_invalid. Two metrics and a logged exception.
That gives you a direct empirical answer: if this were happening, those two metrics would be non-zero. That beats arguing about it.
Is there anything real underneath?
Possibly, but not for the reason given. The plausible case is spans that never came from a Sentry SDK — OTLP ingestion, where there's no platform concept to map. Notably record_generic_event_processed(project, platform=None, ...) in this same file defaults platform to None, so the pipeline's own authors clearly consider a missing platform possible.
If you want to harden it, the fix is a default in build_shim_event_data rather than anything in this PR — and it belongs in a separate change, since it's pre-existing and would affect the transaction path's expectations too.
NOTE: This is heavily based on the work done in #123147, but includes a number of additional fixes identified by Claude as needing to be part of that change. Because the final result differs quite a bit from that PR, I created a new one which supersedes it. H/t to Claude for a great deal of help with the reasoning and rough drafts of all of the code included here, as well as all the tests.
Currently, occurrences from the span segment pipeline set
is_buffered_spans, which builds anEventbacked only bysnuba_dataand never writes to nodestore. The eventstream payload is therefore just{"received": ...}, so occurrences are reaching Snuba and showing up in the issue feed, but issue details is broken because there's no corresponding event JSON.To fix this problem, this PR drops the special case and sends segment-derived occurrences through the same path that every other occurrence takes. Each detected problem now carries its own event holding only the spans its evidence points at, filtered down to the fields issue details and Seer actually read, and trimmed to safely fit within the occurrence producer's limits.
Notes:
Since the
eventwe create is entirely synthetic, the occurrence id is reused for the event id.Trimming happens both in terms of how many spans we keep and in terms of the data in those spans. Offender spans are given priority over parent and cause spans, since offender spans are the ones actually at the root of whatever problem we're reporting.
Any span whose id is referenced in one of the span id lists (parent, cause, or offender) but which isn't present in the segment is dropped, so we never error out trying to pull data from a span we don't have.
This fixes new occurrences only. Ones already written by the old path still point at nodestore keys that were never created, and stay broken until they age out.
There are a number of other issues Claude noted when working on this, but for ease of review those have been split off into follow-up PRs.