Refactor histogram_macros.h. This improves documentation for the macros in the file, moves LOCAL_* …

base/metrics/histogram_macros.h

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef BASE_METRICS_HISTOGRAM_MACROS_H_
 #define BASE_METRICS_HISTOGRAM_MACROS_H_
 
-#include "base/atomicops.h"
 #include "base/logging.h"

[Mark P, 2016/09/23 19:30:42]

I think this doesn't need logging anymore.

[rkaplow, 2016/09/26 20:16:10]

Done.

 #include "base/metrics/histogram.h"
+#include "base/metrics/histogram_macros_internal.h"
+#include "base/metrics/local_histogram_macros.h"
 #include "base/time/time.h"
 
-// Macros for efficient use of histograms. See documentation in histogram.h.
-//
+// TODO(nikunjb): Move sparse macros here.

[Mark P, 2016/09/23 19:30:41]

nit: here -> to this file

[rkaplow, 2016/09/26 20:16:09]

Done.

 // UMA_HISTOGRAM_SPARSE_SLOWLY is defined in sparse_histogram.h as it has
 // different #include dependencies.
 
-//------------------------------------------------------------------------------
-// Histograms are often put in areas where they are called many many times, and
-// performance is critical.  As a result, they are designed to have a very low
-// recurring cost of executing (adding additional samples).  Toward that end,
-// the macros declare a static pointer to the histogram in question, and only
-// take a "slow path" to construct (or find) the histogram on the first run
-// through the macro.  We leak the histograms at shutdown time so that we don't
-// have to validate using the pointers at any time during the running of the
-// process.
-
-// The following code is generally what a thread-safe static pointer
-// initialization looks like for a histogram (after a macro is expanded).  This
-// sample is an expansion (with comments) of the code for
-// LOCAL_HISTOGRAM_CUSTOM_COUNTS().
-
-/*
-  do {
-    // The pointer's presence indicates the initialization is complete.
-    // Initialization is idempotent, so it can safely be atomically repeated.
-    static base::subtle::AtomicWord atomic_histogram_pointer = 0;
-
-    // Acquire_Load() ensures that we acquire visibility to the pointed-to data
-    // in the histogram.
-    base::Histogram* histogram_pointer(reinterpret_cast<base::Histogram*>(
-        base::subtle::Acquire_Load(&atomic_histogram_pointer)));
-
-    if (!histogram_pointer) {
-      // This is the slow path, which will construct OR find the matching
-      // histogram.  FactoryGet includes locks on a global histogram name map
-      // and is completely thread safe.
-      histogram_pointer = base::Histogram::FactoryGet(
-          name, min, max, bucket_count, base::HistogramBase::kNoFlags);
-
-      // Use Release_Store to ensure that the histogram data is made available
-      // globally before we make the pointer visible.
-      // Several threads may perform this store, but the same value will be
-      // stored in all cases (for a given named/spec'ed histogram).
-      // We could do this without any barrier, since FactoryGet entered and
-      // exited a lock after construction, but this barrier makes things clear.
-      base::subtle::Release_Store(&atomic_histogram_pointer,
-          reinterpret_cast<base::subtle::AtomicWord>(histogram_pointer));
-    }
-
-    // Ensure calling contract is upheld, and the name does NOT vary.
-    DCHECK(histogram_pointer->histogram_name() == constant_histogram_name);
-
-    histogram_pointer->Add(sample);
-  } while (0);
-*/
-
-// The above pattern is repeated in several macros.  The only elements that
-// vary are the invocation of the Add(sample) vs AddTime(sample), and the choice
-// of which FactoryGet method to use.  The different FactoryGet methods have
-// various argument lists, so the function with its argument list is provided as
-// a macro argument here.  The name is only used in a DCHECK, to assure that
-// callers don't try to vary the name of the histogram (which would tend to be
-// ignored by the one-time initialization of the histogtram_pointer).
-
-// In some cases (integration into 3rd party code), it's useful to seperate the
-// definition of |atomic_histogram_poiner| from its use. To achieve this we
-// define HISTOGRAM_POINTER_USE, which uses an |atomic_histogram_pointer|, and
-// STATIC_HISTOGRAM_POINTER_BLOCK, which defines an |atomic_histogram_pointer|
-// and forwards to HISTOGRAM_POINTER_USE.
-#define HISTOGRAM_POINTER_USE(atomic_histogram_pointer,                   \
-                              constant_histogram_name,                    \
-                              histogram_add_method_invocation,            \
-                              histogram_factory_get_invocation)           \
-  do {                                                                    \
-    base::HistogramBase* histogram_pointer(                               \
-        reinterpret_cast<base::HistogramBase*>(                           \
-            base::subtle::Acquire_Load(atomic_histogram_pointer)));       \
-    if (!histogram_pointer) {                                             \
-      histogram_pointer = histogram_factory_get_invocation;               \
-      base::subtle::Release_Store(                                        \
-          atomic_histogram_pointer,                                       \
-          reinterpret_cast<base::subtle::AtomicWord>(histogram_pointer)); \
-    }                                                                     \
-    if (DCHECK_IS_ON())                                                   \
-      histogram_pointer->CheckName(constant_histogram_name);              \
-    histogram_pointer->histogram_add_method_invocation;                   \
-  } while (0)
-
-// Defines the static |atomic_histogram_pointer| and forwards to
-// HISTOGRAM_POINTER_USE.
-#define STATIC_HISTOGRAM_POINTER_BLOCK(constant_histogram_name,               \

