ACRA upgrade to 5.1.3 - #4893
Conversation
|
Hmm - I'm not seeing the logcat in my test report for some reason If you have a link to one (or screenshot or something) before I worked in there, that could help me see if I'm messed that part up. The rest seems fine on inspection |
|
I already gave you access to acralyzer right? |
|
Most bug reports have a logcat in them... Here's the first one I opened which has a short logcat: |
|
Oh dang - I missed it but was just able to search and find it... |
|
During my test I had the debug Timber Tree installed, so the tag filter didn't work. Altered that and logcat is fine - that was unrelated to ACRA changes. Good thing I just learned the Timber stuff or I'd have been stumped on that a while. I still have one small thing with the toast lingering after the report dialog is dismissed, then this is done. |
|
This works exactly as intended now I think. The last commit - 74ccb10 commit actually cleans up a bug that was in the previous implementation causing stacks in the log Once this is done, there's a cool feature in ACRA 5.x to configurably limit reports - by time, by amount per trace, by device, by max number saved etc. Looks good to implement since it would eliminate some of the custom per-trace limiting code implemented here. https://www.acra.ch/javadoc/latest/org/acra/annotation/AcraLimiter.html |
|
Hey @timrae - no intention to pester but I'm assuming (a possibly dangerous activity) that this PR is giving you pause as it's such an important part of troubleshooting. So I thought I'd offer that if there's anything I can do to make it more digestible or explain more, I'd be happy to. I actually worked with the ACRA maintainer and he added a couple things into ACRA making it more testable, and I'm holding off on those to not muddy this PR up but I did extra care with this one (even putting my own ACRA server up!) so it should be in pretty good shape, for a major version change Once this is in, the second phase will be a more thorough config test (with new ACRA feature), a local report send test (with an ACRA change and guidance from them), and an implementation of the AcraLimiter to de-dupe and bandwidth-limit report sending |
| ACRA.getConfig().setLogcatArguments(logcatArgs); | ||
|
|
||
| // If you need to test ACRA to make sure it's working, switch which of these lines are commented | ||
| // and do a local build then trigger a crash |
There was a problem hiding this comment.
I don't really like putting verbose HOWTO comments into the source code itself. Can you please remove this and the one in the gradle and instead put that information into the development wiki?
There was a problem hiding this comment.
Okay - I added https://github.com/ankidroid/Anki-Android/wiki/Crash-Reports and I referenced in the Contributing docs (where it was before), and cleaned out the source comments
| */ | ||
| public void setAcraReportingMode(String value) { | ||
| SharedPreferences.Editor editor = ACRA.getACRASharedPreferences().edit(); | ||
| SharedPreferences.Editor editor = getSharedPrefs(this).edit(); |
There was a problem hiding this comment.
Previously we put the acra config parameters into their own config file, whereas here they'll be mixed in with ours. I don't remember why we were separating them before but there was probably a reason.
There was a problem hiding this comment.
Yeah, that was clearly the intention, but it turns out that it was implemented incorrectly so it was still in the shared config space in practice (verified by looking at the prefs files pulled from devices and emulators).
Given the existing prefs in the shared space, and that the preferences themselves have an 'acra' key prefix, it seemed like simplifying the code to match reality made more sense?
| * | ||
| * @param prefs SharedPreferences object the reporting state is persisted in | ||
| */ | ||
| public void setDebugACRAConfig(SharedPreferences prefs) { |
There was a problem hiding this comment.
Can you make these methods private? I know you're using them in the unit test, but I think you can use the deencapsulation feature of the testing framework, right? The interface of a class should not be changed to accommodate a unit test.
There was a problem hiding this comment.
yep - changing these to private now
| /** | ||
| * Puts ACRA Reporting mode into "ask" every time, overrides user choice | ||
| */ | ||
| public void setAcraTestACRAConfig(SharedPreferences prefs) { |
There was a problem hiding this comment.
Again I don't think this function is required, just write what to do in the wiki
There was a problem hiding this comment.
I feel a tension on this one between probability of actual+valid testing in the future, difficulty of testing, and code hygiene.
I'm worried that if it's hard to test - as in you have to go to wiki and get code to paste (or understand a bunch of concepts then generate correct code), vs just toggle a commented line that uses tested code, testing on future changes will either be skipped or won't be valid for some subtle reason. As a personal example, my initial testing was invalid for a variety of reasons from subtle reporting preferences interactions to the Timber logging being different between production and debug.
So I'd personally like to leave this in but if you still feel it should go after a second think on it, I'll put everything in the wiki instead
| } | ||
| resourceId = ACRA.getConfig().resDialogIcon(); | ||
| if(resourceId != 0) { | ||
| dialogBuilder.setIcon(resourceId); |
There was a problem hiding this comment.
It did seem like some effort went into doing the icon, but in practice this code wasn't connected to an icon because none was ever set that I saw? Without setting one, getting one didn't seem to make sense so I removed those lines
There was a problem hiding this comment.
You were prescient here. It looks terrible without an icon. New PR for branding
It's just hard to find enough time to do a proper code review these days, and I often forget about it if I don't get to it within a few days. Feel free to ping me after a few weeks if I don't get back to you :-p |
|
Then I am traveling and go dark for a week :-). All the review stuff seemed on target I'll address it when I get a chance |
- Upgraded to ACRA dependency, but had to be careful of a gradle dep clash - Refactored Strings to constants where used more than once - Refactored ACRA configuration out of onCreate for testability - Removed the “additionalSharedPrefs” config, our config is the default and it's confusing - use init() hooks in CrashDialog in place of deprecated create() - Added test that exercises all the configuration variants I’m aware of - Note exactly how to test crashing, and provide free test acralyzer instance to test against
- Requires Java 1.8 compatibility
- Configuration split amongst different ACRA modules
- Access to configurations through Builders
|
Okay @timrae, I think I've responded to all the review notes (thank you!) and pushed a new commit that handles them. Unless you see something new then we need consensus on the TestACRAConfig thing and then I'll have my fingers crossed for a merge :-) |
|
FYI, we forked acralyzer, and don't use the (stagnant) upstream version anymore. It shouldn't make any difference, but if you're interested to run that on your cloudant instance or update the wiki then feel free: |
Oh! I had no idea - I did a cursory look and saw that our acralyzer was different but assumed upstream had moved on. Wouldn't have guessed it was AnkiDroid picking up the baton...I'm not sure the current ACRA maintainer (who appears new and motivated currently) knew about that so I left him a breadcrumb. Maybe they'll upstream that stuff. Thanks for the pointer |
|
Yeah, while acra itself is pretty well maintained, acralyzer was developed to a kind of proof of concept level and then abandoned by the author. The upstream version has horrendous performance issues and is not really usable for our purposes. |
| /** | ||
| * Puts ACRA Reporting mode into "ask" every time, overrides user choice | ||
| */ | ||
| private void setAcraTestACRAConfig(SharedPreferences prefs) { |
There was a problem hiding this comment.
I don't see a need for this function tbh... The current behavior is that if a developer wants to send a bug report for some reason while in debug mode, then they open preferences and manually enable reporting (reports are sent to the prod acralyzer instance), and this has been sufficient for our needs.
You've changed that behavior a bit here, so that the debug reports are sent to a special debug instance, which makes sense. But in that case, I think it's better to just use the same value for FEEDBACK_REPORT_KEY as the release build and get rid of this manual overriding of the user preference value entirely. Those reports probably still shouldn't be completely open to the public though.
|
@timrae - I think I've addressed everything at this point, with the last 3 commits satisfying codacy and your request. There appears to be two feedback items about the icon and the acra config in a separate file unresolved, but my comments there still stand - they appeared to be unused in practice, so my removal of those bits is just removing dangling items. I can leave them be if you like though. If this is ready I can squash the commits down (and remove the merge commit) for merge |
| * Set the ACRA ConfigurationBuilder and <b>re-initialize the ACRA system</b> with the contents | ||
| * @param acraCoreConfigBuilder the full ACRA config to initialize ACRA with | ||
| */ | ||
| public void setAcraConfigBuilder(CoreConfigurationBuilder acraCoreConfigBuilder) { |
|
Can you please update the wiki page on crash reporting to give the latest info? Developers are gonna want to know how to enable crash reports for their debug builds, and where to find them. Also Android Studio 3.2 is coming out soon, so I think you can remove the part about the advanced profiling. There's no point in squashing down to a single commit as I can do that from the UI, but if you want to partition your commits in a certain way then please feel free to go ahead once you've fixed the visibility of |
|
Okay - I updated the visibility - and I updated the docs. I flagged that part as removable after 3.2 but honestly I'd already forgotten it, and I just changed laptops and Android Studio installs! I'd have needed the pointer today so I left it but with the note to delete after the 3.2 release. I looked at the code and there's not really a way to partition it because it spidered out everywhere and won't compile with individual changes. Squash to one or none I think |
2 participants
Pull Request template
Please, go through these checks before you submit a PR.
ifstatements)Purpose / Description
ACRA 4.6 was noted to have a problem in #4200
Fixes
ACRA 4.11 or 5.x is necessary to move to API>=26 as in #4883
Approach
I upgraded the library and our usage of it according to the documentation.
First I went to 4.11 which enforced a much more rigorous use of configuration and is most of the way to 5.x
Then I went to 5.x which separates the ACRA functionality into multiple modules. This requires Java 1.8 but it's still okay with minSdkVersion as low as 14
The commit messages for each step overview the changes. It's a bit spaghetti unfortunately.
How Has This Been Tested?
I made an instrumented test that verifies our 4 different configs
I also set up a test acralyzer instance on cloudant and configured it so that with one code change and a known crash bug (like #4200 ) you can build with ACRA enabled and see the report generation + result to make sure it works, which I did for both the 4.x and 5.x upgrades. All documented for future devs
Learning (optional, can help others)
I feel like Neo plugged into the training simulator in the matrix and now I know ACRA-fu (and acralyzer)