Skip to content

feat(schema): Add missing fields to DeviceContext#1383

Merged
AbhiPrasad merged 5 commits intomasterfrom
abhi-update-device-context
Aug 8, 2022
Merged

feat(schema): Add missing fields to DeviceContext#1383
AbhiPrasad merged 5 commits intomasterfrom
abhi-update-device-context

Conversation

@AbhiPrasad
Copy link
Contributor

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 a team and bruno-garcia 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
Contributor 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
Contributor 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
Contributor 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