[nikunjb1, 2016/09/23 21:14:12]

I see usage of this outside histogram_macros.h e.g
//src/components/metrics/system_memory_stats_recorder_linux.cc

and 
src/chromecast/base/metrics/cast_histograms.h

Since this is only moved it won't have compilation issues, but is it okay keep
them using internal implementations?

[rkaplow, 2016/09/26 20:16:10]

Probably not ideal - we'd need to see why they were doing this. 

For example
https://codesearch.chromium.org/chromium/src/components/metrics/system_memory...
- not sure this was needed.

One thing we don't make easy to do is a numeric linear histogram. I wonder if we
should add that for cases where people might want it.

I'd rather not do in this CL but we could - what do you guys think?

[Mark P, 2016/09/26 23:06:47]

Generally I've been advising people to use ENUMERATION when they want linear
histograms.  (It's only a small encouragement; I don't feel strongly.)  It's
hard to screw up the bucketing if you change things later.

Maybe we should simply add a LINEAR macro instead?

[Mark P, 2016/09/29 23:15:00]

What do you think about this suggestion?

-                                       histogram_add_method_invocation,       \
-                                       histogram_factory_get_invocation)      \
-  do {                                                                        \
-    static base::subtle::AtomicWord atomic_histogram_pointer = 0;             \
-    HISTOGRAM_POINTER_USE(&atomic_histogram_pointer, constant_histogram_name, \
-                          histogram_add_method_invocation,                    \
-                          histogram_factory_get_invocation);                  \
-  } while (0)
-
-//------------------------------------------------------------------------------
-// Provide easy general purpose histogram in a macro, just like stats counters.
-// Most of these macros use 50 buckets, but check the definition for details.
-//
-// All of these macros must be called with |name| as a runtime constant --- it

[Mark P, 2016/09/23 19:30:41]

This comments needs to stay in this file, prominently.  Perhaps in each section?
 (You make the comment about exponential-size buckets prominent in each section.
 This seems more important to me.)

[rkaplow, 2016/09/26 20:16:10]

made a less technical version at the top of the file.
Add one liners at the beginning of each section

-// doesn't have to literally be a constant, but it must be the same string on
-// all calls from a particular call site. If this rule is violated,
-// STATIC_HISTOGRAM_POINTER_BLOCK will DCHECK, and if DCHECKS are disabled, the
-// data will be written to the wrong histogram.
-
-#define LOCAL_HISTOGRAM_TIMES(name, sample) LOCAL_HISTOGRAM_CUSTOM_TIMES( \
-    name, sample, base::TimeDelta::FromMilliseconds(1), \
+// TODO(rkaplow): Link to proper documentation on metric creation once we have
+// it in a good state.
+
+//------------------------------------------------------------------------------
+// Count histograms. These are used for collecting numeric data. Note that we
+// have macros for more specialized use cases below (memory, time, percentages).
+
+// The number suffixes here refer to the max size of the sample, i.e. COUNT_1000
+// will be able to collect samples of counts up to 1000. The default number of
+// buckets in all default macros is 50.
+// Note that all these macros default to exponential histograms - i.e. the

