Fix overflow in user traffic content type histogram

Fix overflow in user traffic content type histogram

Overflows are likely for DataUse.ContentType.UserTraffic histogram. Fix
the overflow by logging the histogram in increments of kilobytes.

BUG=679046
Review-Url: https://codereview.chromium.org/2851923002
Cr-Commit-Position: refs/heads/master@{#469419}
Committed: https://chromium.googlesource.com/chromium/src/+/6c9c9ed06b946f1e9666212df891ffe8fc873077

[Raj, 2017-04-28 21:57:04]

rajendrant@chromium.org changed reviewers:
+ ryansturm@chromium.org

[Raj, 2017-04-28 21:57:05]

ptal

[RyanSturm, 2017-05-01 18:49:44]

https://codereview.chromium.org/2851923002/diff/1/components/data_use_measure...
File components/data_use_measurement/core/data_use_measurement.cc (right):

https://codereview.chromium.org/2851923002/diff/1/components/data_use_measure...
components/data_use_measurement/core/data_use_measurement.cc:448:
user_traffic_content_type_bytes_[content_type] += bytes;
You might want to capture the foreground/background state here. If you are
storing 500B of background data and a new request starts and finishes entirely
in foreground, will the 500B get attributed to foreground instead of
ambiguous/background?

[Raj, 2017-05-01 20:57:58]

https://codereview.chromium.org/2851923002/diff/1/components/data_use_measure...
File components/data_use_measurement/core/data_use_measurement.cc (right):

https://codereview.chromium.org/2851923002/diff/1/components/data_use_measure...
components/data_use_measurement/core/data_use_measurement.cc:448:
user_traffic_content_type_bytes_[content_type] += bytes;
On 2017/05/01 18:49:44, Ryan Sturm wrote:
> You might want to capture the foreground/background state here. If you are
> storing 500B of background data and a new request starts and finishes entirely
> in foreground, will the 500B get attributed to foreground instead of
> ambiguous/background?

This CL only changes the UMA DataUse.ContentType.UserTrafficKB which does not
have BG/FG distinction in name.
The content_type already captures video/audio in BG/FG explicitly. So the bytes
go in the appropriate user_traffic_content_type_bytes[] bucket.

[RyanSturm, 2017-05-01 21:05:24]

lgtm

https://codereview.chromium.org/2851923002/diff/1/components/data_use_measure...
File components/data_use_measurement/core/data_use_measurement.cc (right):

https://codereview.chromium.org/2851923002/diff/1/components/data_use_measure...
components/data_use_measurement/core/data_use_measurement.cc:448:
user_traffic_content_type_bytes_[content_type] += bytes;
On 2017/05/01 20:57:58, Raj wrote:
> On 2017/05/01 18:49:44, Ryan Sturm wrote:
> > You might want to capture the foreground/background state here. If you are
> > storing 500B of background data and a new request starts and finishes
entirely
> > in foreground, will the 500B get attributed to foreground instead of
> > ambiguous/background?
> 
> This CL only changes the UMA DataUse.ContentType.UserTrafficKB which does not
> have BG/FG distinction in name.
> The content_type already captures video/audio in BG/FG explicitly. So the
bytes
> go in the appropriate user_traffic_content_type_bytes[] bucket.

Thanks for the clarification.

[Raj, 2017-05-01 22:05:46]

The CQ bit was checked by rajendrant@chromium.org

[commit-bot: I haz the power, 2017-05-01 22:06:30]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-01 22:14:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-01 22:14:27]

Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Raj, 2017-05-01 23:58:02]

rajendrant@chromium.org changed reviewers:
+ isherman@chromium.org

[Raj, 2017-05-01 23:58:03]

isherman: ptal histograms.xml

[Raj, 2017-05-02 03:22:47]

Patchset #2 (id:20001) has been deleted

[Raj, 2017-05-02 03:26:40]

The CQ bit was checked by rajendrant@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-02 03:27:00]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-02 04:55:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-02 04:55:31]

Dry run: This issue passed the CQ dry run.

[Ilya Sherman, 2017-05-02 21:28:52]

https://codereview.chromium.org/2851923002/diff/40001/components/data_use_mea...
File components/data_use_measurement/core/data_use_measurement.cc (right):

https://codereview.chromium.org/2851923002/diff/40001/components/data_use_mea...
components/data_use_measurement/core/data_use_measurement.cc:456:
"DataUse.ContentType.UserTrafficKB", 1, DataUseUserData::TYPE_MAX,
This histogram would still overflow if there are ~4.5 Terabytes of data used
during a single run of Chrome.  Is that level of use implausible?  Chrome's
uptime can be tremendously high: as high as months or years.  Though, I guess we
don't really tend to look at data from versions that are many months old, so
maybe that's not a big concern.

[Ilya Sherman, 2017-05-02 21:29:58]

https://codereview.chromium.org/2851923002/diff/40001/components/data_use_mea...
File components/data_use_measurement/core/data_use_measurement.cc (right):

