[Wallet] Adding time metrics to sync and tx events, also new events. #3810
Conversation
jio-gl
changed the title
packages/mobile/src/web3/saga.ts
Outdated
| @@ -77,13 +79,17 @@ export function* checkWeb3SyncProgress() { | |||
| if (latestBlock && latestBlock.number > 0) { | |||
| yield put(completeWeb3Sync(latestBlock.number)) | |||
| Logger.debug(TAG, 'checkWeb3SyncProgress', 'Sync is complete') | |||
| millisecs = Date.now() - initTime | |||
| CeloAnalytics.track(CustomEventNames.sync_complete, { millisecs }) | |||
There was a problem hiding this comment.
This method gets called potentially many times throughout the lifetime of the app. We probably don't want to spam out events for each micro-sync right? We should find a different strategy for tracking sync time
There was a problem hiding this comment.
We can do a Moving Average every N micro-sync and then post every M micro-sync. What do you think.
There was a problem hiding this comment.
I did it now with only one parameter: N = M.
There was a problem hiding this comment.
For example track() is called on start sync and once every 100 syncs. Mean and spot time elapsed since sync start is reported.
Codecov Report
@@ Coverage Diff @@
## master #3810 +/- ##
=========================================
Coverage ? 75.69%
=========================================
Files ? 303
Lines ? 8855
Branches ? 1106
=========================================
Hits ? 6703
Misses ? 2016
Partials ? 136
Continue to review full report at Codecov.
|
|
For loading geth and sync registering only a few events 1 of 100 and also moving average, for tx still all events. |
jmrossy
changed the title
packages/mobile/src/web3/saga.ts
Outdated
| @@ -75,6 +81,13 @@ export function* checkWeb3SyncProgress() { | |||
|
|
|||
| const latestBlock: Block = yield call(getLatestBlock) | |||
| if (latestBlock && latestBlock.number > 0) { | |||
| ;[meanMillisecs, trackCount] = trackMeanMillisecs( | |||
There was a problem hiding this comment.
If I'm reading this correctly, this is the only event function we need to use trackMeanMilliseconds for, right?
There was a problem hiding this comment.
okey, CustomEventNames.sync_complete then.
There was a problem hiding this comment.
okey, perfect -> CustomEventNames.sync_complete
packages/mobile/src/geth/geth.ts
Outdated
| meanMillisecs, | ||
| trackCount, | ||
| TRACK_EVERY_GETH, | ||
| CustomEventNames.geth_failed_genesis_block |
There was a problem hiding this comment.
For these kinds of events - I'm not sure we would expect this to happen very often, so I don't think we really need to call this function, right?
There was a problem hiding this comment.
Yep, these are more abnormal conditions.
There was a problem hiding this comment.
Okey, good idea, then only CustomEventNames.sync_complete
There was a problem hiding this comment.
Nice work! Left a couple of comments. It seems like you are mostly tracking duration with your timers but FYI we have timestamps included by default and can calculate the duration between any two events by subtracting the timestamps.
Also worth nothing that we have a trackSubEvents method that allows easy grouping of related events and will track duration between subEvents for us.
I am measuring the time with regard to the init of a task, is not between two events. I am only submitting one event per task. |
| const latestBlock: Block = yield call(getLatestBlock) | ||
| if (latestBlock && latestBlock.number > 0) { | ||
| yield put(completeWeb3Sync(latestBlock.number)) | ||
| web3SyncAnalyticsTracker.log(syncStartTime) |
There was a problem hiding this comment.
It seems like web3SyncAnalyticsTracker is only used here, so the data is never submitted. Would it be possible to just remove it and use the existing CeloAnalytics?
|
@timmoreton, @joigno and I have done some work on geth analytics instrumentation but it recently occurred to me that this is probably a subset of what you are working on |
Description
Adding time elapsed to Sync Event and Tx Events.
Tested
Manually on Debug console.
Related issued
Part of #3653
Backwards compatibility
Some event names where changed, this is not compatible with existing data on Analytics.