Skip to content

Improve some P3A metrics #3931

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Jan 3, 2020
Merged

Improve some P3A metrics #3931

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e063bcf
Issue 6841: Fix "Is default browser" P3A metric.
Nov 8, 2019
1e2df58
Issue 6841: Fix the ads state P3A metric.
Nov 8, 2019
14620a0
Issue 6841: Drop unused P3A values from local state.
Dec 2, 2019
b260b70
Issue 6841: Please linter.
Dec 16, 2019
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
3 changes: 2 additions & 1 deletion components/brave_ads/browser/ads_service_factory.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,8 @@ void AdsServiceFactory::RegisterProfilePrefs(
registry->RegisterUint64Pref(prefs::kAdsPerDay, 20);

registry->RegisterIntegerPref(prefs::kIdleThreshold, 15);
registry->RegisterBooleanPref(prefs::kBraveAdsWereDisabled, false);
registry->RegisterBooleanPref(prefs::kAdsWereDisabled, false);
registry->RegisterBooleanPref(prefs::kHasAdsP3AState, false);

registry->RegisterBooleanPref(prefs::kShouldShowMyFirstAdNotification, true);

Expand Down
6 changes: 5 additions & 1 deletion components/brave_ads/common/pref_names.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,11 @@ namespace prefs {
const char kEnabled[] = "brave.brave_ads.enabled";

// Stores whether ads were disabled at least once
const char kBraveAdsWereDisabled[] = "brave.brave_ads.were_disabled";
const char kAdsWereDisabled[] = "brave.brave_ads.were_disabled";

// Indicates whether we have any initial state of the ads status metric, besides
// "No Wallet".
const char kHasAdsP3AState[] = "brave.brave_ads.has_p3a_state";

// Stores the maximum amount of ads per hour
const char kAdsPerHour[] = "brave.brave_ads.ads_per_hour";
Expand Down
3 changes: 2 additions & 1 deletion components/brave_ads/common/pref_names.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@ extern const char kEnabled[];

extern const char kAdsPerHour[];
extern const char kAdsPerDay[];
extern const char kBraveAdsWereDisabled[];
extern const char kAdsWereDisabled[];
extern const char kHasAdsP3AState[];

extern const char kIdleThreshold[];

Expand Down
15 changes: 12 additions & 3 deletions components/brave_rewards/browser/rewards_p3a.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,17 +88,17 @@ void UpdateAdsP3AOnPreferenceChange(PrefService *prefs,
if (pref == brave_ads::prefs::kEnabled) {
if (prefs->GetBoolean(brave_ads::prefs::kEnabled)) {
brave_rewards::RecordAdsState(AdsP3AState::kAdsEnabled);
prefs->SetBoolean(brave_ads::prefs::kBraveAdsWereDisabled, false);
prefs->SetBoolean(brave_ads::prefs::kAdsWereDisabled, false);
} else {
// Apparently, the pref was disabled.
brave_rewards::RecordAdsState(
rewards_enabled ? AdsP3AState::kAdsEnabledThenDisabledRewardsOn :
AdsP3AState::kAdsEnabledThenDisabledRewardsOff);
prefs->SetBoolean(brave_ads::prefs::kBraveAdsWereDisabled, true);
prefs->SetBoolean(brave_ads::prefs::kAdsWereDisabled, true);
}
} else if (pref == brave_rewards::prefs::kBraveRewardsEnabled) {
// Rewards pref was changed
if (prefs->GetBoolean(brave_ads::prefs::kBraveAdsWereDisabled)) {
if (prefs->GetBoolean(brave_ads::prefs::kAdsWereDisabled)) {
brave_rewards::RecordAdsState(
rewards_enabled ? AdsP3AState::kAdsEnabledThenDisabledRewardsOn :
AdsP3AState::kAdsEnabledThenDisabledRewardsOff);
Expand All @@ -107,6 +107,15 @@ void UpdateAdsP3AOnPreferenceChange(PrefService *prefs,
}
}

void MaybeRecordInitialAdsP3AState(PrefService* prefs) {
if (!prefs->GetBoolean(brave_ads::prefs::kHasAdsP3AState)) {
const bool ads_state = prefs->GetBoolean(brave_ads::prefs::kEnabled);
RecordAdsState(ads_state ? AdsP3AState::kAdsEnabled
: AdsP3AState::kAdsDisabled);
prefs->SetBoolean(brave_ads::prefs::kHasAdsP3AState, true);
}
}

void RecordNoWalletCreatedForAllMetrics() {
RecordWalletBalanceP3A(false, 0);
RecordAutoContributionsState(AutoContributionsP3AState::kNoWallet, 0);
Expand Down
4 changes: 4 additions & 0 deletions components/brave_rewards/browser/rewards_p3a.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,10 @@ void RecordAdsState(AdsP3AState state);
void UpdateAdsP3AOnPreferenceChange(PrefService* prefs,
const std::string& pref);

// Records an initial metric state ("disabled" or "enabled") if it was not done
// before. Intended to be called if the user has already created a wallet.
void MaybeRecordInitialAdsP3AState(PrefService* local_state);
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be prefs instead of local_state?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope, currently all metrics are intentionally stored in global state, so using it here is also fine. Atm we are ok with it

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I should be more clear. I am talking about variable name. Because in h file you have MaybeRecordInitialAdsP3AState(PrefService* local_state), but in cc you have MaybeRecordInitialAdsP3AState(PrefService* prefs)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, thanks, good catch


void RecordNoWalletCreatedForAllMetrics();

double CalcWalletBalanceForP3A(base::flat_map<std::string, double> wallets,
Expand Down
3 changes: 2 additions & 1 deletion components/brave_rewards/browser/rewards_service_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -927,7 +927,7 @@ void RewardsServiceImpl::OnWalletInitialized(ledger::Result result) {
RecordWalletBalanceP3A(true, 0);
#if BUILDFLAG(BRAVE_ADS_ENABLED)
const bool ads_enabled =
profile_->GetPrefs()->GetBoolean(brave_ads::prefs::kEnabled);
profile_->GetPrefs()->GetBoolean(brave_ads::prefs::kEnabled);
RecordAdsState(ads_enabled ? AdsP3AState::kAdsEnabled
: AdsP3AState::kAdsDisabled);
#endif
Expand Down Expand Up @@ -1069,6 +1069,7 @@ void RewardsServiceImpl::OnLedgerStateLoaded(
}
// Record stats.
RecordBackendP3AStats();
MaybeRecordInitialAdsP3AState(profile_->GetPrefs());
}
if (state.first.empty()) {
RecordNoWalletCreatedForAllMetrics();
Expand Down
21 changes: 21 additions & 0 deletions components/p3a/brave_histogram_rewrite.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,13 @@ namespace {
// Please keep this list sorted and synced with |DoHistogramBravezation|.
constexpr const char* kBravezationHistograms[] = {
"Bookmarks.Count.OnProfileLoad",
"DefaultBrowser.State",
"Extensions.LoadExtension",
"Tabs.TabCount",
"Tabs.WindowCount",
};

// TODO(iefremov): Replace a bunch of 'if's with something more elegant.
// Records the given sample using the proper Brave way.
void DoHistogramBravezation(base::StringPiece histogram_name,
base::HistogramBase::Sample sample) {
Expand All @@ -40,6 +42,25 @@ void DoHistogramBravezation(base::StringPiece histogram_name,
return;
}

if ("DefaultBrowser.State" == histogram_name) {
int answer = 0;
switch (sample) {
case 0: // Not default.
case 1: // Default.
answer = sample;
break;
case 2: // Unknown, merging to "Not default".
answer = 0;
break;
case 3: // Other mode is default, merging to "Default".
answer = 1;
break;
default:
NOTREACHED();
}
UMA_HISTOGRAM_BOOLEAN("Brave.Core.IsDefault", answer);
}

if ("Extensions.LoadExtension" == histogram_name) {
int answer = 0;
if (sample == 1)
Expand Down
20 changes: 14 additions & 6 deletions components/p3a/brave_p3a_log_store.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,10 @@ void RecordP3A(uint64_t answers_count) {

} // namespace

BraveP3ALogStore::BraveP3ALogStore(LogSerializer* serializer,
BraveP3ALogStore::BraveP3ALogStore(Delegate* delegate,
PrefService* local_state)
: serializer_(serializer), local_state_(local_state) {
DCHECK(serializer_);
: delegate_(delegate), local_state_(local_state) {
DCHECK(delegate_);
DCHECK(local_state);
}

Expand Down Expand Up @@ -123,7 +123,8 @@ void BraveP3ALogStore::StageNextLog() {
DCHECK(!log_.find(staged_entry_key_)->second.sent);

uint64_t staged_entry_value = log_[staged_entry_key_].value;
staged_log_ = serializer_->Serialize(staged_entry_key_, staged_entry_value);
staged_log_ = delegate_->Serialize(staged_entry_key_, staged_entry_value);

VLOG(2) << "BraveP3ALogStore::StageNextLog: staged " << staged_entry_key_;
}

Expand Down Expand Up @@ -161,10 +162,17 @@ void BraveP3ALogStore::LoadPersistedUnsentLogs() {
DCHECK(log_.empty());
DCHECK(unsent_entries_.empty());

const base::DictionaryValue* list = local_state_->GetDictionary(kPrefName);
DictionaryPrefUpdate update(local_state_, kPrefName);
base::DictionaryValue* list = update.Get();
for (auto dict_item : list->DictItems()) {
LogEntry entry;
std::string name = dict_item.first;
const std::string name = dict_item.first;
// Check if the metric is obsolete.
if (!delegate_->IsActualMetric(name)) {
// Drop it from the local state.
list->RemoveKey(name);
continue;
}
const base::Value& dict = dict_item.second;
// Value.
if (const base::Value* v =
Expand Down
12 changes: 7 additions & 5 deletions components/p3a/brave_p3a_log_store.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,15 +25,17 @@ namespace brave {
// for now persisted entries never expire.
class BraveP3ALogStore : public metrics::LogStore {
public:
// Prepares a string representaion of an entry.
class LogSerializer {
class Delegate {
public:
// Prepares a string representaion of an entry.
virtual std::string Serialize(base::StringPiece histogram_name,
uint64_t value) const = 0;
virtual ~LogSerializer() {}
// Returns false if the metric is obsolete and should be cleaned up.
virtual bool IsActualMetric(base::StringPiece histogram_name) const = 0;
virtual ~Delegate() {}
};

explicit BraveP3ALogStore(LogSerializer* serializer,
explicit BraveP3ALogStore(Delegate* delegate,
PrefService* local_state);

// TODO(iefremov): Make parent destructor virtual?
Expand Down Expand Up @@ -78,7 +80,7 @@ class BraveP3ALogStore : public metrics::LogStore {
base::Time sent_timestamp; // At the moment only for debugging purposes.
};

const LogSerializer* const serializer_ = nullptr; // Weak.
const Delegate* const delegate_ = nullptr; // Weak.
PrefService* const local_state_ = nullptr;

// TODO(iefremov): Try to replace with base::StringPiece?
Expand Down
12 changes: 11 additions & 1 deletion components/p3a/brave_p3a_service.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,11 +51,13 @@ constexpr uint64_t kDefaultUploadIntervalSeconds = 60 * 60; // 1 hour.
constexpr const char* kCollectedHistograms[] = {
"Brave.P3A.SentAnswersCount",
"Brave.Sync.Status",
"DefaultBrowser.State",
// Deprecated:
// "DefaultBrowser.State",
"Brave.Importer.ImporterSource",
"Brave.Shields.UsageStatus",
// Do not gather detailed info regarding TOR usage for now.
// "Brave.Core.LastTimeTorUsed",
"Brave.Core.IsDefault",
"Brave.Core.TorEverUsed",
"Brave.Core.LastTimeIncognitoUsed",
"Brave.Core.NumberOfExtensions",
Expand Down Expand Up @@ -205,6 +207,14 @@ std::string BraveP3AService::Serialize(base::StringPiece histogram_name,
return message.SerializeAsString();
}

bool
BraveP3AService::IsActualMetric(base::StringPiece histogram_name) const {
static const base::NoDestructor<base::flat_set<base::StringPiece>>
metric_names {std::begin(kCollectedHistograms),
std::end(kCollectedHistograms)};
return metric_names->contains(histogram_name);
}

void BraveP3AService::MaybeOverrideSettingsFromCommandLine() {
base::CommandLine* cmdline = base::CommandLine::ForCurrentProcess();

Expand Down
6 changes: 4 additions & 2 deletions components/p3a/brave_p3a_service.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ class BraveP3AUploader;
// on any thread.
// TODO(iefremov): It should be possible to get rid of refcounted here.
class BraveP3AService : public base::RefCountedThreadSafe<BraveP3AService>,
public BraveP3ALogStore::LogSerializer {
public BraveP3ALogStore::Delegate {
public:
explicit BraveP3AService(PrefService* local_state);

Expand All @@ -43,10 +43,12 @@ class BraveP3AService : public base::RefCountedThreadSafe<BraveP3AService>,
// Needs a living browser process to complete the initialization.
void Init();

// BraveP3ALogStore::LogSerializer
// BraveP3ALogStore::Delegate
std::string Serialize(base::StringPiece histogram_name,
uint64_t value) const override;

bool IsActualMetric(base::StringPiece histogram_name) const override;

private:
friend class base::RefCountedThreadSafe<BraveP3AService>;
~BraveP3AService() override;
Expand Down