-
Notifications
You must be signed in to change notification settings - Fork 74
RUM-13468: Add totalRam, logicalCpuCount and isLowRam to DeviceInfo
#3024
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
base: develop
Are you sure you want to change the base?
RUM-13468: Add totalRam, logicalCpuCount and isLowRam to DeviceInfo
#3024
Conversation
a5339c4 to
899ef0f
Compare
totalRam, processorCount and isLowRamDevice to DeviceInfo
65945bf to
55a5484
Compare
This comment has been minimized.
This comment has been minimized.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #3024 +/- ##
===========================================
+ Coverage 71.25% 71.26% +0.01%
===========================================
Files 866 866
Lines 31734 31807 +73
Branches 5360 5360
===========================================
+ Hits 22610 22665 +55
- Misses 7596 7614 +18
Partials 1528 1528
🚀 New features to boost your workflow:
|
55a5484 to
c356214
Compare
c356214 to
a4e01bd
Compare
| "processor_count": { | ||
| "type" : "number", | ||
| "description": "Number of device processors", | ||
| "readOnly": true | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably the naming is not the best one, because processor is a chip containing multiple cores. We probably want to send the number of physical cores, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @0xnm thanks for the review! I’m picking this PR up again, sorry for the delay.
I initially went with processor_count rather than something more precise is because both the Android and iOS APIs use the term processor, even though they effectively return the number of CPU cores:
- Android:
Runtime.getRuntime().availableProcessors() - iOS:
ProcessInfo.processInfo.processorCount
That said, I'm not particularly happy with this name either, as it could be clearer. I was thinking cpu_core_count.
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apple docs say:
processorCount
The number of processing cores available on the computer.
Android docs say:
Returns the number of processors available to the Java virtual machine.
This value may change during a particular invocation of the virtual machine. Applications that are sensitive to the number of available processors should therefore occasionally poll this property and adjust their resource usage appropriately.
Apple docs are more explicit about the cores meaning behind. But given that both are using processors <-> cores lingvo interchangeably, I don't have a strong no against it, so I'm okay with any of cpu_core_count or processor_count. Maybe the former is a bit better for understanding (not sure if we can find any mobile devices with multiple CPU units).
| override val processorCount: Int by lazy(LazyThreadSafetyMode.PUBLICATION) { | ||
| Runtime.getRuntime().availableProcessors() | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if we want to send the number of processors (chips) or cores, but this method returns the number of physical cores and not the number of chips.
| } | ||
| } | ||
|
|
||
| override val processorCount: Int by lazy(LazyThreadSafetyMode.PUBLICATION) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unless we have any performance concerns, we can use default mode
| override val processorCount: Int by lazy(LazyThreadSafetyMode.PUBLICATION) { | |
| override val processorCount: Int by lazy { |
| } | ||
|
|
||
| @Suppress("UnsafeThirdPartyFunctionCall") // Called within a try/catch block | ||
| override val totalRam: Int? by lazy(LazyThreadSafetyMode.PUBLICATION) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| override val totalRam: Int? by lazy(LazyThreadSafetyMode.PUBLICATION) { | |
| override val totalRam: Int? by lazy { |
| } | ||
|
|
||
| @Suppress("UnsafeThirdPartyFunctionCall") // Called within a try/catch block | ||
| override val isLowRamDevice: Boolean? by lazy(LazyThreadSafetyMode.PUBLICATION) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| override val isLowRamDevice: Boolean? by lazy(LazyThreadSafetyMode.PUBLICATION) { | |
| override val isLowRamDevice: Boolean? by lazy { |
| Runtime.getRuntime().availableProcessors() | ||
| } | ||
|
|
||
| @Suppress("UnsafeThirdPartyFunctionCall") // Called within a try/catch block |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor: it is better to move this annotation right to the call site rather than to keep it for the whole method. I guess here it will be getMemoryInfo and ActivityManager.MemoryInfo() constructor can be declared as safe in YAML config.
| val processorCount: Int = forge.anInt() | ||
| val totalRam: Int = forge.anInt() | ||
| val isLowRamDevice: Boolean = forge.aBool() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| val processorCount: Int = forge.anInt() | |
| val totalRam: Int = forge.anInt() | |
| val isLowRamDevice: Boolean = forge.aBool() | |
| val processorCount = forge.anInt() | |
| val totalRam = forge.aNullable { anInt() } | |
| val isLowRamDevice = forge.aNullable { aBool() } |
| totalRam = forge.anInt(), | ||
| isLowRamDevice = forge.aBool() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| totalRam = forge.anInt(), | |
| isLowRamDevice = forge.aBool() | |
| totalRam = forge.aNullable { anInt() }, | |
| isLowRamDevice = forge.aNullable { aBool() } |
| processorCount = datadogContext.deviceInfo.processorCount, | ||
| totalRam = datadogContext.deviceInfo.totalRam, | ||
| isLowRamDevice = datadogContext.deviceInfo.isLowRamDevice |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to add assertions for these new properties in tests for RumViewScope, RumResourceScope, etc.
totalRam, processorCount and isLowRamDevice to DeviceInfototalRam, processorCount and isLowRamDevice to DeviceInfo
a4e01bd to
5235618
Compare
totalRam, processorCount and isLowRamDevice to DeviceInfototalRam, logicalCpuCount and isLowRam to DeviceInfo
5235618 to
43fda1b
Compare
What does this PR do?
Adds new attributes to the Device Info:
ActivityManager.isLowRamDevice)Additional Notes
Review checklist (to be filled by reviewers)