Skip to content

fix(span-buffer): Copy the event_id field into attributes #94914

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

gggritso
Copy link
Member

@gggritso gggritso commented Jul 4, 2025

Closes STREAM-294. The event_id field should be copied into the sentry.event_id attribute, to copy what Snuba does. I'm not sure why this field is missing, is there a specific reason? I took the liberty of adding a ticket and putting up the PR 🙏🏻

@gggritso gggritso requested review from a team as code owners July 4, 2025 15:37
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Jul 4, 2025
@untitaker
Copy link
Member

event_id will stop existing once transactions are gone. so looking forward we cannot support it.

@gggritso
Copy link
Member Author

gggritso commented Jul 7, 2025

@untitaker I'm confused. People will continue sending transactions via old SDKs, those transactions will keep having event IDs, Relay will continue setting that attribute. I'm not sure how many people have alerts/dashboards/workflow that rely on it, but dropping event_id from the buffer literally causes data loss, no?

@untitaker
Copy link
Member

untitaker commented Jul 7, 2025

I mean, I don't know how you use this attribute in the UI. But shouldn't the product end up working the same no matter how a user sends the data? if the product relies on event_id today to identify transactions, it will start to change behavior once the user upgrades the SDK. so some kind of migration has to be done.

@gggritso
Copy link
Member Author

gggritso commented Jul 7, 2025

@untitaker okay, I talked with some other folks on the performance team, here's the scoop:

  1. The Performance product uses the event_id for a bunch of important functionality. The biggest one is that in the Trace Waterfall, event_id is used to fetch a bunch of extra info (breadcrumbs, performance issues, etc.) from nodestore, from occurrences, and other places
  2. There is a clear path forward to stop relying on event_id, but it's long-term. We can stop using it once performance issue occurrences, breadcrumbs and other telemetry makes its way into EAP. Until that happens, we need event_id to look those things up

So, I think the upshot is that we have to keep event_id handling in the buffer for now, because otherwise switching to the buffer will break a bunch of existing functionality. FWIW Shruthi confirmed that there's a clear path forward for migrating user queries and UI functionality off event_id in the future

@gggritso gggritso merged commit 0ad18c8 into master Jul 7, 2025
67 checks passed
@gggritso gggritso deleted the georgegritsouk/stream-294-propagate-event_id-field-to-span-attributes branch July 7, 2025 20:17
andrewshie-sentry pushed a commit that referenced this pull request Jul 14, 2025
Closes STREAM-294. The `event_id` field should be copied into the
`sentry.event_id` attribute, to copy what Snuba does. I'm not sure why
this field is missing, is there a specific reason? I took the liberty of
adding a ticket and putting up the PR 🙏🏻
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