[Mark P, 2016/09/23 19:30:41]

nit: omit "Note that"

[rkaplow, 2016/09/26 20:16:10]

Done.

+// lengths of the bucket ranges exponentially increase as the sample range
+// increases.
+// These should *not* be used if you are interested in exact counts, i.e. a
+// bucket range of 1. In these cases, you should use the ENUMERATION macros
+// defined later.
+
+// Sample Usage:
+// UMA_HISTOGRAM_COUNTS_100("My.Histogram", sample);
+
+#define UMA_HISTOGRAM_COUNTS_100(name, sample) UMA_HISTOGRAM_CUSTOM_COUNTS(    \
+    name, sample, 1, 100, 50)
+
+#define UMA_HISTOGRAM_COUNTS_1000(name, sample) UMA_HISTOGRAM_CUSTOM_COUNTS(   \
+    name, sample, 1, 1000, 50)
+
+#define UMA_HISTOGRAM_COUNTS_10000(name, sample) UMA_HISTOGRAM_CUSTOM_COUNTS(  \
+    name, sample, 1, 10000, 50)
+
+#define UMA_HISTOGRAM_COUNTS_100000(name, sample) UMA_HISTOGRAM_CUSTOM_COUNTS( \
+    name, sample, 1, 100000, 50)
+
+#define UMA_HISTOGRAM_COUNTS_1000000(name, sample) UMA_HISTOGRAM_CUSTOM_COUNTS(\
+    name, sample, 1, 1000000, 50)
+
+// This can be used when the default ranges are not sufficient. This macro lets
+// the metric developer customize the min and max of the sampled range, as well
+// as the number of buckets recorded. Usage:

[Mark P, 2016/09/23 19:30:41]

Please mention here that values less than the min are always included in an
underflow bucket.  This is makes sense to always use a min of 1 even if you're
going to emit 0 values to the histogram.

[rkaplow, 2016/09/26 20:16:10]

gave a quick answer - think it's worth putting an example or this clear enough?

+// UMA_HISTOGRAM_CUSTOM_COUNTS("My.Histogram", 1, 100000000, 100);
+#define UMA_HISTOGRAM_CUSTOM_COUNTS(name, sample, min, max, bucket_count)      \

[nikunjb1, 2016/09/23 21:14:12]

Small comment: Generally it is easier to follow the code if the most general
#define is at the top and special values are below it.

So, first define UMA_HISTOGRAM_CUSTOM_COUNTS and then specialize the macro with
counts like UMA_HISTOGRAM_COUNTS_100

(Same with UMA_HISTOGRAM_CUSTOM_TIMES)

[Mark P, 2016/09/23 21:24:08]

I agree that Nik's suggestion would make the code easier to read.  However, I
think we should offer the more common macros first.  I prefer this order. 
People should only have to read down to CUSTOM_whatever if they don't see
anything they like sooner.

[rkaplow, 2016/09/26 20:16:10]

That was my reasoning for placing it this way - I'd rather them see the easy
macros first.

+    INTERNAL_HISTOGRAM_CUSTOM_COUNTS_WITH_FLAG(                                \
+        name, sample, min, max, bucket_count,                                  \
+        base::HistogramBase::kUmaTargetedHistogramFlag)
+
+
+//------------------------------------------------------------------------------
+// Timing histograms. These are used for collecting timing data (generally
+// latencies).
+
+// These macros create exponentially sized histograms (lengths of the bucket
+// ranges exponentially increase as the sample range increases). The input
+// sample must be a number measured in ms.

[Mark P, 2016/09/23 19:30:42]

This last sentence is wrong I think.  Isn't the input a TimeDuration object?

Did you mean to say that the value emitted will be recorded at millisecond
granularity?

If so, you might want to clarify the sample usage while you're here.

[rkaplow, 2016/09/26 20:16:10]

Ah, thanks.

+
+// Sample Usage:
+// UMA_HISTOGRAM_TIMES("My.Timing.Histogram", duration_in_ms);
+
+// Short timings - up to 10 seconds.
+#define UMA_HISTOGRAM_TIMES(name, sample) UMA_HISTOGRAM_CUSTOM_TIMES(          \
+    name, sample, base::TimeDelta::FromMilliseconds(1),                        \
     base::TimeDelta::FromSeconds(10), 50)
 
