-
Notifications
You must be signed in to change notification settings - Fork 50
[RUM-11606] [V3] JS Refresh Rate normalization to 0-60 fps #1018
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: feature/v3
Are you sure you want to change the base?
[RUM-11606] [V3] JS Refresh Rate normalization to 0-60 fps #1018
Conversation
8384763
to
6fd6016
Compare
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.
Great work 🚀
Two questions:
- Is this aligned 1:1 with the native SDKs logic?
- Have you manually tested this?
* @param fpsBudget: The maximum fps under which the frame Time will be normalized [0-fpsBudget]. Defaults to 60Hz. | ||
* @param deviceDisplayFps: The maximum fps supported by the device. If not provided it will be set from the value obtained from the app context. | ||
*/ | ||
@Suppress("ComplexMethod") |
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.
Quick heads-up: once we rebase develop
on feature/v3
, because of a bump in the detekt plugin version needed for RN 0.82 and Gradle 9 support, this will have to be changed to CyclomaticComplexMethod
, or it won't be picked up and checks will fail.
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.
Thanks for raising!
I've already changed it here: 6e58ddf
Yes, this logic follows the one performed on the native side. The only main difference is that native SDKs report fps metrics instead of frame times. I've decided to keep frame time on the RN SDK as that's the metric that was already defined and exposed by the native SDK for the RN side to report to the backend:
Yes, I have tested it manually on iOS and Android devices, Both running on old and new architecture. |
6fd6016
to
6e58ddf
Compare
What does this PR do?
This PR adds a normalization step to the tracking of the js refresh rate metric, making it so that all reported values fall within the 0-60hz bracket.
I've added a convenience function that will handle normalization as well as obtaining the device capabilities and then normalize the input frame time accordingly.
This is a breaking change, as once users update to V3 all their js refresh rates will get normalized.
NOTE: The metric provider for the RN SDK works with frame times in seconds and they are reported as such to the backend, where they are turned into FPS metrics, that's the reason why the normalization function works with frame times and not fps values.
Motivation
As we state on our Mobile Vitals documentation:
This, however, was not correct for js refresh rates, which we being reported without normalization, so devices capable of higher refresh rates than 60hz could report values above that bracket. This was confusing and not consistent with the expectations laid out by the documentation, so we needed to fix it.
Review checklist (to be filled by reviewers)