-
Notifications
You must be signed in to change notification settings - Fork 93
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
feat(threads): Add state as an optional key in the threads interface #1896
feat(threads): Add state as an optional key in the threads interface #1896
Conversation
Thread state is currently sent by the Android SDK but isn't extracted. We want to extract and store it on the event json.
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.
LGTM.
@@ -9,6 +9,7 @@ | |||
- Strip quotes from client hint values. ([#1874](https://github.com/getsentry/relay/pull/1874)) | |||
- Add Dotnet, Javascript and PHP support for profiling. ([#1871](https://github.com/getsentry/relay/pull/1871), [#1876](https://github.com/getsentry/relay/pull/1876), [#1885](https://github.com/getsentry/relay/pull/1885)) | |||
- Scrub `span.data.http.query` with default scrubbers. ([#1889](https://github.com/getsentry/relay/pull/1889)) | |||
- Add `thread.state` field to protocol. ([#1896](https://github.com/getsentry/relay/pull/1896)) |
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.
Please also add an entry to py/CHANGELOG.md
.
@@ -125,6 +125,10 @@ pub struct Thread { | |||
/// A flag indicating whether the thread was responsible for rendering the user interface. | |||
pub main: Annotated<bool>, | |||
|
|||
/// Thread state at the time of the crash. | |||
#[metastructure(skip_serialization = "empty")] | |||
pub state: Annotated<String>, |
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.
Now that I'm thinking about it, it's probably best to make this an enum
instead. From your PR description, I assume you're focusing mostly on Android for now, and you link to two places where there's a list of possible values. We can start from these.
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.
Approving as String
because only Android has 38 different options, and I don't think it's worth the effort of typing every case at the moment.
Thread state is an optional string currently sent by the Android SDK on the threads interface but isn't extracted. We want to extract and store it on the event json. We don't anticipate running any queries against this field. Corresponding relay changes (getsentry/relay#1896).
Thread state is an optional string currently sent by the Android SDK on the threads interface but isn't extracted. We want to extract and store it on the event json. Currently, we capture the JVM states, but in the ANRv2 implementation, we expect to capture to states the Android VM provides. Note, these values are for Android only, other SDKs could send other values.
We only need to save thread state in nodestore, we don't anticipate running any queries against this field.
Here is an example event payload with thread states.
closes getsentry/sentry#45196