-
-
Notifications
You must be signed in to change notification settings - Fork 64
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Automatic Error Collection #603
Conversation
The server running is using a local dinky server, pls no abooze. TODO: Implement Sentry catcher to ALL EXCEPTION HOLY THAT IS A LOT
`USEREMOTELOGGING`, automatically enabled on Preview & Debug branches for obvious reasons. update logger types
Does not initialize or de-initialize the utility. Note: An app restart may be required when selecting the option
sends it to Sentry so we don't have to refactor all our exception calls
Move SentrySDK init to its own helper
…nts' into sentry-error-collection
This added current log for when exception is sent to Dsn
1. Redacted username in the log file when its not debug build 2. Properly dispose SentrySdk when toggled off 3. Pipe SentrySdk log to Logger 4. Use correct flusher for SentryHelper.cs 5. Fixed cleaner loop for ExceptionHandler_ForLoop by @neon-nyan Co-authored-by: Kemal Setya Adhi <[email protected]>
No need for adding Sentry's ExceptionHandler on catcher that has SendException already
Qodana for .NET17 new problems were found
💡 Qodana analysis was run in the pull request mode: only the changed files were checked Contact Qodana teamContact us at [email protected]
|
Note: - Some submodules cannot import SentryHelper, which is in `Hi3Helper.Core` - Some calls are asynchronous, mostly for the handled exceptions, but also where it felt best to use them, feel free to change them if needed - Most are categorized as `UnhandledOther`, but if we feel the need to change some, we can, it simply affects tagging on the frontend dashboard.
Awaiting resolution for getsentry/sentry-dotnet#3747 |
1. Only uploads log file when exception is unhandled 2. Add cache dir path using Collapse's LocalLow data path 3. Dispose _loopToken before creating a new one to prevent memory leak 4. Reordered LogWriteLine on initialization 5. Release exception redirect event when feature is disabled
Also add a way to disable log uploading. This is due to a bug(?) where log upload length reported as 0 and causes BadRequest error. See getsentry/sentry-dotnet#3747
Ready for review Notes:
|
Alternatively, we could try to apply for Sentry's OSS program https://sentry.io/for/open-source/
Please anyone (devs or users) to state your opinion in this thread |
I think IMO it's worth applying for Sentry's OSS program, simply because we're using their SDK. A solution could be that we have 2 endpoints, 1 primary and 1 fallback, but at that point we'd need to implement more checks to select 1 automatically, and that's 2 different platforms so I don't know how pertinent that is. Overall, applying is worth it, we lose nothing by trying. For point 2, I don't think it's a big deal, we also have Edit: Some notable benefits for using Sentry is that we won't need to rely on our own equipment which brings the added benefit that we'll have 100% uptime for the feature. |
1. Use Sentry's own backend Still at testing phase, if any change happens it will be changed again to our self-hosted instance. 2. Add a way to disable via system environment variable Setting env var "DISABLE_SENTRY" to true or 1 will disable exception handling 3. Omit IP address from being sent 4. Regionalized SentryHandler.cs 5. Updated PRIVACY.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are lots of lines that need the type check to avoid OperationCancelledException
or TaskCancelledException
spamming Sentry. Other than that, looks fine for me.
throw ex.Flatten().InnerExceptions.First(); | ||
var innerExceptionsFirst = ex.Flatten().InnerExceptions.First(); | ||
SentryHelper.ExceptionHandler(innerExceptionsFirst, SentryHelper.ExceptionType.UnhandledOther); | ||
throw innerExceptionsFirst; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think throwing AggregateException
into Sentry will be a good idea since it can actually be another type exception like OperationCancelledException
or TaskCancelledException
. In my honest opinion, I think we need to check the innerExceptionsFirst
first to check if it's no other than OperationCancelledException
or TaskCancelledException
, then just throw it. Other than that, just ignore it
throw ex.Flatten().InnerExceptions.First(); | ||
var innerExceptionsFirst = ex.Flatten().InnerExceptions.First(); | ||
SentryHelper.ExceptionHandler(innerExceptionsFirst, SentryHelper.ExceptionType.UnhandledOther); | ||
throw innerExceptionsFirst; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above
throw ex.Flatten().InnerExceptions.First(); | ||
var innerExceptionsFirst = ex.Flatten().InnerExceptions.First(); | ||
SentryHelper.ExceptionHandler(innerExceptionsFirst, SentryHelper.ExceptionType.UnhandledOther); | ||
throw innerExceptionsFirst; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above
catch | ||
catch (Exception ex) | ||
|
||
{ | ||
await SentryHelper.ExceptionHandlerAsync(ex, SentryHelper.ExceptionType.UnhandledOther); | ||
IsRunning = false; | ||
throw; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This catch might probably throw OperationCancelledException
or TaskCancelledException
too due to the usage of Game Repair mechanism for checking the game integrity before the delta-patch come into play.
catch (Exception ex) | ||
{ | ||
await SentryHelper.ExceptionHandlerAsync(ex, SentryHelper.ExceptionType.UnhandledOther); | ||
LogWriteLine($"Error has occurred while performing delta-patch!\r\n{ex}", LogType.Error, true); | ||
throw; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This catch might also throw OperationCancelledException
or TaskCancelledException
. Type check is needed to exclude these two cancellation exceptions from sentry
catch (Exception ex) | ||
{ | ||
await SentryHelper.ExceptionHandlerAsync(ex, SentryHelper.ExceptionType.UnhandledOther); | ||
throw; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need cancellation exception check
await SentryHelper.ExceptionHandlerAsync(ex, SentryHelper.ExceptionType.UnhandledOther); | ||
LogWriteLine($"Failed while getting CDN content from: {cdnProp.Name} (prefix: {cdnProp.URLPrefix}) (relPath: {relativeURL})\r\n{ex}", LogType.Error, true); | ||
return null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above
await SentryHelper.ExceptionHandlerAsync(ex, SentryHelper.ExceptionType.UnhandledOther); | ||
LogWriteLine($"Failed while getting CDN content from: {cdnProp.Name} (prefix: {cdnProp.URLPrefix}) (relPath: {relativeURL})\r\n{ex}", LogType.Error, true); | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above
await SentryHelper.ExceptionHandlerAsync(ex, SentryHelper.ExceptionType.UnhandledOther); | ||
LogWriteLine($"Failed while getting CDN content from: {cdnProp.Name} (prefix: {cdnProp.URLPrefix}) (relPath: {relativeURL})\r\n{ex}", LogType.Error, true); | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above
await SentryHelper.ExceptionHandlerAsync(ex, SentryHelper.ExceptionType.UnhandledOther); | ||
LogWriteLine($"CDN content from: {cdnProp.Name} (prefix: {cdnProp.URLPrefix}) (relPath: {relativeURL}) has failed to initialize due to an exception:\r\n{ex}", LogType.Error, true); | ||
return CDNUtilHTTPStatus.CreateInitializationError(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above
1. Use the InnerException if exception caught is an AggregateException 2. Throw TCE/OCE to the bin and not upload them 3. Deduplicate exception handling mechanism and use an inner method instead
Fixed in 653ed45 by returning all ExceptionHandler method if it sees an OCE/TCE being caught and send them straight out to the bin, also the handler will use the first exception instead if it sees AggregateException being caught |
Also set the environment as either Debug/non-debug based if there is any debugger attached
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approval received. Merging to main... We also need to notify users about the privacy policy changes and let them know the way to disable sentry's mechanism can be done either by
|
Main Goal
Add a way for Collapse to automatically report errors to a self-hosted instance of SentrySDK compatible server
PR Status :
TODO
Add hard crash detectionUnnecessaryAdd ExceptionHandler to EVERY METHOD CATCHERsDone