Improvements during startup #101
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Manifest Files |
Generated by 🚫 Danger Swift against 4c9d60c |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #101 +/- ##
==========================================
- Coverage 91.95% 91.92% -0.04%
==========================================
Files 408 408
Lines 26447 26438 -9
==========================================
- Hits 24320 24303 -17
- Misses 2127 2135 +8
|
| ) | ||
| } | ||
|
|
||
| self?.lastUpdateTime = Date().timeIntervalSince1970 |
There was a problem hiding this comment.
If updateIfNeeded is called for the first time many times in quick succession its possible that we kick off multiple jobs onto this async queue.
Should we guard against this with the lastUpdateTime or should we introduce a variable to track that an update is currently in progress?
| @@ -30,6 +32,7 @@ public class EmbraceConfig { | |||
| self.notificationCenter = notificationCenter | |||
| self.logger = logger | |||
| self.configurable = configurable | |||
| self.queue = DispatchQueue(label: "com.embrace.config", attributes: .concurrent) | |||
|
|
|||
| update() | |||
There was a problem hiding this comment.
We should move this call to update as it is a side effect of the init method.
Think it makes more sense if we call this in the Embrace.start method in the async job here
| @@ -92,11 +92,11 @@ public class MockEmbraceConfigurable: EmbraceConfigurable { | |||
| } | |||
| } | |||
|
|
|||
| public let updateExpectation = XCTestExpectation(description: "update called") | |||
| public var updateCallCount = 0 | |||
There was a problem hiding this comment.
Curious why you moved away from the XCTestExpectation. Was this causing test failures?
Improves
Embrace.setupby 10ms-20msEmbraceConfigwas unintentionally doing some stuff on the main thread. It is now being dispatched async on a different queue.Improves
Embrace.startby 10ms-20msCaptureServices.startinto 2 methods. Only theinstallneeded to be done on the main thread.Fixes the remote config being fetched twice on startup.