-
Notifications
You must be signed in to change notification settings - Fork 106
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
chore: [#176778467] Track loading services details stats #2785
chore: [#176778467] Track loading services details stats #2785
Conversation
Affected stories
|
Codecov Report
@@ Coverage Diff @@
## master #2785 +/- ##
==========================================
- Coverage 54.03% 53.98% -0.06%
==========================================
Files 804 804
Lines 22203 22234 +31
Branches 4170 4180 +10
==========================================
+ Hits 11997 12002 +5
- Misses 10150 10176 +26
Partials 56 56
Continue to review full report at Codecov.
|
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.
@Undermaken could you add SERVICES_DETAIL_LOADING_STATS
to the mixpanel list with the possible payload parameters?
@fabriziofff |
startTime: new Date().getTime() as Millisecond, | ||
servicesId: new Set([...action.payload]), | ||
loaded: 0, | ||
toLoad: servicesDetailLoadTrack.servicesId.size |
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.
toLoad: servicesDetailLoadTrack.servicesId.size | |
toLoad: action.payload.length |
With the current implementation toLoad
is always 0
.
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.
that's a bug, due to refactoring!
Good catch!
fixed
29baf38
trackServicesDetailLoad({ | ||
...servicesDetailLoadTrack, | ||
kind: "COMPLETE", | ||
loadingTime: (new Date().getTime() - | ||
servicesDetailLoadTrack.startTime) as Millisecond | ||
}); |
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.
With some multiple tries on Android, I ran into this case (kind
PARTIAL):
My proposal:
create a common function to calculate the loading time
const calculateLoadingTime = (startTime: Millisecond): Millisecond =>
(startTime !== 0 ? new Date().getTime() - startTime : 0) as Millisecond;
and use it at line 147
and 163
.
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.
Maybe this scenario is when the app changes state but actually it is not loading any service
I add your suggestion and a condition to send "PARTIAL"
only when there is a services loading running
Short description
This PR is useful to keep track of some measurements about loading services details
It is strictly related to this workaround about services loading
This tracking is based on a "in memory" stats tracking: the usage of the store (actions + reducer) adds a certain overhead and needs extra code to cover all cases