Log Rewards errors by default and add a feature flag for verbose logging #8598

zenparsing · 2021-04-21T21:40:09Z

Resolves brave/brave-browser#15500

Enables logging of "error" level messages by default.
Adds a feature flag BraveRewardsVerboseLogging which can be used to turn on verbose logging.
Removes arbitrary data from existing error logging calls.
Adds logging of unexpected HTTP response status codes.
The existing --rewards=persist-logs=1 command line option is still supported, but deprecated in favor of specifying the feature flag on the command line with --enable-features=BraveRewardsVerboseLogging

Submitter Checklist:

I confirm that no security/privacy review is needed, or that I have requested one
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

Error logging

Given that verbose logging is not enabled
When Rewards encounters an error condition and logs an error
Then the error should be visible in the "Logs" tab of brave://rewards-internals

Possible test steps:

Open browser with clean profile pointed to staging.
Trigger a Rewards error:
- Open the Rewards panel and solve UGP captcha incorrectly
Go to brave://rewards-internals
Select the "Logs" tab and click "Refresh"
Verify that an error has been logged
Verify that non-error messages do not appear

Verbose logging

Given that the user has enabled verbose logging from brave://flags
When the user interacts with Rewards
Then detailed logging should be visible in the "Logs" tab of brave://rewards-internals

Possible test steps

Open browser with clean profile pointed to staging.
Go to brave://flags and enable BraveRewardsVerboseLogging.
Relaunch the browser.
Go to brave://rewards-internals
Select the "Logs" tab and click "Refresh"
Verify that detailed information is logged.

tmancey · 2021-04-26T18:08:01Z

components/brave_rewards/browser/rewards_service_impl.cc

@@ -334,6 +334,9 @@ RewardsServiceImpl::RewardsServiceImpl(Profile* profile)
  content::URLDataSource::Add(profile_,
                              std::make_unique<BraveRewardsSource>(profile_));
  ready_ = std::make_unique<base::OneShotEvent>();
+
+  if (base::FeatureList::IsEnabled(features::kVerboseLoggingFeature))
+    persist_log_level_ = kDiagnosticLogMaxVerboseLevel;


Maybe worth checking the performance impact on Android?

cc @deeppandya

I don't think it would have any impact on performance. I can run through the branch and quickly check.

vendor/bat-native-ads/src/bat/ads/internal/account/ad_rewards/ad_rewards.cc

vendor/bat-native-ledger/src/bat/ledger/internal/legacy/bat_state.cc

emerick

LGTM

mkarolin

chromium_src changes LGTM

diracdeltas · 2021-04-26T20:01:17Z

chromium_src/chrome/browser/flag_descriptions.cc

+const char kBraveRewardsVerboseLoggingName[] =
+    "Enable Brave Rewards verbose logging";
+const char kBraveRewardsVerboseLoggingDescription[] =
+    "Enables detailed logging of Brave Rewards system events to a log file";


IMO this should have a warning that says this might log sensitive information.

Agreed - I'll add some preliminary text and attach a screenshot.

tmancey · 2021-04-27T12:18:39Z

components/brave_rewards/browser/rewards_service_impl.h

@@ -790,7 +790,7 @@ class RewardsServiceImpl : public RewardsService,
  bool reset_states_;
  bool ledger_for_testing_ = false;
  bool resetting_rewards_ = false;
-  bool should_persist_logs_ = false;
+  int persist_log_level_ = 0;


0 will only log errors. Ideally, this should be 2, which will also log WARNINGS and INFO. Anything above INFO is considered verbose.

I agree that more logging would be ideal. Unfortunately, logging at 2 will create for us a dilemma: we'll likely have to remove useful information from messages at those levels, which will make those levels less useful for development and detailed debugging. I would prefer that we stay with logging only errors by default in this PR, and consider increasing the default log level as a future change.

vendor/bat-native-ads/src/bat/ads/internal/account/ad_rewards/ad_rewards.cc

vendor/bat-native-ledger/src/bat/ledger/internal/legacy/bat_state.cc

components/brave_rewards/browser/rewards_service_impl.cc

diracdeltas · 2021-04-27T19:56:54Z

chromium_src/chrome/browser/flag_descriptions.cc

+    "Enables detailed logging of Brave Rewards system events to a log file "
+    "stored on your device. Please note that this file may contain sensitive "
+    "information. Please only submit it using "
+    "https://upload-support.brave.com/rewards.";


This might encourage people to send us these files who otherwise might not, and in general we want to collect as little of these log files if possible.

Could we change it to Note that this file may contain sensitive information. Do not share it unless asked to by Brave staff for debugging purposes. cc @hollons

That suggestion looks much better to me! I'm going to go ahead and update the PR while we wait on more feedback.

zenparsing · 2021-04-28T14:25:24Z

CI Results:

MacOS and Linux build and tests succeeded, failed on Archive JUnit-formatted test results step (unrelated to PR).

emerick

LGTM

tmancey

LGTM

kjozwiak · 2021-05-18T06:40:25Z

Verification PASSED on Win 10 x64 using the following build:

