feat(on-demand): Extend support fields in the event getter #2640
Conversation
| /// Width of the screen in pixels. | ||
| #[metastructure(pii = "maybe")] | ||
| pub screen_width_pixels: Annotated<u64>, |
There was a problem hiding this comment.
Does this property having pii make sense? Same for the fields below.
There was a problem hiding this comment.
I thought about this, and I was like, maybe some people want to scrape those fields, since they might not want to share that information with sentry (e.g., a new device with a new resolution is being secretly tested), maybe the UUID is the only thing but also that can be delicate for some users. We can definitely omit it, idk if there is a performance overhead to this.
There was a problem hiding this comment.
I'd rather not mark them PII. They are not, and none of the other fields are marked as such.
The screen size is not PII, however since the other screen attributes are marked as PII, we should stay consistent here.
CHANGELOG.md
Outdated
| @@ -12,6 +12,7 @@ | |||
| - Restrict resource spans to script and css only. ([#2623](https://github.com/getsentry/relay/pull/2623)) | |||
| - Postpone metrics aggregation until we received the project state. ([#2588](https://github.com/getsentry/relay/pull/2588)) | |||
| - Scrub random strings in resource span descriptions. ([#2614](https://github.com/getsentry/relay/pull/2614)) | |||
| - Extend the number of supported fields for the `Event` `Getter`. ([#2640](https://github.com/getsentry/relay/pull/2640)) | |||
There was a problem hiding this comment.
This PR extends the event schema. Could we move this to the features section, and also update the py/changelog?
| /// Width of the screen in pixels. | ||
| #[metastructure(pii = "maybe")] | ||
| pub screen_width_pixels: Annotated<u64>, |
There was a problem hiding this comment.
I'd rather not mark them PII. They are not, and none of the other fields are marked as such.
The screen size is not PII, however since the other screen attributes are marked as PII, we should stay consistent here.
| screen_dpi: Annotated::new(560), | ||
| screen_width_pixels: Annotated::new(1920), | ||
| screen_height_pixels: Annotated::new(1080), | ||
| locale: Annotated::new("US".into()), |
There was a problem hiding this comment.
A correct value for this field would be en-US or de-AT.
| #[metastructure(pii = "maybe")] | ||
| pub screen_height_pixels: Annotated<u64>, | ||
|
|
||
| /// Locale of the device. |
There was a problem hiding this comment.
Let's try to add more information to the doc comments, such as what is the format. I'll pick this up in my follow-up.
This PR adds support for additional fields in the
EventGetterimplementation in order to satisfy a wider range of conditions for on-demand metrics.In addition, it adds the following new fields to the
DeviceContext:Closes getsentry/sentry#58504