NR-186415 rewrote the persistence in the new event system #190
Conversation
|
Great rewrite, persistent store looks SO much better. Running NRTestApp locally with this changes will add comments on code if issues found. |
…t the stress tests to pass with it.
| self = [super init]; | ||
| if (self) { | ||
| events = [[NSMutableArray<NRMAAnalyticEventProtocol> alloc] init]; | ||
| maxBufferSize = kDefaultBufferSize; | ||
| events = [NRMAEventManager getLastSessionEventsArray]; |
There was a problem hiding this comment.
Should this be behind a feature flag to only happen when offline storage is enabled?
There was a problem hiding this comment.
I think it is a good idea to have this as a feature flag but this is separate from offline storage.
Persistence was originally intended to create an event trail for handledExceptions. Now because we believed that events were suppose to persist back into an eventManager and Android agreed they added that and now we should too. This accomplishes the goal of restoring events of an app session that ended unexpectedly.
The way that offline storage currently is CDD'd to work is by storing the data of failed network request into separate files and then sending them all as soon as we send a successful request. There are talks about combining these two things but I haven't heard anything conclusive.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #190 +/- ##
===========================================
+ Coverage 76.68% 76.77% +0.08%
===========================================
Files 167 179 +12
Lines 11308 13003 +1695
===========================================
+ Hits 8672 9983 +1311
- Misses 2636 3020 +384 ☔ View full report in Codecov by Sentry. |
|
Is this PR still valid or needed to be merged or should it be closed? |
|
This PR is half valid. I think the NRMASAM being changed to use the PersistentEventStore is something we want. There are also some other smaller changes that are valid to. However I'm not sure restoring unharvested events to a new session is something we want to do. |
The goal of this pr is to add persisted events back to the event manager if they haven't been sent.
I wanted to combine the two events and session attributes persistence layers into one class.
Also I wanted to pass all the tests including the stress tests without the 30 second write time to better match the original.
I have more to write coming soon...