Skip to content

fix(issue detectors): Fix missing nodestore data on segment-derived occurrences - #123288

Merged
lobsterkatie merged 6 commits into
masterfrom
kmclb-fix-segment-performance-problem-occurrence-creation
Sep 1, 2026
Merged

fix(issue detectors): Fix missing nodestore data on segment-derived occurrences#123288
lobsterkatie merged 6 commits into
masterfrom
kmclb-fix-segment-performance-problem-occurrence-creation

Conversation

@lobsterkatie

Copy link
Copy Markdown
Member

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 an Event backed only by snuba_data and 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 event we 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.

@github-actions github-actions Bot added the Scope: Backend Automatically applied to PRs that change backend components label Sep 1, 2026
"op": span.get("op"),
"description": trimmed_description,
"start_timestamp": span.get("start_timestamp"),
"timestamp": span.get("timestamp"),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"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

@lobsterkatie lobsterkatie Sep 1, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@lobsterkatie
lobsterkatie marked this pull request as ready for review September 1, 2026 00:34
@lobsterkatie
lobsterkatie requested review from a team as code owners September 1, 2026 00:34
Comment on lines +476 to +480
occurrence_event_data = {
**event_data,
"event_id": occurrence_id,
"spans": occurrence_spans,
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. It doesn't raise InvalidEventPayloadError. jsonschema.ValidationError isn't a subclass of ValueError or KeyError (MRO is ValidationError → _Error → Exception), so neither _get_kwargs's except (KeyError, ValueError) nor _process_message's catches it. It propagates as ValidationError and is finally swallowed by run.py's blanket except Exception.

  2. It isn't silent. Before it propagates, validation failure fires occurrence_ingest.event_payload_invalid, logs logger.exception("Error validating event payload, falling back to legacy validation"), retries against LEGACY_EVENT_PAYLOAD_SCHEMA (which also requires a non-empty string platform), then fires occurrence_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.

@lobsterkatie
lobsterkatie merged commit a17c071 into master Sep 1, 2026
75 checks passed
@lobsterkatie
lobsterkatie deleted the kmclb-fix-segment-performance-problem-occurrence-creation branch September 1, 2026 00:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scope: Backend Automatically applied to PRs that change backend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants