oppia-android icon indicating copy to clipboard operation
oppia-android copied to clipboard

Fixes #4334: Log report creation support for performance metrics collection

Open Sarthak2601 opened this issue 2 years ago • 10 comments

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).

Sarthak2601 avatar Jun 08 '22 23:06 Sarthak2601

@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.

Sarthak2601 avatar Jun 13 '22 23:06 Sarthak2601

Unassigning @Sarthak2601 since a re-review was requested. @Sarthak2601, please make sure you have addressed all review comments. Thanks!

oppiabot[bot] avatar Jun 13 '22 23:06 oppiabot[bot]

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!

oppiabot[bot] avatar Jul 04 '22 10:07 oppiabot[bot]

Hi @Sarthak2601. Apologies but I'll need to review this tomorrow.

BenHenning avatar Jul 12 '22 03:07 BenHenning

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 avatar Jul 13 '22 02:07 BenHenning

(Removing Akshay per recent codeowner changes).

BenHenning avatar Jul 15 '22 04:07 BenHenning

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.

Sarthak2601 avatar Jul 18 '22 20:07 Sarthak2601

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 avatar Jul 19 '22 06:07 BenHenning

@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.

Sarthak2601 avatar Jul 23 '22 05:07 Sarthak2601

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!

oppiabot[bot] avatar Aug 05 '22 09:08 oppiabot[bot]

@Sarthak2601 leaving this to you to merge so that you can update any downstream PRs as needed.

BenHenning avatar Sep 02 '22 00:09 BenHenning

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!

oppiabot[bot] avatar Sep 02 '22 00:09 oppiabot[bot]

@Sarthak2601 leaving this to you to merge so that you can update any downstream PRs as needed.

Thanks @BenHenning, merging now!

Sarthak2601 avatar Sep 02 '22 06:09 Sarthak2601