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

Disable logs #6439

Merged
merged 1 commit into from
Aug 18, 2020
Merged

Disable logs #6439

merged 1 commit into from
Aug 18, 2020

Conversation

NejcZdovc
Copy link
Contributor

@NejcZdovc NejcZdovc commented Aug 18, 2020

Resolves brave/brave-browser#11260

Submitter Checklist:

Test Plan:

plan 1:

  • enable rewards
  • make sure that there is no Rewards log file in profile

plan 2:

  • go to master and start browser
  • enable rewards
  • make sure that logs are generated and close browser
  • go to this PR
  • start browser
  • make sure that file is deleted and no further logs are persisted on disk

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

After-merge Checklist:

  • The associated issue milestone is set to the smallest version that the
    changes has landed on.
  • All relevant documentation has been updated.

@NejcZdovc NejcZdovc added this to the 1.14.x - Nightly milestone Aug 18, 2020
@NejcZdovc NejcZdovc requested a review from tmancey August 18, 2020 11:24
@NejcZdovc NejcZdovc self-assigned this Aug 18, 2020
@NejcZdovc NejcZdovc force-pushed the disable-logs branch 2 times, most recently from 6da03a3 to 0826821 Compare August 18, 2020 11:28
Copy link
Collaborator

@tmancey tmancey left a comment

Choose a reason for hiding this comment

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

Please see inline comments

@@ -2323,7 +2323,7 @@ void RewardsServiceImpl::DiagnosticLog(
const int line,
const int verbose_level,
const std::string& message) {
if (ledger_for_testing_) {
if (ledger_for_testing_ || !should_persist_logs_) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: !should_persist_logs_ within a separate if as conditions are not related

Copy link
Collaborator

@tmancey tmancey left a comment

Choose a reason for hiding this comment

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

LGTM++

Copy link
Collaborator

@kylehickinson kylehickinson left a comment

Choose a reason for hiding this comment

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

iOS currently doesn't need to implement this, but when it does, are we talking deleting all logs? Is this like a clear-all?

@NejcZdovc
Copy link
Contributor Author

@kylehickinson just removing Rewards.log file (at least this is how we name it on desktop).

@NejcZdovc NejcZdovc merged commit a51a2a4 into master Aug 18, 2020
@NejcZdovc NejcZdovc deleted the disable-logs branch August 18, 2020 14:08
@kylehickinson
Copy link
Collaborator

Gotcha, which is where all logs are stored on desktop I presume? When we have persisted logs enabled, we have multiple logs files per day. So this would essentially delete all persisted rewards logs

@NejcZdovc NejcZdovc mentioned this pull request Aug 18, 2020
4 tasks
@NejcZdovc
Copy link
Contributor Author

@kylehickinson yeah on desktop we only have one file. and if that is the case on iOS yes you need to delete all of them

@kjozwiak
Copy link
Member

Verification PASSED on macOS 10.15.6 x64 using the following builds:

Brave | 1.14.51 Chromium: 85.0.4183.69 (Official Build) nightly (64-bit)
-- | --
Revision | 4554ea1a1171bd8d06951a4b7d9336afe6c59967-refs/branch-heads/4183@{#1426}
OS | macOS Version 10.15.6 (Build 19G73)

Using the STR/Cases outlined via #6439 (comment), went through the following:

Case 1:

  • downloaded/installed 1.14.51 CR: 85.0.4183.69
  • enabled rewards via the rewards panel (double checked brave://rewards)
  • checked and ensured that Rewards.log wasn't created under ~/Library/Application\ Support/BraveSoftware/Brave-Browser-Nightly/Default
  • ensured that Clear, Refresh and Download full log doesn't work under brave://rewards-internals and doesn't crash (checked brave://crashes)

Case 2:

  • Updated my personal profile which was on 1.14.48 CR: 85.0.4183.69 to 1.14.51 CR: 85.0.4183.69
  • ensured that Rewards.log was removed from ~/Library/Application\ Support/BraveSoftware/Brave-Browser-Nightly/Default
  • ensured there's no crashes under brave://crashes after upgrading
  • ensured that Clear, Refresh and Download full log doesn't work under brave://rewards-internals and doesn't crash (checked brave://crashes)

@kjozwiak
Copy link
Member

@kylehickinson do we need a separate issue for iOS?

@kylehickinson
Copy link
Collaborator

@kjozwiak no, iOS never wrote rewards logs to disk in any live version so we should be OK. Rewards Internals for iOS also removed logs before it merged.

May just want to make a note on the Rewards Internals issue

@srirambv
Copy link
Contributor

@NejcZdovc can you please add steps for Android. Issue is marked to verify on Android as well

@NejcZdovc
Copy link
Contributor Author

@srirambv because you can't check if file is there or not I would just make sure if you go to internals page and click download that nothing happens/file is blank

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disable rewards logs and remove existing ones
5 participants