2 improvements for new crash reporter #9
Conversation
…tes and other possible errors. Remove spurious using.
|
Sure that's a good thing to avoid. I've got an idea, will post an update soon.
Sep 2, 2024 10:18:43 AM Cristi Pufu ***@***.***>:
…
***@***.**** commented on this pull request.
----------------------------------------
In src/AptabasePersistentClient.cs[#9 (comment)]:
> @@ -19,6 +18,8 @@ public class AptabasePersistentClient : IAptabaseClient
private readonly ILogger<AptabasePersistentClient>? _logger;
private readonly CancellationTokenSource _cts;
+ private bool _pauseProcessing;
I understand, the thing that bothers me about this approach, is that we're leaking crash specific details into the persistent client implementation. I'm wondering if we can find a way to pause/start the client, maintaining the separation
—
Reply to this email directly, view it on GitHub[#9 (comment)], or unsubscribe[https://github.com/notifications/unsubscribe-auth/AMNXYKLLE3BPH6WHQDQB5K3ZURXKDAVCNFSM6AAAAABNPDAJRWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDENZVG43DIOJSGE].
You are receiving this because you authored the thread.
[Tracking image][https://github.com/notifications/beacon/AMNXYKKJFCMUAFAM3ETMTJDZURXKDA5CNFSM6AAAAABNPDAJRWWGG33NNVSW45C7OR4XAZNRKB2WY3CSMVYXKZLTORJGK5TJMV32UY3PNVWWK3TUL5UWJTUHUVVLS.gif]
|
Allow cancellation of the retry delay.
|
Pushed the update, identical semantics but it makes the pause a formal property, and adds thread safety. It removes the leak of recognizing crash event details. Feel free to squash with the earlier ones if you agree. I considered adding the new "Paused" property to both channel types, but it's actually the opposite of what we need for non-persistent channel delivery. For those, the sooner the better! BTW also passed the _cts.Token to the retry delay, so it can be canceled more promptly on any teardown. |
|
@TTalpey but how/when do we resume? |
|
In my proposed PR, the Pause test throws at line 91, which lands in the catch{} at line 105, so - 30 seconds. That's plenty of time to crash, or if it actually picks back up it's enough time to be sure. |
|
Re your other proposal, need to find time to review, it's a lot bigger, and has a new API. Hopefully soon. |
Yes, but it will remain paused if the app doesn't actually crash? |
|
No, it's a temporary pause. Note that fetching "Paused" resets the _paused boolean, so it's self-restarting. This is what the earlier PR did too btw. |
A couple of improvements after further testing of the new merge...
It's important to pause sending persistent client events to the aptabase server when an app is crashing, otherwise duplicates appear in the logs. Often this is the first or second line of the stack trace, which becomes confusing. Restore the "pauseProcessing" behavior in the refactored AptabasePersistentClient.
Also, add some basic DeviceInfo platform information to the initial ("00") crash event, to make it more useful and meaningful in the aptabase UI without having to dig into a full "export".