SampleSet -> HistogramSamples (will be reused by SparseHistogram)

chrome/common/metrics/metrics_log_base.cc

 // Copyright (c) 2012 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.
 
 #include "chrome/common/metrics/metrics_log_base.h"
 
 #include "base/base64.h"
 #include "base/basictypes.h"
 #include "base/md5.h"
+#include "base/metrics/histogram_base.h"
+#include "base/metrics/histogram_samples.h"
 #include "base/perftimer.h"
 #include "base/string_number_conversions.h"
 #include "base/sys_byteorder.h"
 #include "base/sys_info.h"
 #include "base/third_party/nspr/prtime.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/common/chrome_version_info.h"
 #include "chrome/common/logging_chrome.h"
 #include "chrome/common/metrics/proto/histogram_event.pb.h"
 #include "chrome/common/metrics/proto/system_profile.pb.h"
 #include "chrome/common/metrics/proto/user_action_event.pb.h"
 #include "libxml/xmlwriter.h"
 
 #define OPEN_ELEMENT_FOR_SCOPE(name) ScopedElement scoped_element(this, name)
 
 using base::Histogram;
+using base::HistogramBase;
+using base::HistogramSamples;
+using base::SampleCountIterator;
 using base::Time;
 using base::TimeDelta;
 using metrics::HistogramEventProto;
 using metrics::SystemProfileProto;
 using metrics::UserActionEventProto;
 
 namespace {
 
 // libxml take xmlChar*, which is unsigned char*
 inline const unsigned char* UnsignedChar(const char* input) {
@@ skipping 406 matching lines @@
   int result = xmlTextWriterEndElement(xml_wrapper_->writer());
   DCHECK_GE(result, 0);
 }
 
 // TODO(JAR): A The following should really be part of the histogram class.
 // Internal state is being needlessly exposed, and it would be hard to reuse
 // this code. If we moved this into the Histogram class, then we could use
 // the same infrastructure for logging StatsCounters, RatesCounters, etc.
 void MetricsLogBase::RecordHistogramDelta(
     const Histogram& histogram,
-    const Histogram::SampleSet& snapshot) {
+    const HistogramSamples& snapshot) {
   DCHECK(!locked_);
   DCHECK_NE(0, snapshot.TotalCount());
-  DCHECK_EQ(histogram.bucket_count(), snapshot.size());

[Ilya Sherman, 2012/08/23 07:50:54]

Why is this DCHECK no longer appropriate?

[kaiwang, 2012/08/24 04:17:58]

There's no "size" anymore. It's just some duplicated information and does not
work with sparse histogram.

 
   // We will ignore the MAX_INT/infinite value in the last element of range[].
 
   OPEN_ELEMENT_FOR_SCOPE("histogram");
 
   std::string base64_name_hash;
   uint64 numeric_name_hash;
   CreateHashes(histogram.histogram_name(),
                &base64_name_hash,
                &numeric_name_hash);
 
   // Write the XML version.
   WriteAttribute("name", base64_name_hash);
 
   WriteInt64Attribute("sum", snapshot.sum());
   // TODO(jar): Remove sumsquares when protobuffer accepts this as optional.
   WriteInt64Attribute("sumsquares", 0);
 
-  for (size_t i = 0; i < histogram.bucket_count(); i++) {
-    if (snapshot.counts(i)) {
-      OPEN_ELEMENT_FOR_SCOPE("histogrambucket");
-      WriteIntAttribute("min", histogram.ranges(i));
-      WriteIntAttribute("max", histogram.ranges(i + 1));
-      WriteIntAttribute("count", snapshot.counts(i));
-    }
+  HistogramBase::Sample min;
+  HistogramBase::Sample max;
+  HistogramBase::Count count;

[Ilya Sherman, 2012/08/23 07:50:54]

nit: These should be probably be scoped within the for loop.

[kaiwang, 2012/08/24 04:17:58]

Done.

+  for (scoped_ptr<SampleCountIterator> it = snapshot.Iterator();
+       !it->Done();
+       it->Next()) {
+    it->Get(&min, &max, &count);
+    WriteIntAttribute("min", min);
+    WriteIntAttribute("max", max);
+    WriteIntAttribute("count", count);
   }
 
   // Write the protobuf version.
   HistogramEventProto* histogram_proto = uma_proto_.add_histogram_event();
   histogram_proto->set_name_hash(numeric_name_hash);
   histogram_proto->set_sum(snapshot.sum());
 
-  for (size_t i = 0; i < histogram.bucket_count(); ++i) {
-    if (snapshot.counts(i)) {
-      HistogramEventProto::Bucket* bucket = histogram_proto->add_bucket();
-      bucket->set_min(histogram.ranges(i));
-      bucket->set_max(histogram.ranges(i + 1));
-      bucket->set_bucket_index(i);
-      bucket->set_count(snapshot.counts(i));
-    }
+  for (scoped_ptr<SampleCountIterator> it = snapshot.Iterator();
+       !it->Done();
+       it->Next()) {
+    it->Get(&min, &max, &count);
+    HistogramEventProto::Bucket* bucket = histogram_proto->add_bucket();
+    bucket->set_min(min);
+    bucket->set_max(max);
+
+    // TODO(kaiwang): do we need bucket_index field?

[Ilya Sherman, 2012/08/23 07:50:54]

Jim, are we realistically ever going to use this field, or is it ok to drop?

+
+    bucket->set_count(count);
   }
 }