Add spannable tracking around SyncResponseHandler #7514
Conversation
amitkma
requested review from
a team and
mnaturel
and removed request for
a team
| @@ -17,12 +17,16 @@ | |||
| package org.matrix.android.sdk.api.extensions | |||
There was a problem hiding this comment.
I wonder why this class is called MetricsExtensions and is in the package extensions? It seems to me there is no extension methods in this file. Maybe we should rename and move it somewhere else?
There was a problem hiding this comment.
Yeah, you are right. Initially, I was thinking of making them as extension of MetricsPlugin but now they are just functions so we need to rename it.
| Timber.v("Should start cryptoService") | ||
| cryptoService.start() | ||
| val relevantPlugins = metricPlugins.filterIsInstance<SyncDurationMetricPlugin>() | ||
| measureMetric(relevantPlugins) { |
There was a problem hiding this comment.
I think we should try to improve readability of this code, what about extracting all the steps in dedicated small methods. And we could also avoid adding comments using good naming for these methods. For example:
measureMetric(relevantPlugins) {
startCryptoService()
handleToDevice()
...
}
| } | ||
|
|
||
| private fun markCryptoSyncCompleted(syncResponse: SyncResponse) { | ||
| // "crypto_sync_handler_onSyncCompleted" span |
There was a problem hiding this comment.
I wonder if it is useful to keep these comments about spans since now we have dedicated private methods?
| * @param block Action/Task to be executed within this span. | ||
| */ | ||
| @OptIn(ExperimentalContracts::class) | ||
| inline fun List<MetricPlugin>.measureMetric(block: () -> Unit) { |
There was a problem hiding this comment.
Yes I think we should update to avoid any confusion.
|
@amitkma When testing the last build I am blocked at a certain step during initial sync (see capture below). Do you reproduce this issue as well? Maybe something related to the refactoring?
|
Yes I do see it and I am looking into it. One of our test is also failing due to this issue. |
|
SonarCloud Quality Gate failed. |
Type of change
Content
SyncDurationMetricPluginis added.Motivation and context
sync_response_handleris one of such metrics.Tests
download_device_keysincreasing here.Tested devices
Checklist
- [ ] UI change has been tested on both light and dark themes- [ ] Accessibility has been taken into account. See https://github.com/vector-im/element-android/blob/develop/CONTRIBUTING.md#accessibility- [ ] Pull request includes screenshots or videos if containing UI changes- [ ] Pull request includes a sign off- [ ] If you have modified the screen flow, or added new screens to the application, you have updated the test UiAllScreensSanityTest.allScreensTest()