Bulk Reports: Add support for histogram range triggers.

Previously we could only specify a lower bound for histogram triggers. This CL introduces an upper bound for triggering, allowing for more fine grained control over when traces are captured.

BUG=525655

Review URL: https://codereview.chromium.org/1349313005

Cr-Commit-Position: refs/heads/master@{#350002}

6 files changed, 157 insertions, 29 deletions

diff --git a/components/tracing/child_trace_message_filter.cc b/components/tracing/child_trace_message_filter.cc
index c30233f..17a3358 100644
--- a/components/tracing/child_trace_message_filter.cc
+++ b/components/tracing/child_trace_message_filter.cc
@@ -237,9 +237,11 @@ void ChildTraceMessageFilter::OnGlobalMemoryDumpResponse(uint64 dump_guid,
 
 void ChildTraceMessageFilter::OnHistogramChanged(
     const std::string& histogram_name,
-    base::Histogram::Sample reference_value,
+    base::Histogram::Sample reference_lower_value,
+    base::Histogram::Sample reference_upper_value,
     base::Histogram::Sample actual_value) {
-  if (actual_value < reference_value)
+  if (actual_value < reference_lower_value ||
+      actual_value > reference_upper_value)
     return;
 
   ipc_task_runner_->PostTask(
@@ -264,11 +266,13 @@ void ChildTraceMessageFilter::SendTriggerMessage(
 
 void ChildTraceMessageFilter::OnSetUMACallback(
     const std::string& histogram_name,
-    int histogram_value) {
+    int histogram_lower_value,
+    int histogram_upper_value) {
   histogram_last_changed_ = base::Time();
   base::StatisticsRecorder::SetCallback(
-      histogram_name, base::Bind(&ChildTraceMessageFilter::OnHistogramChanged,
-                                 this, histogram_name, histogram_value));
+      histogram_name,
+      base::Bind(&ChildTraceMessageFilter::OnHistogramChanged, this,
+                 histogram_name, histogram_lower_value, histogram_upper_value));
 }
 
 void ChildTraceMessageFilter::OnClearUMACallback(
diff --git a/components/tracing/child_trace_message_filter.h b/components/tracing/child_trace_message_filter.h
index 06c276d..124f562 100644
--- a/components/tracing/child_trace_message_filter.h
+++ b/components/tracing/child_trace_message_filter.h
@@ -60,10 +60,13 @@ class TRACING_EXPORT ChildTraceMessageFilter : public IPC::MessageFilter {
   void OnProcessMemoryDumpRequest(
       const base::trace_event::MemoryDumpRequestArgs& args);
   void OnGlobalMemoryDumpResponse(uint64 dump_guid, bool success);
-  void OnSetUMACallback(const std::string& histogram_name, int histogram_value);
+  void OnSetUMACallback(const std::string& histogram_name,
+                        int histogram_lower_value,
+                        int histogram_upper_value);
   void OnClearUMACallback(const std::string& histogram_name);
   void OnHistogramChanged(const std::string& histogram_name,
-                          base::Histogram::Sample reference_value,
+                          base::Histogram::Sample reference_lower_value,
+                          base::Histogram::Sample reference_upper_value,
                           base::Histogram::Sample actual_value);
   void SendTriggerMessage(const std::string& histogram_name);
 
diff --git a/components/tracing/tracing_messages.h b/components/tracing/tracing_messages.h
index 11196e3..cd5a202 100644
--- a/components/tracing/tracing_messages.h
+++ b/components/tracing/tracing_messages.h
@@ -87,9 +87,10 @@ IPC_MESSAGE_CONTROL2(TracingMsg_GlobalMemoryDumpResponse,
                      uint64 /* dump_guid */,
                      bool /* success */)
 
-IPC_MESSAGE_CONTROL2(TracingMsg_SetUMACallback,
+IPC_MESSAGE_CONTROL3(TracingMsg_SetUMACallback,
                      std::string /* histogram_name */,
-                     base::HistogramBase::Sample /* histogram_value */)
+                     base::HistogramBase::Sample /* histogram_lower_value */,
+                     base::HistogramBase::Sample /* histogram_uppwer_value */)
 
 IPC_MESSAGE_CONTROL1(TracingMsg_ClearUMACallback,
                      std::string /* histogram_name */)
diff --git a/content/browser/tracing/background_tracing_config_unittest.cc b/content/browser/tracing/background_tracing_config_unittest.cc
index 4f92501..4aa462c 100644
--- a/content/browser/tracing/background_tracing_config_unittest.cc
+++ b/content/browser/tracing/background_tracing_config_unittest.cc
@@ -191,8 +191,24 @@ TEST_F(BackgroundTracingConfigTest, PreemptiveConfigFromValidString) {
   EXPECT_EQ(config->category_preset(), BackgroundTracingConfigImpl::BENCHMARK);
   EXPECT_EQ(config->rules().size(), 1u);
   EXPECT_EQ(RuleToString(config->rules()[0]),
-            "{\"histogram_name\":\"foo\",\"histogram_value\":1,"
-            "\"rule\":\"MONITOR_AND_DUMP_WHEN_SPECIFIC_HISTOGRAM_AND_VALUE\"}");
+            "{\"histogram_lower_value\":1,\"histogram_name\":\"foo\","
+            "\"histogram_upper_value\":2147483647,\"rule\":\"MONITOR_AND_DUMP_"
+            "WHEN_SPECIFIC_HISTOGRAM_AND_VALUE\"}");
+
+  config = ReadFromJSONString(
+      "{\"mode\":\"PREEMPTIVE_TRACING_MODE\", \"category\": "
+      "\"BENCHMARK\",\"configs\": [{\"rule\": "
+      "\"MONITOR_AND_DUMP_WHEN_SPECIFIC_HISTOGRAM_AND_VALUE\", "
+      "\"histogram_name\":\"foo\", \"histogram_lower_value\": 1, "
+      "\"histogram_upper_value\": 2}]}");
+  EXPECT_TRUE(config);
+  EXPECT_EQ(config->tracing_mode(), BackgroundTracingConfig::PREEMPTIVE);
+  EXPECT_EQ(config->category_preset(), BackgroundTracingConfigImpl::BENCHMARK);
+  EXPECT_EQ(config->rules().size(), 1u);
+  EXPECT_EQ(RuleToString(config->rules()[0]),
+            "{\"histogram_lower_value\":1,\"histogram_name\":\"foo\","
+            "\"histogram_upper_value\":2,\"rule\":\"MONITOR_AND_DUMP_WHEN_"
+            "SPECIFIC_HISTOGRAM_AND_VALUE\"}");
 
   config = ReadFromJSONString(
       "{\"mode\":\"PREEMPTIVE_TRACING_MODE\", \"category\": "
@@ -328,14 +344,15 @@ TEST_F(BackgroundTracingConfigTest, ValidPreemptiveConfigToString) {
     second_dict->SetString(
         "rule", "MONITOR_AND_DUMP_WHEN_SPECIFIC_HISTOGRAM_AND_VALUE");
     second_dict->SetString("histogram_name", "foo");
-    second_dict->SetInteger("histogram_value", 1);
+    second_dict->SetInteger("histogram_lower_value", 1);
+    second_dict->SetInteger("histogram_upper_value", 2);
     config->AddPreemptiveRule(second_dict.get());
 
     EXPECT_EQ(ConfigToString(config.get()),
-              "{\"category\":\"BENCHMARK\",\"configs\":[{\"histogram_name\":"
-              "\"foo\",\"histogram_value\":1,\"rule\":\"MONITOR_AND_DUMP_WHEN_"
-              "SPECIFIC_HISTOGRAM_AND_VALUE\"}],\"mode\":\"PREEMPTIVE_TRACING_"
-              "MODE\"}");
+              "{\"category\":\"BENCHMARK\",\"configs\":[{\"histogram_lower_"
+              "value\":1,\"histogram_name\":\"foo\",\"histogram_upper_value\":"
+              "2,\"rule\":\"MONITOR_AND_DUMP_WHEN_SPECIFIC_HISTOGRAM_AND_"
+              "VALUE\"}],\"mode\":\"PREEMPTIVE_TRACING_MODE\"}");
   }
 }
 
@@ -354,6 +371,37 @@ TEST_F(BackgroundTracingConfigTest, InvalidPreemptiveConfigToString) {
               "{\"category\":\"BENCHMARK\",\"configs\":[],\"mode\":"
               "\"PREEMPTIVE_TRACING_MODE\"}");
   }
+
+  {
+    config.reset(
+        new BackgroundTracingConfigImpl(BackgroundTracingConfig::PREEMPTIVE));
+
+    scoped_ptr<base::DictionaryValue> second_dict(new base::DictionaryValue());
+    second_dict->SetString(
+        "rule", "MONITOR_AND_DUMP_WHEN_SPECIFIC_HISTOGRAM_AND_VALUE");
+    second_dict->SetString("histogram_name", "foo");
+    second_dict->SetInteger("histogram_lower_value", 1);
+
+    EXPECT_EQ(ConfigToString(config.get()),
+              "{\"category\":\"BENCHMARK\",\"configs\":[],\"mode\":"
+              "\"PREEMPTIVE_TRACING_MODE\"}");
+  }
+
+  {
+    config.reset(
+        new BackgroundTracingConfigImpl(BackgroundTracingConfig::PREEMPTIVE));
+
+    scoped_ptr<base::DictionaryValue> second_dict(new base::DictionaryValue());
+    second_dict->SetString(
+        "rule", "MONITOR_AND_DUMP_WHEN_SPECIFIC_HISTOGRAM_AND_VALUE");
+    second_dict->SetString("histogram_name", "foo");
+    second_dict->SetInteger("histogram_lower_value", 1);
+    second_dict->SetInteger("histogram_upper_value", 1);
+
+    EXPECT_EQ(ConfigToString(config.get()),
+              "{\"category\":\"BENCHMARK\",\"configs\":[],\"mode\":"
+              "\"PREEMPTIVE_TRACING_MODE\"}");
+  }
 }
 
 TEST_F(BackgroundTracingConfigTest, ValidReactiveConfigToString) {
diff --git a/content/browser/tracing/background_tracing_manager_browsertest.cc b/content/browser/tracing/background_tracing_manager_browsertest.cc
index b9d9880..c8d9b81 100644
--- a/content/browser/tracing/background_tracing_manager_browsertest.cc
+++ b/content/browser/tracing/background_tracing_manager_browsertest.cc
@@ -559,6 +559,52 @@ IN_PROC_BROWSER_TEST_F(BackgroundTracingManagerBrowserTest,
   }
 }
 
+// This tests that histogram values > upper reference value don't trigger.
+IN_PROC_BROWSER_TEST_F(BackgroundTracingManagerBrowserTest,
+                       ReceiveTraceFailsOnHigherHistogramSample) {
+  {
+    SetupBackgroundTracingManager();
+
+    base::RunLoop run_loop;
+
+    BackgroundTracingManagerUploadConfigWrapper upload_config_wrapper(
+        run_loop.QuitClosure());
+
+    base::DictionaryValue dict;
+    dict.SetString("mode", "PREEMPTIVE_TRACING_MODE");
+    dict.SetString("category", "BENCHMARK");
+
+    scoped_ptr<base::ListValue> rules_list(new base::ListValue());
+    {
+      scoped_ptr<base::DictionaryValue> rules_dict(new base::DictionaryValue());
+      rules_dict->SetString(
+          "rule", "MONITOR_AND_DUMP_WHEN_SPECIFIC_HISTOGRAM_AND_VALUE");
+      rules_dict->SetString("histogram_name", "fake");
+      rules_dict->SetInteger("histogram_lower_value", 1);
+      rules_dict->SetInteger("histogram_upper_value", 3);
+      rules_list->Append(rules_dict.Pass());
+    }
+
+    dict.Set("configs", rules_list.Pass());
+
+    scoped_ptr<BackgroundTracingConfig> config(
+        BackgroundTracingConfigImpl::FromDict(&dict));
+    EXPECT_TRUE(config);
+
+    BackgroundTracingManager::GetInstance()->SetActiveScenario(
+        config.Pass(), upload_config_wrapper.get_receive_callback(),
+        BackgroundTracingManager::NO_DATA_FILTERING);
+
+    // This should fail to trigger a trace since the sample value > the
+    // the upper reference value above.
+    LOCAL_HISTOGRAM_COUNTS("fake", 0);
+
+    run_loop.RunUntilIdle();
+
+    EXPECT_TRUE(upload_config_wrapper.get_receive_count() == 0);
+  }
+}
+
 // This tests that invalid preemptive mode configs will fail.
 IN_PROC_BROWSER_TEST_F(BackgroundTracingManagerBrowserTest,
                        SetActiveScenarioFailsWithInvalidPreemptiveConfig) {
diff --git a/content/browser/tracing/background_tracing_rule.cc b/content/browser/tracing/background_tracing_rule.cc
index b32e0b1..473e3d3 100644
--- a/content/browser/tracing/background_tracing_rule.cc
+++ b/content/browser/tracing/background_tracing_rule.cc
@@ -23,7 +23,9 @@ const char kConfigCategoryKey[] = "category";
 const char kConfigRuleTriggerNameKey[] = "trigger_name";
 
 const char kConfigRuleHistogramNameKey[] = "histogram_name";
-const char kConfigRuleHistogramValueKey[] = "histogram_value";
+const char kConfigRuleHistogramValueOldKey[] = "histogram_value";
+const char kConfigRuleHistogramValue1Key[] = "histogram_lower_value";
+const char kConfigRuleHistogramValue2Key[] = "histogram_upper_value";
 
 const char kConfigRuleRandomIntervalTimeoutMin[] = "timeout_min";
 const char kConfigRuleRandomIntervalTimeoutMax[] = "timeout_max";
@@ -89,8 +91,12 @@ class NamedTriggerRule : public BackgroundTracingRule {
 class HistogramRule : public BackgroundTracingRule,
                       public TracingControllerImpl::TraceMessageFilterObserver {
  public:
-  HistogramRule(const std::string& histogram_name, int histogram_value)
-      : histogram_name_(histogram_name), histogram_value_(histogram_value) {}
+  HistogramRule(const std::string& histogram_name,
+                int histogram_lower_value,
+                int histogram_upper_value)
+      : histogram_name_(histogram_name),
+        histogram_lower_value_(histogram_lower_value),
+        histogram_upper_value_(histogram_upper_value) {}
 
   ~HistogramRule() override {
     base::StatisticsRecorder::ClearCallback(histogram_name_);
@@ -103,7 +109,8 @@ class HistogramRule : public BackgroundTracingRule,
     base::StatisticsRecorder::SetCallback(
         histogram_name_,
         base::Bind(&HistogramRule::OnHistogramChangedCallback,
-                   base::Unretained(this), histogram_name_, histogram_value_));
+                   base::Unretained(this), histogram_name_,
+                   histogram_lower_value_, histogram_upper_value_));
 
     TracingControllerImpl::GetInstance()->AddTraceMessageFilterObserver(this);
   }
@@ -112,7 +119,8 @@ class HistogramRule : public BackgroundTracingRule,
     DCHECK(dict);
     dict->SetString(kConfigRuleKey, kPreemptiveConfigRuleMonitorHistogram);
     dict->SetString(kConfigRuleHistogramNameKey, histogram_name_.c_str());
-    dict->SetInteger(kConfigRuleHistogramValueKey, histogram_value_);
+    dict->SetInteger(kConfigRuleHistogramValue1Key, histogram_lower_value_);
+    dict->SetInteger(kConfigRuleHistogramValue2Key, histogram_upper_value_);
   }
 
   void OnHistogramTrigger(const std::string& histogram_name) const override {
@@ -128,8 +136,8 @@ class HistogramRule : public BackgroundTracingRule,
 
   // TracingControllerImpl::TraceMessageFilterObserver implementation
   void OnTraceMessageFilterAdded(TraceMessageFilter* filter) override {
-    filter->Send(
-        new TracingMsg_SetUMACallback(histogram_name_, histogram_value_));
+    filter->Send(new TracingMsg_SetUMACallback(
+        histogram_name_, histogram_lower_value_, histogram_upper_value_));
   }
 
   void OnTraceMessageFilterRemoved(TraceMessageFilter* filter) override {
@@ -137,9 +145,11 @@ class HistogramRule : public BackgroundTracingRule,
   }
 
   void OnHistogramChangedCallback(const std::string& histogram_name,
-                                  base::Histogram::Sample reference_value,
+                                  base::Histogram::Sample reference_lower_value,
+                                  base::Histogram::Sample reference_upper_value,
                                   base::Histogram::Sample actual_value) {
-    if (reference_value > actual_value)
+    if (reference_lower_value > actual_value ||
+        reference_upper_value < actual_value)
       return;
 
     OnHistogramTrigger(histogram_name);
@@ -147,7 +157,8 @@ class HistogramRule : public BackgroundTracingRule,
 
  private:
   std::string histogram_name_;
-  int histogram_value_;
+  int histogram_lower_value_;
+  int histogram_upper_value_;
 };
 
 class ReactiveTraceForNSOrTriggerOrFullRule : public BackgroundTracingRule {
@@ -298,12 +309,27 @@ scoped_ptr<BackgroundTracingRule> BackgroundTracingRule::PreemptiveRuleFromDict(
     if (!dict->GetString(kConfigRuleHistogramNameKey, &histogram_name))
       return nullptr;
 
+    // Check for the old naming.
     int histogram_value;
-    if (!dict->GetInteger(kConfigRuleHistogramValueKey, &histogram_value))
+    if (dict->GetInteger(kConfigRuleHistogramValueOldKey, &histogram_value))
+      return scoped_ptr<BackgroundTracingRule>(new HistogramRule(
+          histogram_name, histogram_value, std::numeric_limits<int>::max()));
+
+    int histogram_lower_value;
+    if (!dict->GetInteger(kConfigRuleHistogramValue1Key,
+                          &histogram_lower_value))
       return nullptr;
 
-    return scoped_ptr<BackgroundTracingRule>(
-        new HistogramRule(histogram_name, histogram_value));
+    int histogram_upper_value;
+    if (!dict->GetInteger(kConfigRuleHistogramValue2Key,
+                          &histogram_upper_value))
+      histogram_upper_value = std::numeric_limits<int>::max();
+
+    if (histogram_lower_value >= histogram_upper_value)
+      return nullptr;
+
+    return scoped_ptr<BackgroundTracingRule>(new HistogramRule(
+        histogram_name, histogram_lower_value, histogram_upper_value));
   }
 
   return nullptr;