From 47234644caf0f2a1aac3a1db8c548b1a25b1cfe2 Mon Sep 17 00:00:00 2001
From: Chenjie Yu <cjyu@google.com>
Date: Mon, 14 May 2018 10:14:16 -0700
Subject: [PATCH] Configurable data error action in value metric

Right now in value metric, if a later pull produces a smaller number
than the previous one, we use absolute value of the current value.
This is not correct for some atoms as listed in the CL, which should
just take 0.
For some other atoms, this is unexpected error and should just dump
stale data.

Test: manual test
Bug: 79265262
Change-Id: I59fbfd96cbb57be22cd8d21cb57a7c60ca6856ee
---
 cmds/statsd/src/metrics/ValueMetricProducer.cpp | 16 +++++++++++-----
 cmds/statsd/src/metrics/ValueMetricProducer.h   |  2 ++
 cmds/statsd/src/statsd_config.proto             |  2 ++
 3 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/cmds/statsd/src/metrics/ValueMetricProducer.cpp b/cmds/statsd/src/metrics/ValueMetricProducer.cpp
index df8763cda244c..136fd14a8ba3e 100644
--- a/cmds/statsd/src/metrics/ValueMetricProducer.cpp
+++ b/cmds/statsd/src/metrics/ValueMetricProducer.cpp
@@ -87,7 +87,8 @@ ValueMetricProducer::ValueMetricProducer(const ConfigKey& key, const ValueMetric
       mDimensionHardLimit(StatsdStats::kAtomDimensionKeySizeLimitMap.find(pullTagId) !=
                                           StatsdStats::kAtomDimensionKeySizeLimitMap.end()
                                   ? StatsdStats::kAtomDimensionKeySizeLimitMap.at(pullTagId).second
-                                  : StatsdStats::kDimensionKeySizeHardLimit) {
+                                  : StatsdStats::kDimensionKeySizeHardLimit),
+      mUseAbsoluteValueOnReset(metric.use_absolute_value_on_reset()) {
     // TODO: valuemetric for pushed events may need unlimited bucket length
     int64_t bucketSizeMills = 0;
     if (metric.has_bucket()) {
@@ -393,15 +394,20 @@ void ValueMetricProducer::onMatchedLogEventInternalLocked(
             }
         } else {
             // Generally we expect value to be monotonically increasing.
-            // If not, there was a reset event. We take the absolute value as
-            // diff in this case.
+            // If not, take absolute value or drop it, based on config.
             if (interval.startUpdated) {
                 if (value >= interval.start) {
                     interval.sum += (value - interval.start);
+                    interval.hasValue = true;
                 } else {
-                    interval.sum += value;
+                    if (mUseAbsoluteValueOnReset) {
+                        interval.sum += value;
+                        interval.hasValue = true;
+                    } else {
+                        VLOG("Dropping data for atom %d, prev: %lld, now: %lld", mPullTagId,
+                             (long long)interval.start, (long long)value);
+                    }
                 }
-                interval.hasValue = true;
                 interval.startUpdated = false;
             } else {
                 VLOG("No start for matching end %lld", (long long)value);
diff --git a/cmds/statsd/src/metrics/ValueMetricProducer.h b/cmds/statsd/src/metrics/ValueMetricProducer.h
index 113be4b39944c..0136ec1e8b973 100644
--- a/cmds/statsd/src/metrics/ValueMetricProducer.h
+++ b/cmds/statsd/src/metrics/ValueMetricProducer.h
@@ -164,6 +164,8 @@ private:
 
     const size_t mDimensionHardLimit;
 
+    const bool mUseAbsoluteValueOnReset;
+
     FRIEND_TEST(ValueMetricProducerTest, TestNonDimensionalEvents);
     FRIEND_TEST(ValueMetricProducerTest, TestEventsWithNonSlicedCondition);
     FRIEND_TEST(ValueMetricProducerTest, TestPushedEventsWithUpgrade);
diff --git a/cmds/statsd/src/statsd_config.proto b/cmds/statsd/src/statsd_config.proto
index 849d5f683d679..eb77299846e6d 100644
--- a/cmds/statsd/src/statsd_config.proto
+++ b/cmds/statsd/src/statsd_config.proto
@@ -265,6 +265,8 @@ message ValueMetric {
   optional AggregationType aggregation_type = 8 [default = SUM];
 
   optional int64 min_bucket_size_nanos = 10;
+
+  optional bool use_absolute_value_on_reset = 11 [default = false];
 }
 
 message Alert {