https://codereview.chromium.org/2851923002/diff/40001/components/data_use_mea...
components/data_use_measurement/core/data_use_measurement.cc:456:
"DataUse.ContentType.UserTrafficKB", 1, DataUseUserData::TYPE_MAX,
On 2017/05/02 21:28:52, Ilya Sherman wrote:
> This histogram would still overflow if there are ~4.5 Terabytes of data used
> during a single run of Chrome.  Is that level of use implausible?  Chrome's
> uptime can be tremendously high: as high as months or years.  Though, I guess
we
> don't really tend to look at data from versions that are many months old, so
> maybe that's not a big concern.

Actually, slightly less than 4.5 TB, as I was calculating for unsigned ints. 
But, still, "single-digit terabytes" is the right order of magnitude.

[Raj, 2017-05-02 22:32:06]

On 2017/05/02 21:29:58, Ilya Sherman wrote:
>
https://codereview.chromium.org/2851923002/diff/40001/components/data_use_mea...
> File components/data_use_measurement/core/data_use_measurement.cc (right):
> 
>
https://codereview.chromium.org/2851923002/diff/40001/components/data_use_mea...
> components/data_use_measurement/core/data_use_measurement.cc:456:
> "DataUse.ContentType.UserTrafficKB", 1, DataUseUserData::TYPE_MAX,
> On 2017/05/02 21:28:52, Ilya Sherman wrote:
> > This histogram would still overflow if there are ~4.5 Terabytes of data used
> > during a single run of Chrome.  Is that level of use implausible?  Chrome's
> > uptime can be tremendously high: as high as months or years.  Though, I
guess
> we
> > don't really tend to look at data from versions that are many months old, so
> > maybe that's not a big concern.
> 
> Actually, slightly less than 4.5 TB, as I was calculating for unsigned ints. 
> But, still, "single-digit terabytes" is the right order of magnitude.

I guess that's fine. The alternative is to log each of the type in a separate
histogram, which amounts to too many.

[Ilya Sherman, 2017-05-04 06:34:20]

On 2017/05/02 22:32:06, Raj wrote:
> On 2017/05/02 21:29:58, Ilya Sherman wrote:
> >
>
https://codereview.chromium.org/2851923002/diff/40001/components/data_use_mea...
> > File components/data_use_measurement/core/data_use_measurement.cc (right):
> > 
> >
>
https://codereview.chromium.org/2851923002/diff/40001/components/data_use_mea...
> > components/data_use_measurement/core/data_use_measurement.cc:456:
> > "DataUse.ContentType.UserTrafficKB", 1, DataUseUserData::TYPE_MAX,
> > On 2017/05/02 21:28:52, Ilya Sherman wrote:
> > > This histogram would still overflow if there are ~4.5 Terabytes of data
used
> > > during a single run of Chrome.  Is that level of use implausible? 
Chrome's
> > > uptime can be tremendously high: as high as months or years.  Though, I
> guess
> > we
> > > don't really tend to look at data from versions that are many months old,
so
> > > maybe that's not a big concern.
> > 
> > Actually, slightly less than 4.5 TB, as I was calculating for unsigned ints.

> > But, still, "single-digit terabytes" is the right order of magnitude.
> 
> I guess that's fine. The alternative is to log each of the type in a separate
> histogram, which amounts to too many.

Sorry for being slow to respond, I've been pondering whether there's a better
approach that I could suggest here.  I think this CL is definitely an
improvement over the status quo, so LGTM as a solution for now.

I'll try to reach out if we end up coming up with a better long-term solution
for this type of data gathering need.  There's a thread about recording 64-bit
histogram values that you might want to keep an eye on, where a similar metric
is being discussed.

[Raj, 2017-05-04 18:07:07]

The CQ bit was checked by rajendrant@chromium.org

[Raj, 2017-05-04 18:07:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
ryansturm@chromium.org
Link to the patchset: https://codereview.chromium.org/2851923002/#ps40001
(title: "change array to int16 and added test")

[commit-bot: I haz the power, 2017-05-04 18:07:46]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-04 19:21:43]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493921227324550,
"parent_rev": "53e9c2e81bf4fe7d0f6bcaebca0c717b19430313", "commit_rev":
"6c9c9ed06b946f1e9666212df891ffe8fc873077"}

[commit-bot: I haz the power, 2017-05-04 19:21:55]

Description was changed from

==========
Fix overflow in user traffic content type histogram

Overflows are likely for DataUse.ContentType.UserTraffic histogram. Fix
the overflow by logging the histogram in increments of kilobytes.

BUG=679046
==========

to

==========
Fix overflow in user traffic content type histogram

Overflows are likely for DataUse.ContentType.UserTraffic histogram. Fix
the overflow by logging the histogram in increments of kilobytes.

BUG=679046

Review-Url: https://codereview.chromium.org/2851923002
Cr-Commit-Position: refs/heads/master@{#469419}
Committed:
https://chromium.googlesource.com/chromium/src/+/6c9c9ed06b946f1e9666212df891...
==========

[commit-bot: I haz the power, 2017-05-04 19:21:57]

Committed patchset #2 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/6c9c9ed06b946f1e9666212df891...