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

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

Merged
merged 1 commit into from
Apr 29, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions chromium_src/chrome/browser/about_flags.cc
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,10 @@ using ntp_background_images::features::kBraveNTPSuperReferralWallpaper;
flag_descriptions::kBravePermissionLifetimeName, \
flag_descriptions::kBravePermissionLifetimeDescription, kOsAll, \
FEATURE_VALUE_TYPE(permissions::features::kPermissionLifetime)}, \
{"brave-rewards-verbose-logging", \
flag_descriptions::kBraveRewardsVerboseLoggingName, \
flag_descriptions::kBraveRewardsVerboseLoggingDescription, kOsDesktop, \
FEATURE_VALUE_TYPE(brave_rewards::features::kVerboseLoggingFeature)}, \
{"brave-rewards-bitflyer", \
flag_descriptions::kBraveRewardsBitflyerName, \
flag_descriptions::kBraveRewardsBitflyerDescription, kOsDesktop, \
Expand Down
8 changes: 8 additions & 0 deletions chromium_src/chrome/browser/flag_descriptions.cc
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,14 @@ const char kBraveSyncDescription[] =
const char kBraveIpfsName[] = "Enable IPFS";
const char kBraveIpfsDescription[] =
"Enable native support of IPFS.";
const char kBraveRewardsVerboseLoggingName[] =
"Enable Brave Rewards verbose logging";
const char kBraveRewardsVerboseLoggingDescription[] =
"Enables detailed logging of Brave Rewards system events to a log file "
"stored on your device. Please note that this log file could include "
"information such as browsing history and credentials such as passwords "
"and access tokens depending on your activity. Please do not share it "
"unless asked to by Brave staff.";
const char kBraveRewardsBitflyerName[] = "Enable bitFlyer for Brave Rewards";
const char kBraveRewardsBitflyerDescription[] =
"Enables support for bitFlyer as an external wallet provider for Brave "
Expand Down
2 changes: 2 additions & 0 deletions chromium_src/chrome/browser/flag_descriptions.h
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ extern const char kBraveSyncName[];
extern const char kBraveSyncDescription[];
extern const char kBraveIpfsName[];
extern const char kBraveIpfsDescription[];
extern const char kBraveRewardsVerboseLoggingName[];
extern const char kBraveRewardsVerboseLoggingDescription[];
extern const char kBraveRewardsBitflyerName[];
extern const char kBraveRewardsBitflyerDescription[];
extern const char kNativeBraveWalletName[];
Expand Down
20 changes: 10 additions & 10 deletions components/brave_rewards/browser/rewards_service_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe worth checking the performance impact on Android?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

}

