New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 716600 link

Starred by 1 user
Status: Fixed
Owner:
asvitk...@chromium.org
Closed: May 2017
Cc:
skare@chromium.org
isherman@chromium.org
asvitk...@chromium.org
█ rkaplow@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug



Sign in to add a comment

Logging MaxInt32 to an UMA sparse histogram results in a bucket max of MinInt32.

Project Member Reported by asvitk...@chromium.org, Apr 28 2017 
Logging MaxInt32 to an UMA sparse histogram results in a bucket max of MinInt32.

TEST_P(SparseHistogramTest, ExtremeValues) {
  base::HistogramBase* counter = base::SparseHistogram::FactoryGet(
      "wooga", base::HistogramBase::kUmaTargetedHistogramFlag);
  // counter->Add(-2147483648);
  counter->Add(2147483647);
  std::unique_ptr<HistogramSamples> snapshot = counter->SnapshotSamples();

  for (std::unique_ptr<SampleCountIterator> it = snapshot->Iterator();
       !it->Done(); it->Next()) {
    base::Histogram::Sample min;
    base::Histogram::Sample max;
    base::Histogram::Count count;
    it->Get(&min, &max, &count);
    printf("%d %d %d\n", min, max, count);
  }
}

Prints: 2147483647 -2147483648 1

This is obviously incorrect since we then stick this into an 64 bit proto field and -2147483648 shouldn't be the max of that bucket. So we should fix this somehow - either disallowing logging such a value or handling so that it gets sent (and processed server-side) correctly.

Comment 1 by asvitk...@chromium.org, Apr 28 2017 

One option is to change type of SampleCountIterator::Get's max param to be 64 bit, so that it supports getting values of the appropriate range.

The calling code can then handle it appropriately. In the case of serializing to proto, the field is already 64 bit so we can just set it.

However, we'll need to check if our server-side pipeline will accept it.

Comment 2 by isherman@chromium.org, Apr 28 2017 

What happens if the client code tries to record MaxInt64?  Or are you suggesting only allowing 32-bit values, but using a 64-bit int to represent the max?  (Also, presumably the min needs the same treatment?)

Comment 3 by asvitk...@chromium.org, Apr 28 2017 

Histogram::Sample is an Int32, so it's not possible to record outside of the range of that (without truncation and thus compiler warnings I think), given the API.

I was suggesting only changing the Max field of the iterator API. Other things (except those using that API) don't need to change. Min doesn't need to change because its inclusive - and samples are still 32 bit. Max needs to change because it's exclusive.

(The other option is to disallow logging MaxInt32 - e.g. with a DCHECK or something.)

Comment 4 by isherman@chromium.org, Apr 28 2017 

Okay.  Sounds reasonable, I s'pose =)
Project Member

Comment 5 by bugdroid1@chromium.org, May 3 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3f17b1463420e026459e2d7fde6335cf5e9b1852

commit 3f17b1463420e026459e2d7fde6335cf5e9b1852
Author: asvitkine <asvitkine@chromium.org>
Date: Wed May 03 21:37:37 2017

Fix overflow when logging MaxInt32 to a sparse histogram.

The problem was that a bucket gets reported with an inclusive
min value and exclusive max value, so max must to be +1 of
the sample value for a sparse histogram value.

Because the code wasn't handling this specifically, it caused the
bucket max to be reported as a MinInt32 due to overflow of a 32
bit value. This CL changes the relevant iterator API to use int64_t
for the max, to support MaxInt32 being logged.

BUG= 716600 
TBR=jbauman@chromium.org, groby@chromium.org

Review-Url: https://codereview.chromium.org/2853853002
Cr-Commit-Position: refs/heads/master@{#469136}

[modify] https://crrev.com/3f17b1463420e026459e2d7fde6335cf5e9b1852/base/metrics/histogram_samples.cc
[modify] https://crrev.com/3f17b1463420e026459e2d7fde6335cf5e9b1852/base/metrics/histogram_samples.h
[modify] https://crrev.com/3f17b1463420e026459e2d7fde6335cf5e9b1852/base/metrics/persistent_sample_map.cc
[modify] https://crrev.com/3f17b1463420e026459e2d7fde6335cf5e9b1852/base/metrics/persistent_sample_map_unittest.cc
[modify] https://crrev.com/3f17b1463420e026459e2d7fde6335cf5e9b1852/base/metrics/sample_map.cc
[modify] https://crrev.com/3f17b1463420e026459e2d7fde6335cf5e9b1852/base/metrics/sample_map_unittest.cc
[modify] https://crrev.com/3f17b1463420e026459e2d7fde6335cf5e9b1852/base/metrics/sample_vector.cc
[modify] https://crrev.com/3f17b1463420e026459e2d7fde6335cf5e9b1852/base/metrics/sample_vector.h
[modify] https://crrev.com/3f17b1463420e026459e2d7fde6335cf5e9b1852/base/metrics/sample_vector_unittest.cc
[modify] https://crrev.com/3f17b1463420e026459e2d7fde6335cf5e9b1852/base/metrics/sparse_histogram.cc
[modify] https://crrev.com/3f17b1463420e026459e2d7fde6335cf5e9b1852/base/metrics/sparse_histogram_unittest.cc
[modify] https://crrev.com/3f17b1463420e026459e2d7fde6335cf5e9b1852/chrome/browser/ui/webui/task_scheduler_internals/task_scheduler_internals_ui.cc
[modify] https://crrev.com/3f17b1463420e026459e2d7fde6335cf5e9b1852/components/metrics/histogram_encoder.cc
[modify] https://crrev.com/3f17b1463420e026459e2d7fde6335cf5e9b1852/components/tracing/child/child_trace_message_filter.cc
[modify] https://crrev.com/3f17b1463420e026459e2d7fde6335cf5e9b1852/components/translate/core/common/translate_metrics_unittest.cc

Comment 6 by asvitk...@chromium.org, May 3 2017

Status: Fixed (was: Assigned)
►

Sign in to add a comment