-
Notifications
You must be signed in to change notification settings - Fork 104
feat(session): Add unhandled session status type
#4939
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
c54dd86 to
92e3074
Compare
Dav1dde
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have some session integration tests in test_session.py, would be great if you can add/modify them to also test the new codepaths.
|
@Dav1dde I don't think that instead i'll add some tests at the bottom of session.rs and mod.rs. The existing tests are not comprehensive to test all the session state types, but i'll expand them as best i can. |
|
It would be good to still have a single integration test in Python, you're right it seems like the tests in I generally want at least one integration test for every protocol change, just to show it works, especially since integration tests very easily can setup a 'Relay chain' to make sure, the protocol change also works when interacting with multiple different types of Relays. A Rust unittest, can unfortunately not do that. On a side note, I am now seeing that these tests are pretty worthless, as Relay stopped supporting producing to a sessions topic over a year ago :( |
Co-authored-by: David Herberth <david.herberth@sentry.io>
This is in service of this project: https://www.notion.so/sentry/RFC-Session-Status-unhandled-2308b10e4b5d801d9805fece61533ac5#2308b10e4b5d80e9b312ecbea27a4af3
TL/DR:
We're adding a new enum value to split up the existing
crashedrelease health metric into two more specific metrics:For cases like Flutter: a Dart error will not crash the running process, while a native error will. We want to be clearer about this when we report errors to sentry users.
This PR
This PR will allow us to properly count
sessionandsessionsenvelopes that contain the new value. When asessionenvelope arrives that has the new value:status: unhandledwe want theSessionLikemetric to handle that the same way we handlestatus: abnormalorstatus: crashed. We'll subtract 1 from theerrorscount and return1fromunhandled_count().Similarly, for
sessionsenvelopes we need to implement theunhandled_count()function.Finally, we have to implement the if-statements that will get the new count from the session,
session.unhandled_count()and push the value into the list.Fixes REPLAY-549