-// For folks that need real specific times, use this to select a precise range
-// of times you want plotted, and the number of buckets you want used.
-#define LOCAL_HISTOGRAM_CUSTOM_TIMES(name, sample, min, max, bucket_count) \
-    STATIC_HISTOGRAM_POINTER_BLOCK(name, AddTime(sample), \
-        base::Histogram::FactoryTimeGet(name, min, max, bucket_count, \
-                                        base::HistogramBase::kNoFlags))
-
-#define LOCAL_HISTOGRAM_COUNTS(name, sample) LOCAL_HISTOGRAM_CUSTOM_COUNTS( \
-    name, sample, 1, 1000000, 50)
-
-#define LOCAL_HISTOGRAM_COUNTS_100(name, sample) \
-    LOCAL_HISTOGRAM_CUSTOM_COUNTS(name, sample, 1, 100, 50)
-
-#define LOCAL_HISTOGRAM_COUNTS_10000(name, sample) \
-    LOCAL_HISTOGRAM_CUSTOM_COUNTS(name, sample, 1, 10000, 50)
-
-#define LOCAL_HISTOGRAM_CUSTOM_COUNTS(name, sample, min, max, bucket_count) \
-    INTERNAL_HISTOGRAM_CUSTOM_COUNTS_WITH_FLAG(                             \
-        name, sample, min, max, bucket_count, base::HistogramBase::kNoFlags)
-
-// This is a helper macro used by other macros and shouldn't be used directly.
-#define INTERNAL_HISTOGRAM_CUSTOM_COUNTS_WITH_FLAG(name, sample, min, max, \
-                                                   bucket_count, flag)     \
-    STATIC_HISTOGRAM_POINTER_BLOCK(                                        \
-        name, Add(sample),                                                 \
-        base::Histogram::FactoryGet(name, min, max, bucket_count, flag))
-
-// This is a helper macro used by other macros and shouldn't be used directly.
-// One additional bucket is created in the LinearHistogram for the illegal
-// values >= boundary_value so that mistakes in calling the UMA enumeration
-// macros can be detected.
-#define INTERNAL_HISTOGRAM_ENUMERATION_WITH_FLAG(name, sample, boundary, flag) \
-    STATIC_HISTOGRAM_POINTER_BLOCK(                                            \
-        name, Add(sample),                                                     \
-        base::LinearHistogram::FactoryGet(                                     \
-            name, 1, boundary, boundary + 1, flag))
-
-#define LOCAL_HISTOGRAM_PERCENTAGE(name, under_one_hundred) \
-    LOCAL_HISTOGRAM_ENUMERATION(name, under_one_hundred, 101)
-
-#define LOCAL_HISTOGRAM_BOOLEAN(name, sample) \
-    STATIC_HISTOGRAM_POINTER_BLOCK(name, AddBoolean(sample), \
-        base::BooleanHistogram::FactoryGet(name, base::Histogram::kNoFlags))
-
-// Support histograming of an enumerated value.  The samples should always be
-// strictly less than |boundary_value| -- this prevents you from running into
-// problems down the line if you add additional buckets to the histogram.  Note
-// also that, despite explicitly setting the minimum bucket value to |1| below,
-// it is fine for enumerated histograms to be 0-indexed -- this is because
-// enumerated histograms should never have underflow. One additional bucket is
-// created in the LinearHistogram for the illegal values >= boundary_value so
-// that mistakes in calling this macro can be detected.
-#define LOCAL_HISTOGRAM_ENUMERATION(name, sample, boundary_value) \
-    STATIC_HISTOGRAM_POINTER_BLOCK(name, Add(sample), \
-        base::LinearHistogram::FactoryGet(name, 1, boundary_value, \
-            boundary_value + 1, base::HistogramBase::kNoFlags))
-
-// Support histograming of an enumerated value. Samples should be one of the

[Mark P, 2016/09/23 19:30:42]

Please keep this comment by the CUSTOM_ENUMERATION histogram, even if you are
discovering people from using it.

[rkaplow, 2016/09/26 20:16:10]

This was attached to the local version - I assume we'd rather put it with the
UMA version now, right?
I put it with the UMA version as I just made the comment in the local_ section
to refer to UMA documentation.

