Skip to content
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.

Sign up for GitHub

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(on-demand): Extend support fields in the event getter #2640

Merged
merged 7 commits into from
Oct 23, 2023
Merged

feat(on-demand): Extend support fields in the event getter #2640

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ab19015
feat(on-demand): Extend support fields in the event getter
iambriccardo Oct 20, 2023
c3619ac
Add tests
iambriccardo Oct 20, 2023
1e1bee7
Implement
iambriccardo Oct 20, 2023
c0f7180
Update fixtures
iambriccardo Oct 20, 2023
9679a8e
Add changelog
iambriccardo Oct 20, 2023
a03576d
Update changelog
iambriccardo Oct 23, 2023
528419a
Merge
iambriccardo Oct 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR extends the event schema. Could we move this to the features section, and also update the py/changelog?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure


## 23.10.0

Expand Down
26 changes: 26 additions & 0 deletions relay-event-schema/src/protocol/contexts/device.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#[cfg(feature = "jsonschema")]
use relay_jsonschema_derive::JsonSchema;
use relay_protocol::{Annotated, Empty, FromValue, IntoValue, Object, Value};
use uuid::Uuid;

use crate::processor::ProcessValue;

Expand Down Expand Up @@ -161,6 +162,22 @@ pub struct DeviceContext {
/// Whether location support is available on the device.
pub supports_location_service: Annotated<bool>,

/// Width of the screen in pixels.
#[metastructure(pii = "maybe")]
pub screen_width_pixels: Annotated<u64>,
Comment on lines +165 to +167
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this property having pii make sense? Same for the fields below.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@jan-auer jan-auer Oct 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


/// Height of the screen in pixels.
#[metastructure(pii = "maybe")]
pub screen_height_pixels: Annotated<u64>,

/// Locale of the device.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

#[metastructure(pii = "maybe")]
pub locale: Annotated<String>,

/// UUID of the device.
#[metastructure(pii = "maybe")]
pub uuid: Annotated<Uuid>,

/// Additional arbitrary fields for forwards compatibility
#[metastructure(additional_properties, retain = "true", pii = "maybe")]
pub other: Object<Value>,
Expand Down Expand Up @@ -200,6 +217,7 @@ impl super::DefaultContext for DeviceContext {
#[cfg(test)]
mod tests {
use relay_protocol::{Annotated, Object, Value};
use uuid::uuid;

use super::*;
use crate::protocol::Context;
Expand Down Expand Up @@ -243,6 +261,10 @@ mod tests {
"supports_gyroscope": true,
"supports_audio": true,
"supports_location_service": true,
"screen_width_pixels": 1920,
"screen_height_pixels": 1080,
"locale": "US",
"uuid": "abadcade-feed-dead-beef-baddadfeeded",
"other": "value",
"type": "device"
}"#;
Expand Down Expand Up @@ -285,6 +307,10 @@ mod tests {
supports_gyroscope: Annotated::new(true),
supports_audio: Annotated::new(true),
supports_location_service: Annotated::new(true),
screen_width_pixels: Annotated::new(1920),
screen_height_pixels: Annotated::new(1080),
locale: Annotated::new("US".to_string()),
uuid: Annotated::new(uuid!("abadcade-feed-dead-beef-baddadfeeded")),
other: {
let mut map = Object::new();
map.insert(
Expand Down
93 changes: 92 additions & 1 deletion relay-event-schema/src/protocol/event.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -650,16 +650,59 @@ impl Getter for Event {
"user.geo.region" => self.user.value()?.geo.value()?.region.as_str()?.into(),
"user.geo.subdivision" => self.user.value()?.geo.value()?.subdivision.as_str()?.into(),
"request.method" => self.request.value()?.method.as_str()?.into(),
"sdk.name" => self.client_sdk.value()?.name.as_str()?.into(),
"sdk.version" => self.client_sdk.value()?.version.as_str()?.into(),

// Partial implementation of contexts.
"contexts.device.name" => self.context::<DeviceContext>()?.name.as_str()?.into(),
"contexts.device.arch" => self.context::<DeviceContext>()?.arch.as_str()?.into(),
"contexts.device.battery_level" => self
.context::<DeviceContext>()?
.battery_level
.value()?
.into(),
"contexts.device.brand" => self.context::<DeviceContext>()?.brand.as_str()?.into(),
"contexts.device.charging" => self.context::<DeviceContext>()?.charging.value()?.into(),
"contexts.device.family" => self.context::<DeviceContext>()?.family.as_str()?.into(),
"contexts.device.locale" => self.context::<DeviceContext>()?.locale.as_str()?.into(),
"contexts.device.online" => self.context::<DeviceContext>()?.online.value()?.into(),
"contexts.device.orientation" => self
.context::<DeviceContext>()?
.orientation
.as_str()?
.into(),
"contexts.device.name" => self.context::<DeviceContext>()?.name.as_str()?.into(),
"contexts.device.screen_density" => self
.context::<DeviceContext>()?
.screen_density
.value()?
.into(),
"contexts.device.screen_dpi" => {
self.context::<DeviceContext>()?.screen_dpi.value()?.into()
}
"contexts.device.screen_width_pixels" => self
.context::<DeviceContext>()?
.screen_width_pixels
.value()?
.into(),
"contexts.device.screen_height_pixels" => self
.context::<DeviceContext>()?
.screen_height_pixels
.value()?
.into(),
"contexts.device.simulator" => {
self.context::<DeviceContext>()?.simulator.value()?.into()
}
"contexts.os.build" => self.context::<OsContext>()?.build.as_str()?.into(),
"contexts.os.kernel_version" => {
self.context::<OsContext>()?.kernel_version.as_str()?.into()
}
"contexts.os.name" => self.context::<OsContext>()?.name.as_str()?.into(),
"contexts.os.version" => self.context::<OsContext>()?.version.as_str()?.into(),
"contexts.browser.name" => self.context::<BrowserContext>()?.name.as_str()?.into(),
"contexts.browser.version" => {
self.context::<BrowserContext>()?.version.as_str()?.into()
}
"contexts.device.uuid" => self.context::<DeviceContext>()?.uuid.value()?.into(),
"contexts.trace.status" => self
.context::<TraceContext>()?
.status
Expand Down Expand Up @@ -717,6 +760,7 @@ mod tests {
use chrono::{TimeZone, Utc};
use relay_protocol::{ErrorKind, Map, Meta};
use similar_asserts::assert_eq;
use uuid::uuid;

use super::*;
use crate::protocol::{Headers, IpAddr, JsonLenientString, PairList, TagEntry};
Expand Down Expand Up @@ -998,6 +1042,11 @@ mod tests {
segment: Annotated::new("user-seg".into()),
..Default::default()
}),
client_sdk: Annotated::new(ClientSdkInfo {
name: Annotated::new("sentry-javascript".into()),
version: Annotated::new("1.87.0".into()),
..Default::default()
}),
exceptions: Annotated::new(Values {
values: Annotated::new(vec![Annotated::new(Exception {
value: Annotated::new(JsonLenientString::from(
Expand Down Expand Up @@ -1028,6 +1077,12 @@ mod tests {
name: Annotated::new("iphone".to_string()),
family: Annotated::new("iphone-fam".to_string()),
model: Annotated::new("iphone7,3".to_string()),
screen_dpi: Annotated::new(560),
screen_width_pixels: Annotated::new(1920),
screen_height_pixels: Annotated::new(1080),
locale: Annotated::new("US".into()),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A correct value for this field would be en-US or de-AT.

uuid: Annotated::new(uuid!("abadcade-feed-dead-beef-baddadfeeded")),
charging: Annotated::new(true),
..DeviceContext::default()
});
contexts.add(OsContext {
Expand Down Expand Up @@ -1079,6 +1134,42 @@ mod tests {
event.get_value("event.tags.custom")
);
assert_eq!(None, event.get_value("event.tags.doesntexist"));
assert_eq!(
Some(Val::String("sentry-javascript")),
event.get_value("event.sdk.name")
);
assert_eq!(
Some(Val::String("1.87.0")),
event.get_value("event.sdk.version")
);
assert_eq!(
Some(Val::String("17.4.0")),
event.get_value("event.contexts.os.kernel_version")
);
assert_eq!(
Some(Val::I64(560)),
event.get_value("event.contexts.device.screen_dpi")
);
assert_eq!(
Some(Val::Bool(true)),
event.get_value("event.contexts.device.charging")
);
assert_eq!(
Some(Val::U64(1920)),
event.get_value("event.contexts.device.screen_width_pixels")
);
assert_eq!(
Some(Val::U64(1080)),
event.get_value("event.contexts.device.screen_height_pixels")
);
assert_eq!(
Some(Val::String("US")),
event.get_value("event.contexts.device.locale")
);
assert_eq!(
Some(Val::Uuid(uuid!("abadcade-feed-dead-beef-baddadfeeded"))),
event.get_value("event.contexts.device.uuid")
);
}

#[test]
Expand Down
4 changes: 2 additions & 2 deletions relay-server/tests/snapshots/test_fixtures__android__pii_stripping.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,11 +45,11 @@ expression: SerializableAnnotated(&event)
"external_free_storage": 534702080,
"boot_time": null,
"timezone": null,
"screen_width_pixels": null,
"screen_height_pixels": null,
"archs": [
"x86"
],
"screen_height_pixels": null,
"screen_width_pixels": null,
"type": "device"
},
"os": {
Expand Down
37 changes: 37 additions & 0 deletions relay-server/tests/snapshots/test_fixtures__event_schema.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1209,6 +1209,14 @@ expression: "relay_event_schema::protocol::event_json_schema()"
"format": "uint64",
"minimum": 0.0
},
"locale": {
"description": " Locale of the device.",
"default": null,
"type": [
"string",
"null"
]
},
"low_memory": {
"description": " Whether the device was low on memory.",
"default": null,
Expand Down Expand Up @@ -1314,6 +1322,16 @@ expression: "relay_event_schema::protocol::event_json_schema()"
"format": "uint64",
"minimum": 0.0
},
"screen_height_pixels": {
"description": " Height of the screen in pixels.",
"default": null,
"type": [
"integer",
"null"
],
"format": "uint64",
"minimum": 0.0
},
"screen_resolution": {
"description": " Device screen resolution.\n\n (e.g.: 800x600, 3040x1444)",
"default": null,
Expand All @@ -1322,6 +1340,16 @@ expression: "relay_event_schema::protocol::event_json_schema()"
"null"
]
},
"screen_width_pixels": {
"description": " Width of the screen in pixels.",
"default": null,
"type": [
"integer",
"null"
],
"format": "uint64",
"minimum": 0.0
},
"simulator": {
"description": " Simulator/prod indicator.",
"default": null,
Expand Down Expand Up @@ -1397,6 +1425,15 @@ expression: "relay_event_schema::protocol::event_json_schema()"
],
"format": "uint64",
"minimum": 0.0
},
"uuid": {
"description": " UUID of the device.",
"default": null,
"type": [
"string",
"null"
],
"format": "uuid"
}
},
"additionalProperties": false
Expand Down
Loading