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.

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(schema): Add missing fields to DeviceContext #1383

Merged
merged 5 commits into from
Aug 8, 2022

Conversation

AbhiPrasad
Copy link
Member

While helping review getsentry/develop#658, I realized the context spec in the develop docs is out of sync with the struct in Relay.

This PR syncs the DeviceContext with https://develop.sentry.dev/sdk/event-payloads/contexts/#device-context

Adds processor_count, cpu_description, processor_frequency, device_type, battery_status, device_unique_identifier, supports_vibration, supports_accelerometer, supports_gyroscope, supports_audio, and supports_location_service.

Another PR will be opened to update the other Contexts (and add CultureContext)

Sync the DeviceContext with https://develop.sentry.dev/sdk/event-payloads/contexts/#device-context

Adds `processor_count`, `cpu_description`, `processor_frequency`,
`device_type`, `battery_status`, `device_unique_identifier`,
`supports_vibration`, `supports_accelerometer`, `supports_gyroscope`,
`supports_audio`, and `supports_location_service`.
@AbhiPrasad AbhiPrasad requested review from bruno-garcia and a team August 4, 2022 18:00
Copy link
Member

@jjbayer jjbayer left a comment

Choose a reason for hiding this comment

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

@AbhiPrasad thanks for this! I think this matches @untitaker's
request from when the fields were documented: getsentry/develop#358 (review)

@untitaker Could you advise on what fields should have pii = "maybe"? Existing fields have it on most numeric fields but not on all string fields.

@untitaker
Copy link
Member

untitaker commented Aug 5, 2022

I think it's fine to add pii = "true" here. This is definitely PII. We use maybe as a way to express, "we are not sure if we want to scrub those things by default, but it should be possible to write Advanced Datascrubbing rules that specifically target those fields". In hindsight I think this distinction was a mistake because it causes a lot of confusion if people attempt to configure a rule like [Remove] [Credit Cards] from ** and it doesn't apply.

@AbhiPrasad
Copy link
Member Author

To clarify - I'll add the pii = "true" to the metastructure macro for device_unique_identifier, do we do it for the rest of the fields also?

@untitaker
Copy link
Member

untitaker commented Aug 5, 2022

@AbhiPrasad Is there a usecase for scrubbing sensitive data from them? (=> maybe) Do sdks send PII in those fields by default? (=> true)

@AbhiPrasad AbhiPrasad self-assigned this Aug 5, 2022
@AbhiPrasad
Copy link
Member Author

Alright - made some changes based on that to the pii macro, but maybe @bruno-garcia can comment on this more

/// CPU description.
///
/// For example, Intel(R) Core(TM)2 Quad CPU Q6600 @ 2.40GHz.
#[metastructure(pii = "maybe")]
Copy link
Member

Choose a reason for hiding this comment

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

Not sure I'd mark this one as Maybe. Is this a thing? Where one could use the CPU desc to identify users?

Copy link
Member Author

Choose a reason for hiding this comment

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

This has been a thing with fingerprinting - but not sure either. No harm to keep it maybe then, we can always change it to true or remove entirely afterwards.

Copy link
Member

Choose a reason for hiding this comment

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

yeah I think "fingerprinting" is in scope for "maybe". If there is no attribute it is literally impossible to remove the value and that seems wrong as well.

@AbhiPrasad AbhiPrasad enabled auto-merge (squash) August 8, 2022 13:55
@AbhiPrasad AbhiPrasad merged commit 39f69a7 into master Aug 8, 2022
@AbhiPrasad AbhiPrasad deleted the abhi-update-device-context branch August 8, 2022 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants