RUM-8875 Add updateExternalRefreshRate to internal RUM API#2772
Conversation
refs: RUM-8875
| handleEvent(RumRawEvent.UpdatePerformanceMetric(metric, value)) | ||
| } | ||
|
|
||
| override fun updateExternalRefreshRate(frameTimeSeconds: Double) { |
There was a problem hiding this comment.
how often this method will be called? is it on the scale of every few milliseconds?
There was a problem hiding this comment.
No, I think it would be unwise of us to call it every frame. My initial plan was that the Unity SDK would respect the VitalsUpdateFrequency setting that (presumably) configures the internal vital monitors, so depending on that setting, it would be called roughly every 100ms, every 500ms, every 1000ms, or never.
(It may also be called immediately before stopView, in the event that we're stopping a view in which we've never called updateExternalRefreshRate.)
| argumentCaptor<ViewEvent> { | ||
| verify(mockWriter).write(eq(mockEventBatchWriter), capture(), eq(EventType.DEFAULT)) | ||
| assertThat(lastValue) | ||
| .hasRefreshRateMetric(expectedRefreshRate, expectedRefreshRate) |
There was a problem hiding this comment.
minor: maybe it is better use named arguments here to avoid questions why both arguments are the same
| min = kotlin.math.min(min, refreshRate) | ||
| max = kotlin.math.max(max, refreshRate) |
There was a problem hiding this comment.
| min = kotlin.math.min(min, refreshRate) | |
| max = kotlin.math.max(max, refreshRate) | |
| min = min(min, refreshRate) | |
| max = max(max, refreshRate) |
and the add the imports for kotlin.math.min and kotlin.math.max.
Alternatively coerceAtMost / coerceAtLeast methods can be used.
| // WHEN - Add external refresh rate data | ||
| testedScope.handleEvent( | ||
| RumRawEvent.UpdateExternalRefreshRate(externalFrameTime), | ||
| mockWriter | ||
| ) | ||
|
|
||
| // AND - Add internal refresh rate data (should be ignored) | ||
| vitalListener.onVitalUpdate(VitalInfo(1, internalRefreshRate, internalRefreshRate, internalRefreshRate)) | ||
|
|
||
| val result = testedScope.handleEvent( | ||
| RumRawEvent.KeepAlive(), | ||
| mockWriter | ||
| ) | ||
|
|
||
| // THEN - Should use external data, not internal |
There was a problem hiding this comment.
minor: we can probably remove comments from When/And/Then as it is pretty much clear from the code itself.
Same for below.
| val externalFrameTime = forge.aDouble(min = 0.016, max = 0.017) // ~60 FPS | ||
| val expectedExternalRefreshRate = 1.0 / externalFrameTime | ||
|
|
||
| val internalRefreshRate = forge.aDouble(min = 30.0, max = 45.0) // Different range |
There was a problem hiding this comment.
probably we can put some very wide range here since it is about priority? so range doesn't probably matter. Otherwise min = 0.016, max = 0.017 looks a bit questionable.
Same can be applied for the tests below.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2772 +/- ##
===========================================
+ Coverage 69.79% 69.86% +0.07%
===========================================
Files 824 824
Lines 30877 30894 +17
Branches 5196 5201 +5
===========================================
+ Hits 21548 21582 +34
+ Misses 7852 7844 -8
+ Partials 1477 1468 -9
🚀 New features to boost your workflow:
|
|
Thanks for the review, @0xnm! I tweaked the new unit tests as you recommended. |
5 participants
refs: RUM-8875
What does this PR do?
This PR adds a new
updateExternalRefreshRatefunction to the internalAdvancedRumMonitorinterface, then updatesRumViewScopeso that it will populaterefresh_rate_averageand related properties based on the data received via this API function, if any such calls have been made.If no calls have been made to
updateExternalRefreshRatein the context of the current view,RumViewScopewill continue to use the data collected fromframeRateVitalListeneras before.Regardless of whether any calls are made to
updateExternalRefreshRate, the vitals monitoring system withincom.datadog.android.rum.internal.vitalswill continue operating exactly as it does now. We make no attempt to disable or otherwise interfere with theframeRateVitalListener.Motivation
Per RUM-8875:
FPSVitalListenerusesJankStatsto sample framerate metrics, but this approach does not work with Unity on Android, as Unity manages its rendering pipeline internally in a way that doesn't expose frame timing information toJankStats. As a result, Unity apps simply do not receive useful data forrefresh_rate_averageet al. in RUM View events.Providing a new
updateExternalRefreshRatewill allow the Unity SDK to periodically self-report frame timing data collected from the Unity runtime, ensuring that the Android SDK can populate refresh-rate-related View properties with useful data.Additional Notes
I'm new to this codebase and I'm not terribly familiar with Kotlin, so please feel free to treat these changes with extra scrutiny.
Summary of code changes
AdvancedRumMonitor.kt: Addfun updateExternalRefreshRate(frameTimeSeconds: Double)toAdvancedRumMonitorinterfaceapiSurface: Regenerateddd-sdk-android-rum.api: Regenerated_RumInternalProxy.kt: Proxy calls toAdvancedRumimplRumRawEvent.kt: DefineUpdateExternalRefreshRateeventRumViewManagerScope.kt: Register that event insilentOrphanEventTypes, so that calls toupdateExternalRefreshRatemade when no view is active will be dropped without errorRumViewScope.kt:externalRefreshRateInfoas a nullableVitalInfopropertyUpdateExternalRefreshRateevents by callingonUpdateExternalRefreshRateonUpdateExternalRefreshRate, set or updateexternalRefreshRateInfoproperty based on supplied value, guarding against divide-by-zeroexternalRefreshRateInfofor refresh rate metrics if set; uselastFrameRateInfootherwiseDatadogRumMonitor.kt:updateExternalRefreshRatecalls by constructing anUpdateExternalRefreshRateevent and passing it tohandleEventSummary of tests added
RumInternalProxyTest.kt: Add a test to exercise proxying of internal API calls for new functionRumRawEventExt.kt: Ensure that new event type is included inForge.anyRumEventRumViewScopeTest.kt: Add test coverage for changes toRumViewScope, asserting that:updateExternalRefreshRateand the internalJankStatslistener, the external data will be preferredJankStatsdata is present, it will be used as normalReview checklist (to be filled by reviewers)