-// std::vector<int> list provided via |custom_ranges|. See comments above
-// CustomRanges::FactoryGet about the requirement of |custom_ranges|.
-// You can use the helper function CustomHistogram::ArrayToCustomRanges to
-// transform a C-style array of valid sample values to a std::vector<int>.
-#define LOCAL_HISTOGRAM_CUSTOM_ENUMERATION(name, sample, custom_ranges) \
-    STATIC_HISTOGRAM_POINTER_BLOCK(name, Add(sample), \
-        base::CustomHistogram::FactoryGet(name, custom_ranges, \
-                                          base::HistogramBase::kNoFlags))
-
-#define LOCAL_HISTOGRAM_MEMORY_KB(name, sample) LOCAL_HISTOGRAM_CUSTOM_COUNTS( \
-    name, sample, 1000, 500000, 50)
-
-//------------------------------------------------------------------------------
-// The following macros provide typical usage scenarios for callers that wish
-// to record histogram data, and have the data submitted/uploaded via UMA.
-// Not all systems support such UMA, but if they do, the following macros
-// should work with the service.
-
-#define UMA_HISTOGRAM_TIMES(name, sample) UMA_HISTOGRAM_CUSTOM_TIMES( \
-    name, sample, base::TimeDelta::FromMilliseconds(1), \
-    base::TimeDelta::FromSeconds(10), 50)
-
-#define UMA_HISTOGRAM_MEDIUM_TIMES(name, sample) UMA_HISTOGRAM_CUSTOM_TIMES( \
-    name, sample, base::TimeDelta::FromMilliseconds(10), \
+// Medium timings - up to 3 minutes. Note this starts at 10ms (no good reason,
+// but not worth changing).
+#define UMA_HISTOGRAM_MEDIUM_TIMES(name, sample) UMA_HISTOGRAM_CUSTOM_TIMES(   \
+    name, sample, base::TimeDelta::FromMilliseconds(10),                       \
     base::TimeDelta::FromMinutes(3), 50)
 
-// Use this macro when times can routinely be much longer than 10 seconds.
-#define UMA_HISTOGRAM_LONG_TIMES(name, sample) UMA_HISTOGRAM_CUSTOM_TIMES( \
-    name, sample, base::TimeDelta::FromMilliseconds(1), \
+// Long timings - up to an hour.
+#define UMA_HISTOGRAM_LONG_TIMES(name, sample) UMA_HISTOGRAM_CUSTOM_TIMES(     \
+    name, sample, base::TimeDelta::FromMilliseconds(1),                        \
     base::TimeDelta::FromHours(1), 50)
 
-// Use this macro when times can routinely be much longer than 10 seconds and
-// you want 100 buckets.
+// Long timings with higher granularity - up to an hour with 100 buckets.
 #define UMA_HISTOGRAM_LONG_TIMES_100(name, sample) UMA_HISTOGRAM_CUSTOM_TIMES( \
-    name, sample, base::TimeDelta::FromMilliseconds(1), \
+    name, sample, base::TimeDelta::FromMilliseconds(1),                        \
     base::TimeDelta::FromHours(1), 100)
 