Brave | 1.26.22 Chromium: 91.0.4472.57 (Official Build) nightly (64-bit)
--- | ---
Revision | e3443317fa07f1e9997e4a9c738eddfefc3c0292-refs/branch-heads/4472_54@{#6}
OS | Windows 10 OS Version 2009 (Build 19042.964)

Error logging

disconnected the internet/network and attempted to create a wallet within a new profile
after ~1minute, enabled the network/internet and created a wallet
connected to a production uphold account
verified that only errors are appearing in brave://rewards-internals while #brave-rewards-verbose-logging is disabled
ensured that Refresh, Clear & Download are working as expected

Verbose logging

enabled #brave-rewards-verbose-logging via brave://flags
once enabled, created a new wallet
connected to my production Uphold account
verified there's more detailed logs present under brave://rewards-internals
ensured that Refresh, Clear & Download are working as expected

kjozwiak · 2021-05-22T03:26:41Z

Went through the above cases on Android and added the verification notes on Nightly for uplift via #8841 (comment).

github-actions bot assigned zenparsing Apr 21, 2021

zenparsing force-pushed the ksmith-rewards-log-errors branch 2 times, most recently from 45b9141 to 5f03daa Compare April 26, 2021 15:47

zenparsing marked this pull request as ready for review April 26, 2021 17:42

zenparsing requested review from tmancey and a team as code owners April 26, 2021 17:42

zenparsing requested a review from emerick April 26, 2021 17:47

zenparsing added the feature/rewards label Apr 26, 2021

zenparsing force-pushed the ksmith-rewards-log-errors branch from 5f03daa to baa2948 Compare April 26, 2021 18:12

tmancey reviewed Apr 26, 2021

View reviewed changes

emerick approved these changes Apr 26, 2021

View reviewed changes

zenparsing force-pushed the ksmith-rewards-log-errors branch from baa2948 to c0b0e2e Compare April 26, 2021 18:20

mkarolin approved these changes Apr 26, 2021

View reviewed changes

diracdeltas reviewed Apr 26, 2021

View reviewed changes

tmancey reviewed Apr 27, 2021

View reviewed changes

vendor/bat-native-ads/src/bat/ads/internal/account/ad_rewards/ad_rewards.cc Outdated Show resolved Hide resolved

tmancey reviewed Apr 27, 2021

View reviewed changes

vendor/bat-native-ads/src/bat/ads/internal/account/ad_rewards/ad_rewards.cc Outdated Show resolved Hide resolved

tmancey reviewed Apr 27, 2021

View reviewed changes

vendor/bat-native-ledger/src/bat/ledger/internal/legacy/bat_state.cc Outdated Show resolved Hide resolved

tmancey reviewed Apr 27, 2021

View reviewed changes

components/brave_rewards/browser/rewards_service_impl.cc Show resolved Hide resolved

zenparsing force-pushed the ksmith-rewards-log-errors branch from c0b0e2e to 59c06c2 Compare April 27, 2021 19:50

zenparsing requested a review from a team as a code owner April 27, 2021 19:50

zenparsing requested a review from bsclifton April 27, 2021 19:50

diracdeltas reviewed Apr 27, 2021

View reviewed changes

zenparsing force-pushed the ksmith-rewards-log-errors branch from 59c06c2 to 4266e7f Compare April 27, 2021 20:42

Log Rewards errors by default and add a feature flag for verbose logging

6136bed

zenparsing force-pushed the ksmith-rewards-log-errors branch from 4266e7f to 6136bed Compare April 28, 2021 21:36

zenparsing requested review from tmancey and emerick April 28, 2021 21:48

emerick approved these changes Apr 28, 2021

View reviewed changes

tmancey approved these changes Apr 28, 2021

View reviewed changes

zenparsing merged commit 202e658 into master Apr 29, 2021

zenparsing deleted the ksmith-rewards-log-errors branch April 29, 2021 11:44

zenparsing added this to the 1.26.x - Nightly milestone Apr 29, 2021

brave-builds pushed a commit that referenced this pull request May 5, 2021

Uplift of #8598 (squashed) to beta

4ecb67c

brave-builds mentioned this pull request May 5, 2021

Log Rewards errors by default and add a feature flag for verbose logging (uplift to 1.25.x) #8717

Merged

7 tasks

bsclifton mentioned this pull request May 10, 2021

(Ledger client) Capture more details when linking anonymous wallet to custodian brave/brave-browser#15257

Closed

zenparsing pushed a commit that referenced this pull request May 12, 2021

Uplift of #8598 and #8767 (squashed) to beta

a30d634

GeetaSarvadnya mentioned this pull request May 20, 2021

Log Rewards errors by default brave/brave-browser#15500

Closed

kjozwiak mentioned this pull request May 22, 2021

#brave-rewards-verbose-logging missing from Android - follow up to #15661 brave/brave-browser#15924

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Log Rewards errors by default and add a feature flag for verbose logging #8598

Log Rewards errors by default and add a feature flag for verbose logging #8598

zenparsing commented Apr 21, 2021 •

edited

Loading

tmancey Apr 26, 2021

zenparsing Apr 26, 2021

deeppandya Apr 27, 2021

emerick left a comment

mkarolin left a comment

diracdeltas Apr 26, 2021

zenparsing Apr 27, 2021

tmancey Apr 27, 2021 •

edited

Loading

zenparsing Apr 27, 2021

diracdeltas Apr 27, 2021

zenparsing Apr 27, 2021

zenparsing commented Apr 28, 2021

emerick left a comment

tmancey left a comment

kjozwiak commented May 18, 2021 •

edited

Loading

kjozwiak commented May 22, 2021

Log Rewards errors by default and add a feature flag for verbose logging #8598

Log Rewards errors by default and add a feature flag for verbose logging #8598

Conversation

zenparsing commented Apr 21, 2021 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

Error logging

Verbose logging

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emerick left a comment

Choose a reason for hiding this comment

mkarolin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmancey Apr 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zenparsing commented Apr 28, 2021

emerick left a comment

Choose a reason for hiding this comment

tmancey left a comment

Choose a reason for hiding this comment

kjozwiak commented May 18, 2021 • edited Loading

kjozwiak commented May 22, 2021

zenparsing commented Apr 21, 2021 •

edited

Loading

tmancey Apr 27, 2021 •

edited

Loading

kjozwiak commented May 18, 2021 •

edited

Loading