Skip to content

Commit

Permalink
Avoid unnecessary loops in external_metrics.cc.
Browse files Browse the repository at this point in the history 
To test multiple histograms:

$ echo -e \
    '\x30\x00\x00\x00linearhistogram\x00Platform.Mutexlox 2 10 100\x00'\
    >> /var/lib/metrics/uma-events
wait 30s
check chrome://histograms/Mutexlox (should have 100 samples in bucket 2)

BUG=b:292583990
TEST=unit tests; tast run platform.Histograms

Change-Id: I048e283ead356c2d50697a73adfbfd925e37b9b1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4760046
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Commit-Queue: Miriam Zimmerman <mutexlox@chromium.org>
Reviewed-by: Luc Nguyen <lucnguyen@google.com>
Reviewed-by: Ian Barkley-Yeung <iby@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1181203}
  • Loading branch information
@mutexlox
mutexlox authored and Chromium LUCI CQ committed Aug 8, 2023
1 parent e00b6b2 commit b0936f9
Show file tree
Hide file tree
Showing 4 changed files with 75 additions and 53 deletions.
95 changes: 57 additions & 38 deletions chrome/browser/ash/external_metrics.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
#include "base/functional/bind.h"
#include "base/metrics/histogram.h"
#include "base/metrics/histogram_functions.h"
#include "base/metrics/sparse_histogram.h"
#include "base/metrics/statistics_recorder.h"
#include "base/metrics/user_metrics.h"
#include "base/strings/string_number_conversions.h"
Expand Down Expand Up @@ -86,24 +87,29 @@ scoped_refptr<ExternalMetrics> ExternalMetrics::CreateForTesting(
return external_metrics;
}