-#define UMA_HISTOGRAM_CUSTOM_TIMES(name, sample, min, max, bucket_count) \
-    STATIC_HISTOGRAM_POINTER_BLOCK(name, AddTime(sample), \
-        base::Histogram::FactoryTimeGet(name, min, max, bucket_count, \
-            base::HistogramBase::kUmaTargetedHistogramFlag))
-
-#define UMA_HISTOGRAM_COUNTS(name, sample) UMA_HISTOGRAM_CUSTOM_COUNTS( \
-    name, sample, 1, 1000000, 50)
-
-#define UMA_HISTOGRAM_COUNTS_100(name, sample) UMA_HISTOGRAM_CUSTOM_COUNTS( \
-    name, sample, 1, 100, 50)
-
-#define UMA_HISTOGRAM_COUNTS_1000(name, sample) UMA_HISTOGRAM_CUSTOM_COUNTS( \
-    name, sample, 1, 1000, 50)
-
-#define UMA_HISTOGRAM_COUNTS_10000(name, sample) UMA_HISTOGRAM_CUSTOM_COUNTS( \
-    name, sample, 1, 10000, 50)
-
-#define UMA_HISTOGRAM_CUSTOM_COUNTS(name, sample, min, max, bucket_count) \
-    INTERNAL_HISTOGRAM_CUSTOM_COUNTS_WITH_FLAG(                           \
-        name, sample, min, max, bucket_count,                             \
-        base::HistogramBase::kUmaTargetedHistogramFlag)
-
-#define UMA_STABILITY_HISTOGRAM_COUNTS_100(name, sample) \
-    UMA_STABILITY_HISTOGRAM_CUSTOM_COUNTS(name, sample, 1, 100, 50)
-
-#define UMA_STABILITY_HISTOGRAM_CUSTOM_COUNTS(name, sample, min, max, \
-                                              bucket_count)           \
-    INTERNAL_HISTOGRAM_CUSTOM_COUNTS_WITH_FLAG(                       \
-        name, sample, min, max, bucket_count,                         \
-        base::HistogramBase::kUmaStabilityHistogramFlag)
-
-#define UMA_HISTOGRAM_MEMORY_KB(name, sample) UMA_HISTOGRAM_CUSTOM_COUNTS( \
-    name, sample, 1000, 500000, 50)
-
-#define UMA_HISTOGRAM_MEMORY_MB(name, sample) UMA_HISTOGRAM_CUSTOM_COUNTS( \
-    name, sample, 1, 1000, 50)
-
-#define UMA_HISTOGRAM_MEMORY_LARGE_MB(name, sample) \
-    UMA_HISTOGRAM_CUSTOM_COUNTS(name, sample, 1, 64000, 100)
-
-#define UMA_HISTOGRAM_PERCENTAGE(name, under_one_hundred) \
-    UMA_HISTOGRAM_ENUMERATION(name, under_one_hundred, 101)
-
-#define UMA_HISTOGRAM_BOOLEAN(name, sample) \
-    STATIC_HISTOGRAM_POINTER_BLOCK(name, AddBoolean(sample), \
-        base::BooleanHistogram::FactoryGet(name, \
-            base::HistogramBase::kUmaTargetedHistogramFlag))
-
-// The samples should always be strictly less than |boundary_value|.  For more
-// details, see the comment for the |LOCAL_HISTOGRAM_ENUMERATION| macro, above.
-#define UMA_HISTOGRAM_ENUMERATION(name, sample, boundary_value) \
-    INTERNAL_HISTOGRAM_ENUMERATION_WITH_FLAG(                   \
-        name, sample, boundary_value,                           \
-        base::HistogramBase::kUmaTargetedHistogramFlag)
-
-// Similar to UMA_HISTOGRAM_ENUMERATION, but used for recording stability
-// histograms.  Use this if recording a histogram that should be part of the
-// initial stability log.
-#define UMA_STABILITY_HISTOGRAM_ENUMERATION(name, sample, boundary_value) \
-    INTERNAL_HISTOGRAM_ENUMERATION_WITH_FLAG(                             \
-        name, sample, boundary_value,                                     \
-        base::HistogramBase::kUmaStabilityHistogramFlag)
-
-#define UMA_HISTOGRAM_CUSTOM_ENUMERATION(name, sample, custom_ranges) \
-    STATIC_HISTOGRAM_POINTER_BLOCK(name, Add(sample), \
-        base::CustomHistogram::FactoryGet(name, custom_ranges, \
+// This can be used when the default ranges are not sufficient. This macro lets
+// the metric developer customize the min and max of the sampled range, as well
+// as the number of buckets recorded. Usage:
+// UMA_HISTOGRAM_CUSTOM_TIMES("Very.Long.Timing.Histogram", duration_in_ms,
+//     base::TimeDelta::FromSeconds(1), base::TimeDelta::FromDays(1), 100);
+#define UMA_HISTOGRAM_CUSTOM_TIMES(name, sample, min, max, bucket_count)       \
+    STATIC_HISTOGRAM_POINTER_BLOCK(name, AddTime(sample),                      \
+        base::Histogram::FactoryTimeGet(name, min, max, bucket_count,          \
             base::HistogramBase::kUmaTargetedHistogramFlag))
 
 // Scoped class which logs its time on this earth as a UMA statistic. This is
 // recommended for when you want a histogram which measures the time it takes
-// for a method to execute. This measures up to 10 seconds.
-#define SCOPED_UMA_HISTOGRAM_TIMER(name) \
+// for a method to execute. This measures up to 10 seconds (this uses

[Mark P, 2016/09/23 19:30:42]

nit: consider
(This uses UMA_HISTOGRAM_TIMES under the hood.)
i.e., don't embed the parenthetical sentence in the other sentence

[rkaplow, 2016/09/26 20:16:10]

just removed the parenthesis.

+// UMA_HISTOGRAM_TIMES under the hood).
+// Sample usage:
+//   void Function() {
+//     SCOPED_UMA_HISTOGRAM_TIMER("Component.FunctionTime");
+//     ...
+//   }
+#define SCOPED_UMA_HISTOGRAM_TIMER(name)                                       \
   SCOPED_UMA_HISTOGRAM_TIMER_EXPANDER(name, false, __COUNTER__)
 
 // Similar scoped histogram timer, but this uses UMA_HISTOGRAM_LONG_TIMES_100,
 // which measures up to an hour, and uses 100 buckets. This is more expensive
 // to store, so only use if this often takes >10 seconds.
-#define SCOPED_UMA_HISTOGRAM_LONG_TIMER(name) \
+#define SCOPED_UMA_HISTOGRAM_LONG_TIMER(name)                                  \
   SCOPED_UMA_HISTOGRAM_TIMER_EXPANDER(name, true, __COUNTER__)
 
-// This nested macro is necessary to expand __COUNTER__ to an actual value.
-#define SCOPED_UMA_HISTOGRAM_TIMER_EXPANDER(name, is_long, key) \
-  SCOPED_UMA_HISTOGRAM_TIMER_UNIQUE(name, is_long, key)
-
-#define SCOPED_UMA_HISTOGRAM_TIMER_UNIQUE(name, is_long, key) \
-  class ScopedHistogramTimer##key { \
-   public: \
-    ScopedHistogramTimer##key() : constructed_(base::TimeTicks::Now()) {} \
-    ~ScopedHistogramTimer##key() { \
-      base::TimeDelta elapsed = base::TimeTicks::Now() - constructed_; \
-      if (is_long) { \
-        UMA_HISTOGRAM_LONG_TIMES_100(name, elapsed); \
-      } else { \
-        UMA_HISTOGRAM_TIMES(name, elapsed); \
-      } \
-    } \
-   private: \
-    base::TimeTicks constructed_; \
-  } scoped_histogram_timer_##key
+
+//------------------------------------------------------------------------------
+// Memory histograms.
+
+// These macros create exponentially sized histograms (lengths of the bucket
+// ranges exponentially increase as the sample range increases). The input
+// sample must be a number measured in ms.

