Update session status to 'unhandled' and extend SessionStatus type #17849
Conversation
…ed' in error handling test
![seer-by-sentry[bot]](https://avatars.githubusercontent.com/in/801464?s=60&v=4){kind=small}
|
|
||
| export type SessionContext = Partial<Session>; | ||
|
|
||
| export type SessionStatus = 'ok' | 'exited' | 'crashed' | 'abnormal'; | ||
| export type SessionStatus = 'ok' | 'exited' | 'crashed' | 'abnormal' | 'unhandled'; | ||
|
|
||
| /** JSDoc */ | ||
| export interface SessionAggregates { |
There was a problem hiding this comment.
Potential bug: The new 'unhandled' session status is not handled by all parts of the system, leading to data corruption in session aggregation and inconsistent status reporting.
-
Description: The introduction of the
'unhandled'session status is incomplete. The session aggregation logic inpackages/node-core/src/integrations/http/httpServerIntegration.tscontains a hardcoded mapping of statuses that was not updated to include'unhandled'. When a session with this new status is processed, the logic attempts to access a non-existent key, which results inbucket['undefined'] = NaN. This corrupts the session aggregate data used for health monitoring. Additionally,packages/core/src/server-runtime-client.tswas not updated and still sets the session status to'crashed'for unhandled exceptions, creating an inconsistency with other parts of the client that now use'unhandled'. -
Suggested fix: Update the session aggregation mapping in
httpServerIntegration.tsto include the'unhandled'status. Also, updateserver-runtime-client.tsand any other remaining locations, including tests, to use the'unhandled'status instead of'crashed'for consistency.
severity: 0.75, confidence: 0.98
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
@Lms24 , we dont need to update the test cases in httpServerIntegration.test.ts file right? I noticed crashed represents unhandled cases, so, it would be pointless to handle unhandled separately, if I got that right.
… update related aggregates
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| } else { | ||
| // If it's an unhandled exception, we always set the status to 'unhandled'. | ||
| // 'unhandled' is the most severe status. | ||
| requestSession.status = 'unhandled'; |
There was a problem hiding this comment.
Bug: Session Status Update Logic Changed
The new logic for handled exceptions only updates session status from 'ok' to 'errored', changing previous behavior where 'abnormal' sessions would also transition. Additionally, unhandled exceptions now set the status to 'unhandled', making the existing comment referencing 'crashed' inaccurate.
There was a problem hiding this comment.
Hey @JealousGx thanks a lot for opening this PR!
Unfortunately, we just identified that this task is a breaking change. Not specifically API breaking but it would break a lot of the Sentry product behaviour (see #17842 (comment)).
This means that we'll not merge this PR until we release the next major version of the SDK (no timeline yet). I still had a brief look over it and my main concern is that this PR changes the session status on server. I would like to revisit the logic here once we more specifically figured out if server-side session statuses. If I had known this upfront, I wouldn't have added the "Good first issue" label. Sorry for the confusion!
Since the next major version is still a while out, I'm gonna close this PR in the meantime. It is linked to the issue though so we can re-open and pick it up once we get to work on this. Also we'll make sure to give you proper credit!
Thanks for contributing and apologies for the confusion!
2 participants
Change the session status from 'crashed' to 'unhandled' in the session update logic and tests. Extend the SessionStatus type to include 'unhandled' for better error handling.
Resolves #17842
yarn lint) & (yarn test).