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(mobile): Synthesize device.class based on specs from device context #1895

Merged
merged 11 commits into from
Mar 6, 2023

Conversation

edwardgou-sentry
Copy link
Contributor

@edwardgou-sentry edwardgou-sentry commented Feb 28, 2023

Adds and uses normalize_device_context which creates a new device.class context value by checking a combination of the device family, processor_frequency, processor_count, and memory_size.

The device.class context value will be used in the sentry issues, performance, and discover products for the device classification project (getsentry/sentry#44449).

@edwardgou-sentry edwardgou-sentry requested a review from a team March 1, 2023 17:53
@edwardgou-sentry edwardgou-sentry marked this pull request as ready for review March 1, 2023 17:53
@edwardgou-sentry edwardgou-sentry requested a review from a team March 1, 2023 17:53
@edwardgou-sentry edwardgou-sentry force-pushed the egou/feat/synthesize-device-class-context branch from 56c5bdd to 541fc9d Compare March 1, 2023 18:05
Copy link
Member

@jan-auer jan-auer left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

Comment on lines 220 to 236
} else if device.processor_frequency.value().is_some()
&& device.processor_count.value().is_some()
&& device.memory_size.value().is_some()
{
if device.processor_frequency.value().unwrap() < &2000
|| device.memory_size.value().unwrap() < &(4 * GIB)
|| device.processor_count.value().unwrap() < &8
{
device.class = Annotated::new(DEVICE_CLASS_LOW);
} else if device.processor_frequency.value().unwrap() < &2500
|| device.memory_size.value().unwrap() < &(6 * GIB)
{
device.class = Annotated::new(DEVICE_CLASS_MEDIUM);
} else {
device.class = Annotated::new(DEVICE_CLASS_HIGH);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

You can avoid these unwraps with an if let statement similar to the one above:

Suggested change
} else if device.processor_frequency.value().is_some()
&& device.processor_count.value().is_some()
&& device.memory_size.value().is_some()
{
if device.processor_frequency.value().unwrap() < &2000
|| device.memory_size.value().unwrap() < &(4 * GIB)
|| device.processor_count.value().unwrap() < &8
{
device.class = Annotated::new(DEVICE_CLASS_LOW);
} else if device.processor_frequency.value().unwrap() < &2500
|| device.memory_size.value().unwrap() < &(6 * GIB)
{
device.class = Annotated::new(DEVICE_CLASS_MEDIUM);
} else {
device.class = Annotated::new(DEVICE_CLASS_HIGH);
}
}
} else if let (Some(&freq), Some(&proc), Some(&mem)) = (
device.processor_frequency.value(),
device.processor_count.value(),
device.memory_size.value(),
) {
if freq < 2000 || mem < 4 * GIB || proc < 8 {
device.class = Annotated::new(DEVICE_CLASS_LOW);
} else if freq < 2500 || mem < 6 * GIB {
device.class = Annotated::new(DEVICE_CLASS_MEDIUM);
} else {
device.class = Annotated::new(DEVICE_CLASS_HIGH);
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 Thanks Jan, updated to if let!

@@ -160,6 +160,10 @@ pub struct DeviceContext {
/// Whether location support is available on the device.
pub supports_location_service: Annotated<bool>,

// The performance class of the device, stored as a number.
// This value is synthesized from the device's specs in normalize_device_context.
pub class: Annotated<u64>,
Copy link
Member

@jan-auer jan-auer Mar 2, 2023

Choose a reason for hiding this comment

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

Since the device class is an enumeration with semantic values, can we make it an enum instead? For instance, what should happen when an SDK sends the value 42?

If you want to allow any number, you can introduce a so-called "newtype":

#[derive(Clone, Copy, Debug, Deserialize, Eq, PartialEq Serialize)]
struct DeviceClass(pub u64);

impl DeviceClass {
    const LOW: Self = Self(1);
    const MEDIUM: Self = Self(2);
    const HIGH: Self = Self(3);
}

// use as `DeviceClass::LOW`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey Jan, I went with the newtype that you suggested!
We have plans to support more device class values in the future and theres use in being able to compare the number values so I think this works best for us.

Copy link
Member

@jan-auer jan-auer left a comment

Choose a reason for hiding this comment

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

This will allow clients to send unknown device class values, which we will store without modification. If this is OK for the rest of the product, this PR can be merged.

@edwardgou-sentry edwardgou-sentry merged commit d10d8ac into master Mar 6, 2023
@edwardgou-sentry edwardgou-sentry deleted the egou/feat/synthesize-device-class-context branch March 6, 2023 20:32
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.

2 participants