[Mark P, 2016/09/23 19:30:42]

ms -> kilobytes

[rkaplow, 2016/09/26 20:16:10]

Done.

+
+// Sample Usage:
+// UMA_HISTOGRAM_MEMORY_KB("My.Memory.Histogram", memory_in_kb);
+
+// Used to measure common KB-granularity memory stats. Range is up to 500000KB -
+// approximately 500M.
+#define UMA_HISTOGRAM_MEMORY_KB(name, sample)                                  \
+    UMA_HISTOGRAM_CUSTOM_COUNTS(name, sample, 1000, 500000, 50)
+
+// Used to measure common MB-granularity memory stats. Range is up to ~64G.
+#define UMA_HISTOGRAM_MEMORY_LARGE_MB(name, sample)                            \
+    UMA_HISTOGRAM_CUSTOM_COUNTS(name, sample, 1, 64000, 100)
+
+
+//------------------------------------------------------------------------------
+// Enumeration histograms.
+
+// These macros create histograms for enumerated data. Ideally, the data should
+// be of the form of "event occurs, log the result". We recommended not putting
+// related but not directly connected data as enums within the same histogram.
+// You should be defining an associated Enum, and the input sample should be
+// an element of the Enum.
+
+// Sample Usage:
+// UMA_HISTOGRAM_ENUMERATION("My.Enumeration", Enum::VALUE, Enum::EVENT_MAX);
+// Enum values can be appended, but the order should never change. The value

[Mark P, 2016/09/23 19:30:41]

Instead of "Enum values can be appended, but the order should never change."
consider something like
"Enum values can be appended, but existing enums must never be renumbered or
delete and reused."

[rkaplow, 2016/09/26 20:16:10]

