Skip to content
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

Block reports sent to ACRA #17402

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Block reports sent to ACRA #17402

wants to merge 2 commits into from

Conversation

voczi
Copy link
Contributor

@voczi voczi commented Nov 9, 2024

See title. Related:
#17392
ankidroid/Anki-Android-Backend#439
ankitects/anki@ba1f5f4

Preemptive "may lord have mercy".

@voczi voczi added Review High Priority Request for high priority review Needs Review labels Nov 9, 2024
Copy link
Member

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

general idea looks good and implementation seems functional (though I have poor Kotlin taste still, so don't rely on that)
some comments on functional/non-functional mix in the comment but I was able to parse through
bit o bike-shedding on throwable vs exception in naming etc

@voczi voczi requested a review from mikehardy November 9, 2024 14:52
Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Architecture:

  • This doesn't process logcat
  • I believe this changes the type of the exception to Throwable

@mikehardy
Copy link
Member

with regard to "This doesn't process logcat" from David -> I noticed while verifying my acrarium purge that there were some instances where the stack prompting a current error did not contain an email but the included logcat did. So it is a valid concern. I altered my purge matcher to also match emails deeper into the report (logcat also, not just original stack) so I'm cleaning them now, but better if they never hit logcat. Whole thing is messy, for sure

@voczi
Copy link
Contributor Author

voczi commented Nov 9, 2024

Work's left to be done. Still unsure about certain things.

@voczi voczi added Needs Author Reply Waiting for a reply from the original author and removed Review High Priority Request for high priority review Needs Review labels Nov 9, 2024
@voczi voczi changed the title Filter/block reports sent to ACRA Block reports sent to ACRA Nov 10, 2024
@voczi voczi added Needs Review and removed Needs Author Reply Waiting for a reply from the original author labels Nov 10, 2024
Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks SO much better, thanks so much!!!

One suggestion, take it or leave it

@david-allison david-allison added Needs Second Approval Has one approval, one more approval to merge and removed Needs Review labels Nov 10, 2024
Co-authored-by: David Allison <62114487+david-allison@users.noreply.github.com>
@voczi voczi added the squash-merge The pull request currently requires maintainers to "Squash Merge" label Nov 10, 2024
david-allison
david-allison previously approved these changes Nov 10, 2024
Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approval still stands.

Important

squash-merge The pull request currently requires maintainers to "Squash Merge"

@david-allison david-allison dismissed their stale review November 11, 2024 01:33

Given discussion on Discord - pushing this back to also be implemented for unhandled exceptions

@lukstbit lukstbit added the Needs Author Reply Waiting for a reply from the original author label Nov 11, 2024
@mikehardy
Copy link
Member

To be concrete about the idea of installing the filter as an uncaught exception handler also, here is exactly how to do it

/**
* We want to send an analytics hit on any exception, then chain to other handlers (e.g., ACRA)
*/
@Synchronized
private fun installDefaultExceptionHandler() {
sOriginalUncaughtExceptionHandler = Thread.getDefaultUncaughtExceptionHandler()
Thread.setDefaultUncaughtExceptionHandler { thread: Thread?, throwable: Throwable ->
sendAnalyticsException(throwable, true)
if (thread == null) {
Timber.w("unexpected: thread was null")
return@setDefaultUncaughtExceptionHandler
}
sOriginalUncaughtExceptionHandler!!.uncaughtException(thread, throwable)
}
}

As long as we already have the filter logic, it's just "store ref to original handler, install ourselves as handler, and after we handle an uncaught exception pass it on to the original handler" - idea being that this is yet another ask on the PR but this one is hopefully satisfyingly easy and then covers the last place these could slip through

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Author Reply Waiting for a reply from the original author Needs Second Approval Has one approval, one more approval to merge squash-merge The pull request currently requires maintainers to "Squash Merge"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants