-
Notifications
You must be signed in to change notification settings - Fork 65
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
[RUM-8654] Slow frames collection support #2518
Conversation
...rum/src/main/kotlin/com/datadog/android/rum/internal/metric/slowframes/SlowFramesListener.kt
Show resolved
Hide resolved
currentViewStartedTimeStampNs = startedTimestampNs | ||
} | ||
|
||
override fun resolveReport(viewId: String): ViewUIPerformanceData { |
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.
This function name is not as indicative enough as onViewCreated
to explain where it should be called. (especially you haven't call it yet in this PR). should it be called when the rum view scope ends?
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.
this approach follows common one,
in InteractionToNextViewMetricResolver and NetworkSettledMetricResolver
How do you think we should call it?
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.
I see, because I didn't find any call site of this function (because you haven't finish the whole feature yet), I am trying to understand where it should be called.
...rum/src/main/kotlin/com/datadog/android/rum/internal/metric/slowframes/SlowFramesListener.kt
Outdated
Show resolved
Hide resolved
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.
I went through the production code so far and left some comments/questions. Didn't check test code yet.
dd-sdk-android-core/src/main/kotlin/com/datadog/android/core/collections/EvictingQueue.kt
Outdated
Show resolved
Hide resolved
dd-sdk-android-core/src/main/kotlin/com/datadog/android/core/collections/EvictingQueue.kt
Outdated
Show resolved
Hide resolved
dd-sdk-android-core/src/main/kotlin/com/datadog/android/core/collections/EvictingQueue.kt
Outdated
Show resolved
Hide resolved
...rum/src/main/kotlin/com/datadog/android/rum/internal/metric/slowframes/SlowFramesListener.kt
Outdated
Show resolved
Hide resolved
...rum/src/main/kotlin/com/datadog/android/rum/internal/metric/slowframes/SlowFramesListener.kt
Outdated
Show resolved
Hide resolved
...rum/src/main/kotlin/com/datadog/android/rum/internal/metric/slowframes/SlowFramesListener.kt
Outdated
Show resolved
Hide resolved
...rum/src/main/kotlin/com/datadog/android/rum/internal/metric/slowframes/SlowFramesListener.kt
Outdated
Show resolved
Hide resolved
...rum/src/main/kotlin/com/datadog/android/rum/internal/metric/slowframes/SlowFramesListener.kt
Outdated
Show resolved
Hide resolved
Question: in the description it says part of the implementation has not been done yet, should this PR targets to a feature branch instead of develop branch then? |
...rum/src/main/kotlin/com/datadog/android/rum/internal/metric/slowframes/SlowFramesListener.kt
Outdated
Show resolved
Hide resolved
...rum/src/main/kotlin/com/datadog/android/rum/internal/metric/slowframes/SlowFramesListener.kt
Outdated
Show resolved
Hide resolved
...rum/src/main/kotlin/com/datadog/android/rum/internal/metric/slowframes/SlowFramesListener.kt
Outdated
Show resolved
Hide resolved
ad51807
to
acf3ed2
Compare
I am gonna merge this PR after new version release, but there is no core changes so should be fine |
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.
nice work! I left some questions, but nothing blocking.
dd-sdk-android-internal/src/main/java/com/datadog/android/internal/collections/EvictingQueue.kt
Show resolved
Hide resolved
...rum/src/main/kotlin/com/datadog/android/rum/internal/domain/state/ViewUIPerformanceReport.kt
Outdated
Show resolved
Hide resolved
...t/kotlin/com/datadog/android/rum/internal/metric/slowframes/DefaultSlowFramesListenerTest.kt
Outdated
Show resolved
Hide resolved
...t/kotlin/com/datadog/android/rum/internal/metric/slowframes/DefaultSlowFramesListenerTest.kt
Outdated
Show resolved
Hide resolved
ec95a18
to
b2ae399
Compare
1e7d5f4
to
2778cc6
Compare
features/dd-sdk-android-rum/src/main/kotlin/com/datadog/android/rum/RumConfiguration.kt
Show resolved
Hide resolved
features/dd-sdk-android-rum/src/main/kotlin/com/datadog/android/rum/RumConfiguration.kt
Outdated
Show resolved
Hide resolved
features/dd-sdk-android-rum/src/main/kotlin/com/datadog/android/rum/RumConfiguration.kt
Show resolved
Hide resolved
features/dd-sdk-android-rum/src/main/kotlin/com/datadog/android/rum/RumConfiguration.kt
Outdated
Show resolved
Hide resolved
...-rum/src/main/kotlin/com/datadog/android/rum/configuration/SlowFrameListenerConfiguration.kt
Outdated
Show resolved
Hide resolved
...-rum/src/main/kotlin/com/datadog/android/rum/configuration/SlowFrameListenerConfiguration.kt
Outdated
Show resolved
Hide resolved
|
||
fun slowFramesRate(viewEndedTimeStamp: Long): Double = when { | ||
viewEndedTimeStamp - viewStartedTimeStamp <= minViewLifetimeThresholdNs -> 0.0 | ||
totalFramesDurationNs > 0.0 -> slowFramesDurationNs.toDouble() / totalFramesDurationNs |
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.
to me it is not a rate
, but ratio
. Are we sure we are using a correct terminology?
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.
Gonna update event schema first, and then change this in a serate ticket
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.
This is a good point.
We followed the naming used by Apple here:
32297c3
to
d12bf4b
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.
I tentatively approve this PR, but we really need to decide on the lingvo rate
vs ratio
before the next release. The chosen terminology should be:
- easily understandable by the customers
- common in the industry (ideally the same wording used by the Play Store as well)
features/dd-sdk-android-rum/src/main/kotlin/com/datadog/android/rum/RumConfiguration.kt
Outdated
Show resolved
Hide resolved
features/dd-sdk-android-rum/src/main/kotlin/com/datadog/android/rum/RumConfiguration.kt
Outdated
Show resolved
Hide resolved
features/dd-sdk-android-rum/src/main/kotlin/com/datadog/android/rum/RumConfiguration.kt
Outdated
Show resolved
Hide resolved
features/dd-sdk-android-rum/src/test/kotlin/com/datadog/android/rum/internal/RumFeatureTest.kt
Outdated
Show resolved
Hide resolved
features/dd-sdk-android-rum/src/test/kotlin/com/datadog/android/rum/internal/RumFeatureTest.kt
Outdated
Show resolved
Hide resolved
...t/kotlin/com/datadog/android/rum/internal/metric/slowframes/DefaultSlowFramesListenerTest.kt
Outdated
Show resolved
Hide resolved
...ures/dd-sdk-android-rum/src/test/kotlin/com/datadog/android/rum/utils/InternalLoggerUtils.kt
Outdated
Show resolved
Hide resolved
15449be
to
8612f3c
Compare
fc2e06f
to
f942dd6
Compare
What does this PR do?
Adds a SlowFrames listener that collects occurrences of jank frames during a view's lifetime.
This PR introduces the underlying logic and corresponding tests but does not yet integrate the functionality. The remaining integration will be completed in future tickets.
Review checklist (to be filled by reviewers)