Done.

+// in |sample| must be strictly less than |enum_max|.
+
+#define UMA_HISTOGRAM_ENUMERATION(name, sample, enum_max)                      \
+    INTERNAL_HISTOGRAM_ENUMERATION_WITH_FLAG(                                  \
+        name, sample, enum_max,                                                \
+        base::HistogramBase::kUmaTargetedHistogramFlag)
+
+// Histogram for boolean values.
+// Sample Usage:
+// UMA_HISTOGRAM_BOOLEAN("Histogram.Boolean", bool);
+#define UMA_HISTOGRAM_BOOLEAN(name, sample)                                    \
+    STATIC_HISTOGRAM_POINTER_BLOCK(name, AddBoolean(sample),                   \
+        base::BooleanHistogram::FactoryGet(name,                               \
+            base::HistogramBase::kUmaTargetedHistogramFlag))
+
+// Histogram for percentages. This will be 100 buckets of length 1.

[Mark P, 2016/09/23 19:30:41]

length -> size

[rkaplow, 2016/09/26 20:16:10]

Done.

+// Sample Usage:
+// UMA_HISTOGRAM_PERCENTAGE("Histogram.Percent", percent_as_int);
+#define UMA_HISTOGRAM_PERCENTAGE(name, under_one_hundred)                      \
+    UMA_HISTOGRAM_ENUMERATION(name, under_one_hundred, 101)
+
+
+//------------------------------------------------------------------------------
+// Stability-specific histograms.
+
+// Histograms logged in as stability histograms will be included in the initial
+// stability log. |MetricsService::PrepareInitialStabilityLog|.

[Mark P, 2016/09/23 19:30:41]

are you missing the word "See" at the beginning of the second "sentence"?
Perhaps even "See comments by declaration of
MetricsService::PrepareInitialStabilityLog()."

[rkaplow, 2016/09/26 20:16:10]

yup I was.

+// For details on usage, see the documentation on the non-stability equivalents.
+
+#define UMA_STABILITY_HISTOGRAM_COUNTS_100(name, sample)                       \
+    UMA_STABILITY_HISTOGRAM_CUSTOM_COUNTS(name, sample, 1, 100, 50)
+
+#define UMA_STABILITY_HISTOGRAM_CUSTOM_COUNTS(name, sample, min, max,          \
+                                              bucket_count)                    \
+    INTERNAL_HISTOGRAM_CUSTOM_COUNTS_WITH_FLAG(                                \
+        name, sample, min, max, bucket_count,                                  \
+        base::HistogramBase::kUmaStabilityHistogramFlag)
+
+#define UMA_STABILITY_HISTOGRAM_ENUMERATION(name, sample, enum_max)            \
+    INTERNAL_HISTOGRAM_ENUMERATION_WITH_FLAG(                                  \
+        name, sample, enum_max,                                                \
+        base::HistogramBase::kUmaStabilityHistogramFlag)
+
+
+//------------------------------------------------------------------------------
+// Deprecated histograms. Not recommended for current use.
+
+// Legacy name for UMA_HISTOGRAM_COUNTS_1000000. Suggest using explicit naming
+// and not using this macro going forward.
+#define UMA_HISTOGRAM_COUNTS(name, sample) UMA_HISTOGRAM_CUSTOM_COUNTS(        \
+    name, sample, 1, 1000000, 50)
+
+// MB-granularity memory metric. This has a short max (1G).
+#define UMA_HISTOGRAM_MEMORY_MB(name, sample)                                  \
+    UMA_HISTOGRAM_CUSTOM_COUNTS(name, sample, 1, 1000, 50)
+
+// For a enum with customized range. In general, sparse histograms should be

[Mark P, 2016/09/23 19:30:42]

nit: a -> an

[rkaplow, 2016/09/26 20:16:10]

Done.

+// used instead.
+#define UMA_HISTOGRAM_CUSTOM_ENUMERATION(name, sample, custom_ranges)          \
+    STATIC_HISTOGRAM_POINTER_BLOCK(name, Add(sample),                          \
+        base::CustomHistogram::FactoryGet(name, custom_ranges,                 \
+            base::HistogramBase::kUmaTargetedHistogramFlag))
 
 #endif  // BASE_METRICS_HISTOGRAM_MACROS_H_