Monitor Add for negative results.

base/metrics/persistent_sample_map.cc

Index: base/metrics/persistent_sample_map.cc
diff --git a/base/metrics/persistent_sample_map.cc b/base/metrics/persistent_sample_map.cc
index 51cc0c709d8b0d9087c75d64c512b06e434f6ba7..f95fc5463a18ab82dd988e18564d139797aa23f7 100644
--- a/base/metrics/persistent_sample_map.cc
+++ b/base/metrics/persistent_sample_map.cc
@@ -20,6 +20,8 @@ namespace {
 enum NegativeSampleReason {
   PERSISTENT_SPARSE_HAVE_LOGGED_BUT_NOT_SAMPLE,
   PERSISTENT_SPARSE_SAMPLE_LESS_THAN_LOGGED,
+  PERSISTENT_SPARSE_ADDED_NEGATIVE_COUNT,
+  PERSISTENT_SPARSE_ADD_WENT_NEGATIVE,
   MAX_NEGATIVE_SAMPLE_REASONS
 };
 
@@ -197,13 +199,23 @@ bool PersistentSampleMap::AddSubtractImpl(SampleCountIterator* iter,
     *GetOrCreateSampleCountStorage(min) +=
         (op == HistogramSamples::ADD) ? count : -count;
 #else
+    NegativeSampleReason reason = MAX_NEGATIVE_SAMPLE_REASONS;
     if (op == HistogramSamples::ADD) {
-      *GetOrCreateSampleCountStorage(min) += count;
+      // Add should generally be adding only positive values.
+      Count* bucket = GetOrCreateSampleCountStorage(min);

[Alexei Svitkine (slow), 2017/04/26 19:37:58]

Nit: call this bucket_value as |bucket| is often used to refer to bucket index.

[bcwhite, 2017/04/26 20:47:14]

Went with |local_count_ptr| to match with |count| value retrieved from iterator.

+      if (count < 0) {
+        reason = PERSISTENT_SPARSE_ADDED_NEGATIVE_COUNT;
+        if (*bucket < -count) {
+          reason = PERSISTENT_SPARSE_ADD_WENT_NEGATIVE;
+          *bucket = 0;
+        }
+      } else {
+        *bucket += count;

[Alexei Svitkine (slow), 2017/04/26 19:37:58]

Couldn't this result in a negative too? i.e. due to overflow.

Let's check for that too.

[bcwhite, 2017/04/26 20:47:14]

Done.

+      }
     } else {
       // Subtract is used only for determining deltas when reporting which
       // means that it's in the "logged" iterator. It should have an active
       // sample record and thus there is no need to try to create one.
-      NegativeSampleReason reason = MAX_NEGATIVE_SAMPLE_REASONS;
       Count* bucket = GetSampleCountStorage(min);
       if (bucket == nullptr) {
         reason = PERSISTENT_SPARSE_HAVE_LOGGED_BUT_NOT_SAMPLE;
@@ -215,10 +227,11 @@ bool PersistentSampleMap::AddSubtractImpl(SampleCountIterator* iter,
           *bucket -= count;
         }
       }
-      if (reason != MAX_NEGATIVE_SAMPLE_REASONS) {
-        UMA_HISTOGRAM_ENUMERATION("UMA.NegativeSamples.Reason", reason,
-                                  MAX_NEGATIVE_SAMPLE_REASONS);
-      }
+    }
+    if (reason != MAX_NEGATIVE_SAMPLE_REASONS) {
+      NOTREACHED();
+      UMA_HISTOGRAM_ENUMERATION("UMA.NegativeSamples.Reason", reason,
+                                MAX_NEGATIVE_SAMPLE_REASONS);
     }
 #endif
   }