Diagnostics: limit events sent#793
Conversation
There was a problem hiding this comment.
Note that we currently perform this deleting + sending diagnostics event when syncing, not tracking. This means the file can potentially exceed the max number of events but it shouldn't be that much if we sync every time the SDK is configured. Also, note that if we fail syncing 3 times, we simply delete the whole file, so it shouldn't grow that much even if it keeps failing syncing. Lmk what you think!
There was a problem hiding this comment.
I separated this logic into this method to be able to send the MAX_EVENTS_STORED_LIMIT_REACHED event in the same sync we detect we've reached the limit. If we added tracking the event to the queue, we would sync the events that existed before, then add the event to the diagnostics file, so we would miss a sync for tracking these events.
Codecov Report
@@ Coverage Diff @@
## diagnostics #793 +/- ##
==============================================
Coverage ? 81.97%
==============================================
Files ? 127
Lines ? 4150
Branches ? 521
==============================================
Hits ? 3402
Misses ? 542
Partials ? 206 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
There was a problem hiding this comment.
does this method need to be synchronized?
There was a problem hiding this comment.
Hmm I would say with our current calls the chances of a race condition are 0 but for the sake of completeness, you're correct 👍
There was a problem hiding this comment.
Actually, I've been thinking that while we can make this class support multi-threading, considering we are using the same thread for everything related to diagnostics that would complicate the class without need. I improved the docstring for DiagnosticsManager but lmk if you still think we should future-proof it against multi-threading.
There was a problem hiding this comment.
YAGNI 👍🏻 sometimes the best way to make things thread safe is to just document that a type can't be used from multiple threads and deal with that outside of the class, keeps things much simpler.
a6873d9 to
51d8760
Compare
03e044b to
fa7ab39
Compare
de85a24 to
a5f1219
Compare
…795) ### Description Based on #793 This PR refactors previous code to split the `DiagnosticsManager` into `DiagnosticsTracker` and `DiagnosticsSynchronizer`. This is a prerequisite to avoid circular dependencies when tracking request data. - Before: `DiagnosticsManager`->`Backend`->`HttpClient`->`DiagnosticsManager`. - After: `DiagnosticsSynchronizer`->`Backend` for syncing and `HttpClient`->`DiagnosticsTracker` for tracking requests I could have tried to extract this logic from the HttpClient, but that would be a bigger refactor. This was the simplest solution I thought for this problem. Also, sorry I didn't see that issue before. I didn't modify the #785 PR directly to try to avoid some conflicts that would happen if I did it there directly.
…795) ### Description Based on #793 This PR refactors previous code to split the `DiagnosticsManager` into `DiagnosticsTracker` and `DiagnosticsSynchronizer`. This is a prerequisite to avoid circular dependencies when tracking request data. - Before: `DiagnosticsManager`->`Backend`->`HttpClient`->`DiagnosticsManager`. - After: `DiagnosticsSynchronizer`->`Backend` for syncing and `HttpClient`->`DiagnosticsTracker` for tracking requests I could have tried to extract this logic from the HttpClient, but that would be a bigger refactor. This was the simplest solution I thought for this problem. Also, sorry I didn't see that issue before. I didn't modify the #785 PR directly to try to avoid some conflicts that would happen if I did it there directly.
…795) ### Description Based on #793 This PR refactors previous code to split the `DiagnosticsManager` into `DiagnosticsTracker` and `DiagnosticsSynchronizer`. This is a prerequisite to avoid circular dependencies when tracking request data. - Before: `DiagnosticsManager`->`Backend`->`HttpClient`->`DiagnosticsManager`. - After: `DiagnosticsSynchronizer`->`Backend` for syncing and `HttpClient`->`DiagnosticsTracker` for tracking requests I could have tried to extract this logic from the HttpClient, but that would be a bigger refactor. This was the simplest solution I thought for this problem. Also, sorry I didn't see that issue before. I didn't modify the #785 PR directly to try to avoid some conflicts that would happen if I did it there directly.
…795) ### Description Based on #793 This PR refactors previous code to split the `DiagnosticsManager` into `DiagnosticsTracker` and `DiagnosticsSynchronizer`. This is a prerequisite to avoid circular dependencies when tracking request data. - Before: `DiagnosticsManager`->`Backend`->`HttpClient`->`DiagnosticsManager`. - After: `DiagnosticsSynchronizer`->`Backend` for syncing and `HttpClient`->`DiagnosticsTracker` for tracking requests I could have tried to extract this logic from the HttpClient, but that would be a bigger refactor. This was the simplest solution I thought for this problem. Also, sorry I didn't see that issue before. I didn't modify the #785 PR directly to try to avoid some conflicts that would happen if I did it there directly.
Description
Based on #785
Deals with CSDK-648
In this PR we add a limit to the number of events that can be sent to the backend and we add a diagnostics event tracking when this happens.