feat(scope): Bring back lastEventId on isolation scope (#11951)
#12022
Conversation
andreiborza
changed the title
lastEventId on isolation scope (#11951)
size-limit report 📦
|
packages/core/src/baseclient.ts
Outdated
| const eventId = event.event_id || hint.event_id; | ||
| if (eventId) { | ||
| isolationScope.setLastEventId(eventId); | ||
| } |
There was a problem hiding this comment.
m: I think we pass all kinds of events through _prepareEvent right? Before removing this function, we only updated the last event id for error events. I'd argue we should do the same now.
see
sentry-javascript/packages/core/src/hub.ts
Lines 396 to 398 in 90b45f0
There was a problem hiding this comment.
Yea, everything goes through that, including captureMessage and captureEvent. I aligned this change to the Python SDK (ref: getsentry/sentry-python#3064) where they also store it for any event.
The name kind of sounds to me like it would really contain any event, not just errors, but I can change it to just errors, wdyt?
cc @mydea
There was a problem hiding this comment.
Sorry, I think I worded this incorrectly. I'm totally fine with error and message events that's all good. They all have event.type === 'undefined'.
As for Replay or transaction/span events, these also go through _prepareEvent but they have a different event.type. I woudn't update lastEventId for them because
- then users will much more likely get completely unexpected
lastEventIdreturn values. For example, a transaction could end right between the error being captured and users callinglastEventId--> the return value would return the transaction event id. - previous behaviour was also to not update the id for these events.
I agree, naming-wise lastEventId implies any event but this has historic reasons. This API probably existed long before we decided to send spans/transactions and also treat them as events with a different type.
Happy to be overruled but I think we should consider at least point 1
There was a problem hiding this comment.
I agree with lukas, that makes the most sense to me too! :)
There was a problem hiding this comment.
@Lms24 fixed. Thanks for the explanation, I assumed too much what an Event is :)
Co-authored-by: Lukas Stracke <lukas.stracke@sentry.io>
andreiborza
force-pushed
the
ab/last-event-id-scope
branch
from
f85d369
to
2e387fc
Compare
…2022) Co-authored-by: Lukas Stracke <lukas.stracke@sentry.io>
As discussed, we'll keep it simple and set the event id regardless of the event getting dropped.
A follow-up PR will update
ReportDialogOptionsto makeeventIdoptional again as well as use the isolation scope directly in React ErrorBoundaries.