-
Notifications
You must be signed in to change notification settings - Fork 74
RUM-11784: Send TTID vital #2944
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-11784: Send TTID vital #2944
Conversation
d6e3cee
to
291bdbd
Compare
🎯 Code Coverage 🔗 Commit SHA: fcd356b | Docs | Was this helpful? Give us feedback! |
291bdbd
to
ca35bfa
Compare
ca35bfa
to
928d6c2
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## aleksandr-gringauz/feature/app-launch-vitals #2944 +/- ##
================================================================================
+ Coverage 71.02% 71.08% +0.06%
================================================================================
Files 829 830 +1
Lines 30381 30447 +66
Branches 5183 5186 +3
================================================================================
+ Hits 21578 21642 +64
- Misses 7347 7356 +9
+ Partials 1456 1449 -7
🚀 New features to boost your workflow:
|
...android-rum/src/main/kotlin/com/datadog/android/rum/internal/domain/scope/RumSessionScope.kt
Show resolved
Hide resolved
...android-rum/src/main/kotlin/com/datadog/android/rum/internal/domain/scope/RumSessionScope.kt
Outdated
Show resolved
Hide resolved
eventAttributes = emptyMap(), | ||
customAttributes = getCustomAttributes(), | ||
view = null, | ||
hasReplay = hasReplay, |
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'm not sure if we need to set this flag at all here. Yes, there is such property, but it is only app start, so no real view yet, hence nothing to replay. We have view = null
above anyway. WDYT?
If we don't set this property, we even simplify the code, because there is not need to pass FeaturesContextResolver
around.
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.
Good question, I'm not sure either.
Let's consider the situation when the customer uses ActivityViewTrackingStrategy.
- The view starts in onResume.
- The first draw (and TTID Vital event as a result) will happen slightly (less than a second) after this, so technically the view already exists.
- The "has_replay" flag for a view that we check here is set here. This happens when a session replay event is recoded here.
- In practice I've never seen
hasReplay = true
in my implementation. As far as I understood the session replay event isn't written fast enough. However if you delay the TTID for 5 secondshasReplay
will be true. We have a race here. - To wrap up. In practice this
hasReplay
in my case is kind of useless and will in most cases befalse
. I can just passhasReplay = null
, I don't think it will be a problem.
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 think having hasReplay = null
here is fine. This property is just a sticky flag which is reduced over view updates and pulled to the session level. So basically once it is set, it means that the given session has Session Replay. It means that if any other view (presumably the real one) update delivers it, this will 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.
changed to hasReplay = null
and simplified the code.
...oid-rum/src/test/kotlin/com/datadog/android/rum/internal/domain/scope/RumSessionScopeTest.kt
Show resolved
Hide resolved
...oid-rum/src/test/kotlin/com/datadog/android/rum/internal/domain/scope/RumSessionScopeTest.kt
Show resolved
Hide resolved
...oid-rum/src/test/kotlin/com/datadog/android/rum/internal/domain/scope/RumSessionScopeTest.kt
Outdated
Show resolved
Hide resolved
3549b6e
into
aleksandr-gringauz/feature/app-launch-vitals
What does this PR do?
This PR implements sending of TTID value using Rum
VitalEvent
withApplicationLaunchProperties
. Merging into a feature branch for now.Motivation
What inspired you to submit this pull request?
Additional Notes
Anything else we should know when reviewing?
Review checklist (to be filled by reviewers)