oppia-android
oppia-android copied to clipboard
Fixes #4334: Log report creation support for performance metrics collection
Explanation
Fixes #4334
Introduces log creation support for performance metrics
- Adds a controller (AppHealthMetricController) for logging and caching of metric logs -- done.
- Modifies OppiaLogger for logging of loggableMetrics -- done.
- Introduces framework for periodically generating metric logs as well as periodically uploading them using the existing implementation of work manager -- framework done, actual worker functions done.
- Introduces classes to get under the hood metric information for logging -- done.
- Introduces tests for all modifications/additions to verify functionality.
- Renamed LogUploadWorkManagerInitialiser to LogReportWorkManagerInitialiser along with LogUploadWorkerModule -- led to multiple file changes.
Note: This PR introduces basic logging structure for cpu usage logging but doesn't introduce a way to actually get usage metric. That portion will be covered by a separate PR (tracked by issue#4466).
Essential Checklist
- [x] The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
- [x] Any changes to scripts/assets files have their rationale included in the PR explanation.
- [x] The PR follows the style guide.
- [x] The PR does not contain any unnecessary code changes from Android Studio (reference).
- [x] The PR is made from a branch that's not called "develop" and is up-to-date with "develop".
- [x] The PR is assigned to the appropriate reviewers (reference).
@BenHenning Although this is in draft stage right now, could you please take a look at this and provide feedback on the overall structure of this PR? It'll be quite helpful.
Unassigning @Sarthak2601 since a re-review was requested. @Sarthak2601, please make sure you have addressed all review comments. Thanks!
Hi @Sarthak2601, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue. If you are still working on this PR, please make a follow-up commit within 3 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!
Hi @Sarthak2601. Apologies but I'll need to review this tomorrow.
Ah, also @Sarthak2601 I suggest syncing with the latest develop & then making sure all of your CI checks are passing before sending this back for review (I think a number of the checks actually caught some of the comments I've left around KDoc presence).
(Removing Akshay per recent codeowner changes).
Ah, also @Sarthak2601 I suggest syncing with the latest develop & then making sure all of your CI checks are passing before sending this back for review (I think a number of the checks actually caught some of the comments I've left around KDoc presence).
@BenHenning I've fixed most of the tests and while I fix the rest, I think it'd be good if you could review the PR -- will help in keeping the pace.
Also, regarding one of the tests related to checking storageUsage of the application, it works fine on AS but fails on CI. The last 4 digits out of 12 don't match but the rest of the digits remains the same. It might be something related to GitHub actions' environment. Will it be okay if we round off storage usage and exclude the last 4 digits in tests? If not, what could be a possible solution for this -- any help is appreciated.
Ah, also @Sarthak2601 I suggest syncing with the latest develop & then making sure all of your CI checks are passing before sending this back for review (I think a number of the checks actually caught some of the comments I've left around KDoc presence).
@BenHenning I've fixed most of the tests and while I fix the rest, I think it'd be good if you could review the PR -- will help in keeping the pace.
Also, regarding one of the tests related to checking storageUsage of the application, it works fine on AS but fails on CI. The last 4 digits out of 12 don't match but the rest of the digits remains the same. It might be something related to GitHub actions' environment. Will it be okay if we round off storage usage and exclude the last 4 digits in tests? If not, what could be a possible solution for this -- any help is appreciated.
I'm curious whether it also differs between Bazel and Gradle @Sarthak2601. It probably has something to do with the test binary "APK" size (per Robolectric). We may need to use a fake for the size computation in order to get a reliable behavior in higher tests.
@BenHenning I've made the requested changes. Please note that I'm yet to build the project with Bazel -- I'll do that over the weekend to fix tests-- but I think it'll be productive to get a review at this stage.
Hi @Sarthak2601, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue. If you are still working on this PR, please make a follow-up commit within 3 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!
@Sarthak2601 leaving this to you to merge so that you can update any downstream PRs as needed.
Hi @Sarthak2601, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to merge this PR once the CI checks pass and you're happy with it. Thanks!
@Sarthak2601 leaving this to you to merge so that you can update any downstream PRs as needed.
Thanks @BenHenning, merging now!