RewardsServiceImpl::~RewardsServiceImpl() {
Expand Down Expand Up @@ -2263,15 +2266,12 @@ void RewardsServiceImpl::WriteDiagnosticLog(const std::string& file,
const int line,
const int verbose_level,
const std::string& message) {
if (ledger_for_testing_ || !should_persist_logs_) {
return;
}

if (resetting_rewards_) {
if (ledger_for_testing_ || resetting_rewards_) {
return;
}

if (verbose_level > kDiagnosticLogMaxVerboseLevel) {
if (verbose_level > kDiagnosticLogMaxVerboseLevel ||
verbose_level > persist_log_level_) {
return;
}

Expand Down Expand Up @@ -2414,13 +2414,13 @@ void RewardsServiceImpl::HandleFlags(const std::string& options) {
continue;
}

// The "persist-logs" command-line flag is deprecated and will be removed
// in a future version. Use --enable-features=BraveRewardsVerboseLogging
// instead.
if (name == "persist-logs") {
const std::string lower = base::ToLowerASCII(value);

if (lower == "true" || lower == "1") {
should_persist_logs_ = true;
} else {
should_persist_logs_ = false;
persist_log_level_ = kDiagnosticLogMaxVerboseLevel;
zenparsing marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand Down
2 changes: 1 addition & 1 deletion components/brave_rewards/browser/rewards_service_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Collaborator

@tmancey tmancey Apr 27, 2021

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.


GetTestResponseCallback test_response_callback_;

Expand Down
3 changes: 3 additions & 0 deletions components/brave_rewards/common/features.cc
Original file line number Diff line number Diff line change
Expand Up @@ -13,5 +13,8 @@ namespace features {
const base::Feature kBitflyerFeature{"BraveRewardsBitflyer",
base::FEATURE_DISABLED_BY_DEFAULT};

const base::Feature kVerboseLoggingFeature{"BraveRewardsVerboseLogging",
base::FEATURE_DISABLED_BY_DEFAULT};

} // namespace features
} // namespace brave_rewards
1 change: 1 addition & 0 deletions components/brave_rewards/common/features.h
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ namespace brave_rewards {
namespace features {

extern const base::Feature kBitflyerFeature;
extern const base::Feature kVerboseLoggingFeature;

} // namespace features
} // namespace brave_rewards
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,8 @@ void AdRewards::OnGetPayments(const UrlResponse& url_response) {
}

if (!payments_->SetFromJson(url_response.body)) {
BLOG(0, "Failed to parse payment balance: " << url_response.body);
BLOG(0, "Failed to parse payment balance");
BLOG(6, "Payment balance response body: " << url_response.body);
OnFailedToReconcileAdRewards();
return;
}
Expand Down Expand Up @@ -324,7 +325,8 @@ void AdRewards::OnGetAdGrants(const UrlResponse& url_response) {
}

if (!ad_grants_->SetFromJson(url_response.body)) {
BLOG(0, "Failed to parse ad grants: " << url_response.body);
BLOG(0, "Failed to parse ad grants");
BLOG(6, "Ad grants response body: " << url_response.body);
OnFailedToReconcileAdRewards();
return;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ type::Result GetParameters::CheckStatusCode(const int status_code) {
}

if (status_code != net::HTTP_OK) {
BLOG(0, "Unexpected HTTP status: " << status_code);
return type::Result::LEDGER_ERROR;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,12 @@ type::Result GetBalance::CheckStatusCode(const int status_code) {
if (status_code == net::HTTP_UNAUTHORIZED ||
status_code == net::HTTP_NOT_FOUND ||
status_code == net::HTTP_FORBIDDEN) {
BLOG(0, "Invalid authorization HTTP status: " << status_code);
return type::Result::EXPIRED_TOKEN;
}

if (status_code != net::HTTP_OK) {
BLOG(0, "Unexpected HTTP status: " << status_code);
return type::Result::LEDGER_ERROR;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,10 +58,12 @@ std::string PostOauth::GeneratePayload(const std::string& external_account_id,

type::Result PostOauth::CheckStatusCode(const int status_code) {
if (status_code == net::HTTP_UNAUTHORIZED) {
BLOG(0, "Unauthorized access");
return type::Result::EXPIRED_TOKEN;
}

if (status_code != net::HTTP_OK) {
BLOG(0, "Unexpected HTTP status: " << status_code);
return type::Result::LEDGER_ERROR;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ std::string PostTransaction::GeneratePayload(

type::Result PostTransaction::CheckStatusCode(const int status_code) {
if (status_code == net::HTTP_UNAUTHORIZED) {
BLOG(0, "Unauthorized access");
return type::Result::EXPIRED_TOKEN;
}

Expand All @@ -64,6 +65,7 @@ type::Result PostTransaction::CheckStatusCode(const int status_code) {
}

if (status_code != net::HTTP_OK) {
BLOG(0, "Unexpected HTTP status: " << status_code);
return type::Result::LEDGER_ERROR;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ type::Result GetCredentials::CheckStatusCode(const int status_code) {
}

if (status_code != net::HTTP_OK) {
BLOG(0, "Unexpected HTTP status: " << status_code);
return type::Result::RETRY;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ type::Result PostCredentials::CheckStatusCode(const int status_code) {
}

if (status_code != net::HTTP_OK) {
BLOG(0, "Unexpected HTTP status: " << status_code);
return type::Result::LEDGER_ERROR;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ type::Result PostOrder::CheckStatusCode(const int status_code) {
}

if (status_code != net::HTTP_CREATED) {
BLOG(0, "Unexpected HTTP status: " << status_code);
return type::Result::LEDGER_ERROR;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ type::Result PostTransactionAnon::CheckStatusCode(const int status_code) {
}

if (status_code != net::HTTP_CREATED) {
BLOG(0, "Unexpected HTTP status: " << status_code);
return type::Result::LEDGER_ERROR;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ type::Result PostTransactionUphold::CheckStatusCode(const int status_code) {
}

if (status_code != net::HTTP_CREATED) {
BLOG(0, "Unexpected HTTP status: " << status_code);
return type::Result::LEDGER_ERROR;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ type::Result PostVotes::CheckStatusCode(const int status_code) {
}

if (status_code != net::HTTP_OK) {
BLOG(0, "Unexpected HTTP status: " << status_code);
return type::Result::LEDGER_ERROR;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,7 @@ type::Result GetPublisher::CheckStatusCode(const int status_code) {
}

if (status_code != net::HTTP_OK) {
BLOG(0, "Unexpected HTTP status: " << status_code);
return type::Result::LEDGER_ERROR;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ type::Result GetAvailable::CheckStatusCode(const int status_code) {
}

if (status_code != net::HTTP_OK) {
BLOG(0, "Unexpected HTTP status: " << status_code);
return type::Result::LEDGER_ERROR;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ type::Result GetCaptcha::CheckStatusCode(const int status_code) {
}

if (status_code != net::HTTP_OK) {
BLOG(0, "Unexpected HTTP status: " << status_code);
return type::Result::LEDGER_ERROR;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ type::Result GetDrain::CheckStatusCode(const int status_code) {
}

if (status_code != net::HTTP_OK) {
BLOG(0, "Unexpected HTTP status: " << status_code);
return type::Result::LEDGER_ERROR;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ type::Result GetRecoverWallet::CheckStatusCode(const int status_code) {
}

if (status_code != net::HTTP_OK) {
BLOG(0, "Unexpected HTTP status: " << status_code);
return type::Result::LEDGER_ERROR;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ type::Result GetSignedCreds::CheckStatusCode(const int status_code) {
}

if (status_code != net::HTTP_OK) {
BLOG(0, "Unexpected HTTP status: " << status_code);
return type::Result::LEDGER_ERROR;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ type::Result GetWalletBalance::CheckStatusCode(const int status_code) {
}

if (status_code != net::HTTP_OK) {
BLOG(0, "Unexpected HTTP status: " << status_code);
return type::Result::LEDGER_ERROR;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ type::Result PostBatLoss::CheckStatusCode(const int status_code) {
}

if (status_code != net::HTTP_OK) {
BLOG(0, "Unexpected HTTP status: " << status_code);
return type::Result::LEDGER_ERROR;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ type::Result PostCaptcha::CheckStatusCode(const int status_code) {
}

if (status_code != net::HTTP_OK) {
BLOG(0, "Unexpected HTTP status: " << status_code);
return type::Result::LEDGER_ERROR;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ type::Result PostClaimBitflyer::CheckStatusCode(const int status_code) {
}

if (status_code != net::HTTP_OK) {
BLOG(0, "Unexpected HTTP status: " << status_code);
return type::Result::LEDGER_ERROR;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ type::Result PostClaimBrave::CheckStatusCode(const int status_code) {
}

if (status_code != net::HTTP_OK) {
BLOG(0, "Unexpected HTTP status: " << status_code);
return type::Result::LEDGER_ERROR;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,7 @@ type::Result PostClaimUphold::CheckStatusCode(const int status_code) {
}

if (status_code != net::HTTP_OK) {
BLOG(0, "Unexpected HTTP status: " << status_code);
return type::Result::LEDGER_ERROR;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ type::Result PostClobberedClaims::CheckStatusCode(const int status_code) {
}

if (status_code != net::HTTP_OK) {
BLOG(0, "Unexpected HTTP status: " << status_code);
return type::Result::LEDGER_ERROR;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ type::Result PostCreds::CheckStatusCode(const int status_code) {
}

if (status_code != net::HTTP_OK) {
BLOG(0, "Unexpected HTTP status: " << status_code);
return type::Result::LEDGER_ERROR;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ type::Result PostDevicecheck::CheckStatusCode(const int status_code) {
}

if (status_code != net::HTTP_OK) {
BLOG(0, "Unexpected HTTP status: " << status_code);
return type::Result::LEDGER_ERROR;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ type::Result PostSafetynet::CheckStatusCode(const int status_code) {
}

if (status_code != net::HTTP_OK) {
BLOG(0, "Unexpected HTTP status: " << status_code);
return type::Result::LEDGER_ERROR;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ type::Result PostSuggestions::CheckStatusCode(const int status_code) {
}

if (status_code != net::HTTP_OK) {
BLOG(0, "Unexpected HTTP status: " << status_code);
return type::Result::LEDGER_ERROR;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ type::Result PostSuggestionsClaim::CheckStatusCode(const int status_code) {
}

if (status_code != net::HTTP_OK) {
BLOG(0, "Unexpected HTTP status: " << status_code);
return type::Result::LEDGER_ERROR;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ type::Result PostWalletBrave::CheckStatusCode(const int status_code) {
}

if (status_code != net::HTTP_CREATED) {
BLOG(0, "Unexpected HTTP status: " << status_code);
return type::Result::LEDGER_ERROR;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ type::Result PutCaptcha::CheckStatusCode(const int status_code) {
}

if (status_code != net::HTTP_OK) {
BLOG(0, "Unexpected HTTP status: " << status_code);
return type::Result::LEDGER_ERROR;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ type::Result PutDevicecheck::CheckStatusCode(const int status_code) {
}

if (status_code != net::HTTP_OK) {
BLOG(0, "Unexpected HTTP status: " << status_code);
return type::Result::LEDGER_ERROR;
}

Expand Down
Loading