From afc177ee210053bb3e9ecdbe5a59c42a293fa3fe Mon Sep 17 00:00:00 2001 From: Scott Graham Date: Fri, 2 Sep 2016 12:10:57 -0700 Subject: [PATCH] Pull metrics instrumentation out to central file Solves two problems with having the macros inline: 1. Deduplicates some of the logic (in this case, the name of the histogram, and whether it should be divided by 1024); 2. More useful check for compilation. As the macros are no-ops in Crashpad, it was easy to use the wrong name for a variable in the arguments to the macros (see .mm!) This way, we have some better chance of at least having code that compiles when built in Chromium if all the arguments are passed to Metrics::Something() in a standalone build. Also rolls mini_chromium DEPS to include: 99213eb Mark histogram arguments as unused to avoid warnings R=mark@chromium.org BUG=crashpad:100 Change-Id: I9f7fc3b85854fd61c1ebdf0084d728a7b690c2f1 Reviewed-on: https://chromium-review.googlesource.com/380445 Reviewed-by: Mark Mentovai --- DEPS | 2 +- client/crash_report_database_mac.mm | 5 ++-- client/crash_report_database_win.cc | 5 ++-- util/misc/metrics.cc | 27 +++++++++++++++++++ util/misc/metrics.h | 41 +++++++++++++++++++++++++++++ util/util.gyp | 2 ++ 6 files changed, 75 insertions(+), 7 deletions(-) create mode 100644 util/misc/metrics.cc create mode 100644 util/misc/metrics.h diff --git a/DEPS b/DEPS index 47db5a3d..6aa198d6 100644 --- a/DEPS +++ b/DEPS @@ -25,7 +25,7 @@ deps = { '93cc6e2c23e4d5ebd179f388e67aa907d0dfd43d', 'crashpad/third_party/mini_chromium/mini_chromium': Var('chromium_git') + '/chromium/mini_chromium@' + - 'b5ec9ce232a3a9d97e438ee38e27d65b5efbe1de', + '99213eb425034f947c72bb6fc92a6e46e4070591', 'buildtools': Var('chromium_git') + '/chromium/buildtools.git@' + 'f8fc76ea5ce4a60cda2fa5d7df3d4a62935b3113', diff --git a/client/crash_report_database_mac.mm b/client/crash_report_database_mac.mm index 3434a22a..861a699e 100644 --- a/client/crash_report_database_mac.mm +++ b/client/crash_report_database_mac.mm @@ -26,7 +26,6 @@ #include "base/logging.h" #include "base/mac/scoped_nsautorelease_pool.h" -#include "base/metrics/histogram_macros.h" #include "base/posix/eintr_wrapper.h" #include "base/scoped_generic.h" #include "base/strings/string_piece.h" @@ -36,6 +35,7 @@ #include "util/file/file_io.h" #include "util/mac/xattr.h" #include "util/misc/initialization_state_dcheck.h" +#include "util/misc/metrics.h" namespace crashpad { @@ -359,8 +359,7 @@ CrashReportDatabaseMac::FinishedWritingCrashReport(NewReport* report, return kFileSystemError; } - UMA_HISTOGRAM_MEMORY_KB("Crashpad.CrashReportSizeKB", - LoggingFileSizeByHandle(handle.get()) / 1024); + Metrics::CrashReportSize(report->handle); return kNoError; } diff --git a/client/crash_report_database_win.cc b/client/crash_report_database_win.cc index cbef0cc1..5a7db2d1 100644 --- a/client/crash_report_database_win.cc +++ b/client/crash_report_database_win.cc @@ -24,13 +24,13 @@ #include #include "base/logging.h" -#include "base/metrics/histogram_macros.h" #include "base/numerics/safe_math.h" #include "base/strings/string16.h" #include "base/strings/stringprintf.h" #include "base/strings/utf_string_conversions.h" #include "client/settings.h" #include "util/misc/initialization_state_dcheck.h" +#include "util/misc/metrics.h" namespace crashpad { @@ -679,8 +679,7 @@ OperationStatus CrashReportDatabaseWin::FinishedWritingCrashReport( ReportState::kPending)); *uuid = scoped_report->uuid; - UMA_HISTOGRAM_MEMORY_KB("Crashpad.CrashReportSizeKB", - LoggingFileSizeByHandle(handle.get()) / 1024); + Metrics::CrashReportSize(handle.get()); return kNoError; } diff --git a/util/misc/metrics.cc b/util/misc/metrics.cc new file mode 100644 index 00000000..6a363b13 --- /dev/null +++ b/util/misc/metrics.cc @@ -0,0 +1,27 @@ +// Copyright 2016 The Crashpad Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "util/misc/metrics.h" + +#include "base/metrics/histogram_macros.h" + +namespace crashpad { + +void Metrics::CrashReportSize(FileHandle file) { + const FileOffset size = LoggingFileSizeByHandle(file); + UMA_HISTOGRAM_CUSTOM_COUNTS( + "Crashpad.CrashReportSize", size, 0, 5 * 1024 * 1024, 50); +} + +} // namespace crashpad diff --git a/util/misc/metrics.h b/util/misc/metrics.h new file mode 100644 index 00000000..315a122f --- /dev/null +++ b/util/misc/metrics.h @@ -0,0 +1,41 @@ +// Copyright 2016 The Crashpad Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#ifndef CRASHPAD_UTIL_MISC_METRICS_H_ +#define CRASHPAD_UTIL_MISC_METRICS_H_ + +#include "base/macros.h" +#include "util/file/file_io.h" + +namespace crashpad { + +//! \brief Container class to hold shared UMA metrics integration points. +//! +//! Each static function in this class will call a `UMA_*` from +//! `base/metrics/histogram_macros.h`. When building Crashpad standalone, +//! (against mini_chromium), these macros do nothing. When built against +//! Chromium's base, they allow integration with its metrics system. +class Metrics { + public: + //! \brief Reports the size of a crash report file in bytes. Should be called + //! when a new report is written to disk. + static void CrashReportSize(FileHandle file); + + private: + DISALLOW_IMPLICIT_CONSTRUCTORS(Metrics); +}; + +} // namespace crashpad + +#endif // CRASHPAD_UTIL_MISC_METRICS_H_ diff --git a/util/util.gyp b/util/util.gyp index e8ad186e..7ce77de8 100644 --- a/util/util.gyp +++ b/util/util.gyp @@ -92,6 +92,8 @@ 'misc/initialization_state.h', 'misc/initialization_state_dcheck.cc', 'misc/initialization_state_dcheck.h', + 'misc/metrics.cc', + 'misc/metrics.h', 'misc/pdb_structures.cc', 'misc/pdb_structures.h', 'misc/random_string.cc',