void ExternalMetrics::RecordActionUI(const std::string& action_string) {
base::RecordComputedAction(action_string);
void ExternalMetrics::RecordActionUI(const std::string& action_string,
int num_samples) {
for (int i = 0; i < num_samples; ++i) {
base::RecordComputedAction(action_string);
}
}

void ExternalMetrics::RecordAction(const std::string& action) {
void ExternalMetrics::RecordAction(const metrics::MetricSample& sample) {
content::GetUIThreadTaskRunner({})->PostTask(
FROM_HERE,
base::BindOnce(&ExternalMetrics::RecordActionUI, this, action));
FROM_HERE, base::BindOnce(&ExternalMetrics::RecordActionUI, this,
sample.name(), sample.num_samples()));
}

void ExternalMetrics::RecordCrashUI(const std::string& crash_kind) {
ChromeOSMetricsProvider::LogCrash(crash_kind);
void ExternalMetrics::RecordCrashUI(const std::string& crash_kind,
int num_samples) {
ChromeOSMetricsProvider::LogCrash(crash_kind, num_samples);
}

void ExternalMetrics::RecordCrash(const std::string& crash_kind) {
void ExternalMetrics::RecordCrash(const metrics::MetricSample& crash_sample) {
content::GetUIThreadTaskRunner({})->PostTask(
FROM_HERE,
base::BindOnce(&ExternalMetrics::RecordCrashUI, this, crash_kind));
base::BindOnce(&ExternalMetrics::RecordCrashUI, this, crash_sample.name(),
crash_sample.num_samples()));
}

void ExternalMetrics::RecordHistogram(const metrics::MetricSample& sample) {
Expand All @@ -114,8 +120,12 @@ void ExternalMetrics::RecordHistogram(const metrics::MetricSample& sample) {
return;
}

base::UmaHistogramCustomCounts(sample.name(), sample.sample(), sample.min(),
sample.max(), sample.bucket_count());
// We don't use base::UmaHistogramCustomCounts here because it doesn't support
// AddCount.
base::HistogramBase* histogram = base::Histogram::FactoryGet(
sample.name(), sample.min(), sample.max(), sample.bucket_count(),
base::HistogramBase::kUmaTargetedHistogramFlag);
histogram->AddCount(sample.sample(), sample.num_samples());
}

void ExternalMetrics::RecordLinearHistogram(
Expand All @@ -125,21 +135,32 @@ void ExternalMetrics::RecordLinearHistogram(
DLOG(ERROR) << "Invalid linear histogram: " << sample.name();
return;
}
base::UmaHistogramExactLinear(sample.name(), sample.sample(), sample.max());
// We don't use base::UmaHistogramExactLinear because it doesn't support
// AddCount.
base::HistogramBase* histogram = base::LinearHistogram::FactoryGet(
sample.name(), 1, sample.max(), static_cast<size_t>(sample.max() + 1),
base::HistogramBase::kUmaTargetedHistogramFlag);
histogram->AddCount(sample.sample(), sample.num_samples());
}

void ExternalMetrics::RecordSparseHistogram(
const metrics::MetricSample& sample) {
CHECK_EQ(metrics::MetricSample::SPARSE_HISTOGRAM, sample.type());
// We suspect a chromeos process reports a metric as regular and then later as
// a sparse enum histogram. See https://crbug.com/1173221
base::HistogramBase* histogram =
base::StatisticsRecorder::FindHistogram(sample.name());
if (histogram && histogram->GetHistogramType() != base::SPARSE_HISTOGRAM) {
LOG(FATAL) << "crbug.com/1173221 name " << sample.name() << " "
<< sample.ToString();
{
base::HistogramBase* histogram =
base::StatisticsRecorder::FindHistogram(sample.name());
if (histogram && histogram->GetHistogramType() != base::SPARSE_HISTOGRAM) {
LOG(FATAL) << "crbug.com/1173221 name " << sample.name() << " "
<< sample.ToString();
}
}
base::UmaHistogramSparse(sample.name(), sample.sample());

// We don't use base::UmaHistogramSparse because it doesn't support AddCount.
base::HistogramBase* histogram = base::SparseHistogram::FactoryGet(
sample.name(), base::HistogramBase::kUmaTargetedHistogramFlag);
histogram->AddCount(sample.sample(), sample.num_samples());
}

int ExternalMetrics::CollectEvents() {
Expand All @@ -153,26 +174,24 @@ int ExternalMetrics::CollectEvents() {

cumulative_num_samples += sample.num_samples();

for (int i = 0; i < sample.num_samples(); ++i) {
// Do not use the UMA_HISTOGRAM_... macros here. They cache the Histogram
// instance and thus only work if |sample.name()| is constant.
switch (sample.type()) {
case metrics::MetricSample::CRASH:
RecordCrash(sample.name());
break;
case metrics::MetricSample::USER_ACTION:
RecordAction(sample.name());
break;
case metrics::MetricSample::HISTOGRAM:
RecordHistogram(sample);
break;
case metrics::MetricSample::LINEAR_HISTOGRAM:
RecordLinearHistogram(sample);
break;
case metrics::MetricSample::SPARSE_HISTOGRAM:
RecordSparseHistogram(sample);
break;
}
// Do not use the UMA_HISTOGRAM_... macros here. They cache the Histogram
// instance and thus only work if |sample.name()| is constant.
switch (sample.type()) {
case metrics::MetricSample::CRASH:
RecordCrash(sample);
break;
case metrics::MetricSample::USER_ACTION:
RecordAction(sample);
break;
case metrics::MetricSample::HISTOGRAM:
RecordHistogram(sample);
break;
case metrics::MetricSample::LINEAR_HISTOGRAM:
RecordLinearHistogram(sample);
break;
case metrics::MetricSample::SPARSE_HISTOGRAM:
RecordSparseHistogram(sample);
break;
}
}

Expand Down
8 changes: 4 additions & 4 deletions chrome/browser/ash/external_metrics.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,17 +59,17 @@ class ExternalMetrics : public base::RefCountedThreadSafe<ExternalMetrics> {
~ExternalMetrics();

// Passes an action event to the UMA service on the UI thread.
void RecordActionUI(const std::string& action_string);
void RecordActionUI(const std::string& action_string, int num_samples);

// Passes an action event to the UMA service.
void RecordAction(const std::string& action_name);
void RecordAction(const metrics::MetricSample& sample);

// Records an external crash of the given string description to
// UMA service on the UI thread.
void RecordCrashUI(const std::string& crash_kind);
void RecordCrashUI(const std::string& crash_kind, int num_samples);

// Records an external crash of the given string description.
void RecordCrash(const std::string& crash_kind);
void RecordCrash(const metrics::MetricSample& sample);

// Records an histogram. |sample| is expected to be an histogram.
void RecordHistogram(const metrics::MetricSample& sample);
Expand Down
23 changes: 13 additions & 10 deletions chrome/browser/metrics/chromeos_metrics_provider.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,11 +65,11 @@ using metrics::SystemProfileProto;

namespace {

void IncrementPrefValue(const char* path) {
void IncrementPrefValue(const char* path, int num_samples) {
PrefService* pref = g_browser_process->local_state();
DCHECK(pref);
int value = pref->GetInteger(path);
pref->SetInteger(path, value + 1);
pref->SetInteger(path, value + num_samples);
}

} // namespace
Expand All @@ -93,15 +93,18 @@ void ChromeOSMetricsProvider::RegisterPrefs(PrefRegistrySimple* registry) {
}

// static
void ChromeOSMetricsProvider::LogCrash(const std::string& crash_type) {
if (crash_type == "user")
IncrementPrefValue(prefs::kStabilityOtherUserCrashCount);
else if (crash_type == "kernel")
IncrementPrefValue(prefs::kStabilityKernelCrashCount);
else if (crash_type == "uncleanshutdown")
IncrementPrefValue(prefs::kStabilitySystemUncleanShutdownCount);
else
void ChromeOSMetricsProvider::LogCrash(const std::string& crash_type,
int num_samples) {
if (crash_type == "user") {
IncrementPrefValue(prefs::kStabilityOtherUserCrashCount, num_samples);
} else if (crash_type == "kernel") {
IncrementPrefValue(prefs::kStabilityKernelCrashCount, num_samples);
} else if (crash_type == "uncleanshutdown") {
IncrementPrefValue(prefs::kStabilitySystemUncleanShutdownCount,
num_samples);
} else {
NOTREACHED() << "Unexpected Chrome OS crash type " << crash_type;
}

// Wake up metrics logs sending if necessary now that new
// log data is available.
Expand Down
2 changes: 1 addition & 1 deletion chrome/browser/metrics/chromeos_metrics_provider.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ class ChromeOSMetricsProvider : public metrics::MetricsProvider {
static void RegisterPrefs(PrefRegistrySimple* registry);

// Records a crash.
static void LogCrash(const std::string& crash_type);
static void LogCrash(const std::string& crash_type, int num_samples);

// Returns Enterprise Enrollment status.
static EnrollmentStatus GetEnrollmentStatus();
Expand Down

0 comments on commit b0936f9

Please sign in to comment.