RUM-4516 flag critical events in custom persistence#2044
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2044 +/- ##
===========================================
- Coverage 83.18% 82.97% -0.21%
===========================================
Files 493 494 +1
Lines 17691 17709 +18
Branches 2681 2682 +1
===========================================
- Hits 14716 14693 -23
- Misses 2238 2274 +36
- Partials 737 742 +5
|
0xnm
left a comment
0xnm
left a comment
There was a problem hiding this comment.
Overall the change looks clear to me, I left one question.
Although maybe alternative to this would be just adding something like PersistenceStrategy#flush method which could be called when crash happens, this could reduce change layout.
Or, maybe, we could say that if custom persistence strategy is used, then user is responsible for it, meaning they should register their own unhandled exception handler (but, probably, this is not a great integration experience).
| */ | ||
| @WorkerThread | ||
| fun write(writer: EventBatchWriter, element: T): Boolean | ||
| fun write(writer: EventBatchWriter, element: T, eventType: EventType): Boolean |
There was a problem hiding this comment.
what do you think about adding a default value here eventType: EventType = EventType.DEFAULT in order to reduce the change layout and make this change backward compatible (just in case)?
There was a problem hiding this comment.
Well that's a thought but I'm trying to limit the use of default values when it's not necessary.
IMHO, using default values makes sense for public APIs our customer might use, but for our internals, it's a source of errors since we could get lazy and not set a value we're supposed to set.
|
@0xnm regarding your idea to provide a |
4 participants
What does this PR do?
Adds an additional flag when writing event to let storage handle those differently.
Motivation
Because some users with custom PersistenceStrategy use mainly a memory based storage, they need to know when the app is about to crash in order to persist the data in a way that can be retrieved after an app reboot.
Additional Notes
An additional type was created for telemetry, just to be able to distinguish it.