Record DRP histogram on how long back savings were cleared

Record DRP histogram on how long back savings were cleared

Record Data Reduction Proxy (DRP) histogram on how long back savings were
automatically cleared because the system clock moved back. If this happens
too frequently, then we will need to add a mechanism to get around this.
This would also help in debugging 0% savings feedback reports.

BUG=658354
Committed: https://crrev.com/2c3ecceea409b42c873949bac0aa1acaa80451e3
Cr-Commit-Position: refs/heads/master@{#438011}

[tbansal1, 2016-12-03 19:24:11]

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

[commit-bot: I haz the power, 2016-12-03 19:24:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tbansal1, 2016-12-03 19:39:22]

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

[commit-bot: I haz the power, 2016-12-03 19:39:36]

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, 2016-12-03 19:49:28]

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

[commit-bot: I haz the power, 2016-12-03 19:49:29]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tbansal1, 2016-12-04 06:24:25]

Patchset #2 (id:20001) has been deleted

[tbansal1, 2016-12-04 06:24:33]

Patchset #1 (id:1) has been deleted

[tbansal1, 2016-12-04 06:24:37]

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

[commit-bot: I haz the power, 2016-12-04 06:24:48]

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, 2016-12-04 07:20:00]

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

[commit-bot: I haz the power, 2016-12-04 07:20:01]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2016-12-05 17:42:35]

Description was changed from

==========
Record DRP prefs on when savings were cleared

BUG=
==========

to

==========
Record DRP histogram on how long back savings were cleared

Record Data Reduction Proxy (DRP) histogram on how long back savings were 
automatically cleared because the system clock moved back. If this happens 
too frequently, then we will need to add a mechanism to get around this.
This would also help in debugging 0% savings feedback reports.

BUG=658354
==========

[tbansal1, 2016-12-05 17:42:52]

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

[tbansal1, 2016-12-05 17:42:53]

ryansturm: ptal. Thanks.

[RyanSturm, 2016-12-06 18:19:33]

https://codereview.chromium.org/2546273002/diff/40001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc
(right):

https://codereview.chromium.org/2546273002/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc:285:
"DataReductionProxy.DaysSinceSavingsCleared.NegativeSystemClock",
What do you expect to get out of this histogram to look like? It seems like it
will record 0 when the clock resets and then for the users that it has reset it
will record some number of days.

Seems like you want to know how often a person's data gets reset (which is just
the number of 0's).

Is this part of the CL just for per user diagnostics? I don't see how it will be
useful except for per user diagnostic unless you are looking to find a class of
users that consistently reset their time.

[tbansal1, 2016-12-06 18:29:12]

https://codereview.chromium.org/2546273002/diff/40001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc
(right):

https://codereview.chromium.org/2546273002/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc:285:
"DataReductionProxy.DaysSinceSavingsCleared.NegativeSystemClock",
On 2016/12/06 18:19:33, Ryan Sturm wrote:
> What do you expect to get out of this histogram to look like? It seems like it
> will record 0 when the clock resets and then for the users that it has reset
it
> will record some number of days.
> 
> Seems like you want to know how often a person's data gets reset (which is
just
> the number of 0's).
> 
> Is this part of the CL just for per user diagnostics? I don't see how it will
be
> useful except for per user diagnostic unless you are looking to find a class
of
> users that consistently reset their time.

Two uses:
(1) Record how frequently the savings are reset. This can be found by just
counting the number of samples in the 0 bucket.
(2) When there is a user feedback report with 0% savings, we can look at how
long back the savings were cleared because of system clock moving back.

[RyanSturm, 2016-12-06 18:30:05]

On 2016/12/06 18:29:12, tbansal1 wrote:
>
https://codereview.chromium.org/2546273002/diff/40001/components/data_reducti...
> File
> components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc
> (right):
> 
>
https://codereview.chromium.org/2546273002/diff/40001/components/data_reducti...
>
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc:285:
> "DataReductionProxy.DaysSinceSavingsCleared.NegativeSystemClock",
> On 2016/12/06 18:19:33, Ryan Sturm wrote:
> > What do you expect to get out of this histogram to look like? It seems like
it
> > will record 0 when the clock resets and then for the users that it has reset
> it
> > will record some number of days.
> > 
> > Seems like you want to know how often a person's data gets reset (which is
> just
> > the number of 0's).
> > 
> > Is this part of the CL just for per user diagnostics? I don't see how it
will
> be
> > useful except for per user diagnostic unless you are looking to find a class
> of
> > users that consistently reset their time.
> 
> Two uses:
> (1) Record how frequently the savings are reset. This can be found by just
> counting the number of samples in the 0 bucket.
sgtm.

> (2) When there is a user feedback report with 0% savings, we can look at how
> long back the savings were cleared because of system clock moving back.
It's weird, but that's what I thought you were using it for.
lgtm.

[tbansal1, 2016-12-06 19:07:21]

tbansal@chromium.org changed reviewers:
+ asvitkine@chromium.org

[tbansal1, 2016-12-06 19:07:22]

asvitkine: ptal at histograms.xml. Thanks.

[tbansal1, 2016-12-06 19:47:12]

Patchset #2 (id:60001) has been deleted

[Alexei Svitkine (slow), 2016-12-07 16:47:27]

https://codereview.chromium.org/2546273002/diff/40001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc
(right):

https://codereview.chromium.org/2546273002/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc:940:
0, 0, 365, 50);
Seems strange that you're recording this in a completely different place than
the other samples.

I suggest at least making a helper function for it, so that the macro, name and
params don't have to be duplicated and calling it from both places.

https://codereview.chromium.org/2546273002/diff/40001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc
(right):

https://codereview.chromium.org/2546273002/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc:289:
0, 365, 50);
Min value should be  >= 1, see comment in histogram_macros.h.

[tbansal1, 2016-12-08 00:21:53]

Patchset #2 (id:80001) has been deleted

[tbansal1, 2016-12-08 00:22:27]

Patchset #2 (id:100001) has been deleted

[tbansal1, 2016-12-08 00:23:20]

asvitkine: ptal.

https://codereview.chromium.org/2546273002/diff/40001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc
(right):

https://codereview.chromium.org/2546273002/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc:940:
0, 0, 365, 50);
On 2016/12/07 16:47:27, Alexei Svitkine (slow) wrote:
> Seems strange that you're recording this in a completely different place than
> the other samples.
> 
> I suggest at least making a helper function for it, so that the macro, name
and
> params don't have to be duplicated and calling it from both places.

Using two different histograms now since they were essentially recording
different things.

https://codereview.chromium.org/2546273002/diff/40001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc
(right):

https://codereview.chromium.org/2546273002/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.cc:289:
0, 365, 50);
On 2016/12/07 16:47:27, Alexei Svitkine (slow) wrote:
> Min value should be  >= 1, see comment in histogram_macros.h.

Done.

[RyanSturm, 2016-12-08 00:29:52]

This makes more sense now. Thanks for the changes.

[Alexei Svitkine (slow), 2016-12-08 21:24:24]

https://codereview.chromium.org/2546273002/diff/120001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc
(right):

https://codereview.chromium.org/2546273002/diff/120001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc:938:
"DataReductionProxy.CountSavingsCleared.NegativeSystemClock", 1, 2);
I think logging this in isolation is not very helpful.

Suppose you have a value like "19383". Is this a lot? Not so much? It doesn't
tell you without knowing something else - like "out of how many users?". It
would be better to record a boolean histogram here for both false and true cases
so you can tell the ratio in the population.

[tbansal1, 2016-12-08 21:47:16]

https://codereview.chromium.org/2546273002/diff/120001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc
(right):

https://codereview.chromium.org/2546273002/diff/120001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc:938:
"DataReductionProxy.CountSavingsCleared.NegativeSystemClock", 1, 2);
On 2016/12/08 21:24:24, Alexei Svitkine (slow) wrote:
> I think logging this in isolation is not very helpful.
> 
> Suppose you have a value like "19383". Is this a lot? Not so much? It doesn't
> tell you without knowing something else - like "out of how many users?". It
> would be better to record a boolean histogram here for both false and true
cases
> so you can tell the ratio in the population.

There are other UMA histograms that record that. The denominator here simply
would be # of Chrome sessions, or # of users (depending on the use case) that
have DRP enabled. I can record this as a boolean but then we are just sending
duplicate data.

[Alexei Svitkine (slow), 2016-12-08 22:05:41]

The best practice is to have the data all in one histogram.

On Thu, Dec 8, 2016 at 4:47 PM, <tbansal@chromium.org> wrote:

>
> https://codereview.chromium.org/2546273002/diff/120001/
> components/data_reduction_proxy/core/browser/data_
> reduction_proxy_compression_stats.cc
> File
> components/data_reduction_proxy/core/browser/data_
> reduction_proxy_compression_stats.cc
> (right):
>
> https://codereview.chromium.org/2546273002/diff/120001/
> components/data_reduction_proxy/core/browser/data_
> reduction_proxy_compression_stats.cc#newcode938
> components/data_reduction_proxy/core/browser/data_
> reduction_proxy_compression_stats.cc:938:
> "DataReductionProxy.CountSavingsCleared.NegativeSystemClock", 1, 2);
> On 2016/12/08 21:24:24, Alexei Svitkine (slow) wrote:
> > I think logging this in isolation is not very helpful.
> >
> > Suppose you have a value like "19383". Is this a lot? Not so much? It
> doesn't
> > tell you without knowing something else - like "out of how many
> users?". It
> > would be better to record a boolean histogram here for both false and
> true cases
> > so you can tell the ratio in the population.
>
> There are other UMA histograms that record that. The denominator here
> simply would be # of Chrome sessions, or # of users (depending on the
> use case) that have DRP enabled. I can record this as a boolean but then
> we are just sending duplicate data.
>
> https://codereview.chromium.org/2546273002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[tbansal1, 2016-12-09 23:46:13]

Patchset #3 (id:140001) has been deleted

[tbansal1, 2016-12-09 23:46:20]

Patchset #4 (id:180001) has been deleted

[tbansal1, 2016-12-09 23:46:30]

Patchset #3 (id:160001) has been deleted

[tbansal1, 2016-12-09 23:47:49]

asvitkine: ptal. Thanks.

[Alexei Svitkine (slow), 2016-12-12 20:06:25]

lgtm

[tbansal1, 2016-12-12 23:19:54]

The CQ bit was checked by tbansal@chromium.org

[tbansal1, 2016-12-12 23:19:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
ryansturm@chromium.org, asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/2546273002/#ps220001
(title: "Rebased")

[commit-bot: I haz the power, 2016-12-12 23:21:31]

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, 2016-12-13 03:05:23]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1481584794369190,
"parent_rev": "9062f16d6e087d18bd958b1a7ac7b2b839ff08bb", "commit_rev":
"6a60deb4244bcc45c04552745bdaa91c16a825b3"}

[commit-bot: I haz the power, 2016-12-13 03:05:49]

Description was changed from

==========
Record DRP histogram on how long back savings were cleared

Record Data Reduction Proxy (DRP) histogram on how long back savings were 
automatically cleared because the system clock moved back. If this happens 
too frequently, then we will need to add a mechanism to get around this.
This would also help in debugging 0% savings feedback reports.

BUG=658354
==========

to

==========
Record DRP histogram on how long back savings were cleared

Record Data Reduction Proxy (DRP) histogram on how long back savings were 
automatically cleared because the system clock moved back. If this happens 
too frequently, then we will need to add a mechanism to get around this.
This would also help in debugging 0% savings feedback reports.

BUG=658354

Review-Url: https://codereview.chromium.org/2546273002
==========

[commit-bot: I haz the power, 2016-12-13 03:05:50]

Committed patchset #4 (id:220001)

[commit-bot: I haz the power, 2016-12-13 03:07:55]

Description was changed from

==========
Record DRP histogram on how long back savings were cleared

Record Data Reduction Proxy (DRP) histogram on how long back savings were 
automatically cleared because the system clock moved back. If this happens 
too frequently, then we will need to add a mechanism to get around this.
This would also help in debugging 0% savings feedback reports.

BUG=658354

Review-Url: https://codereview.chromium.org/2546273002
==========

to

==========
Record DRP histogram on how long back savings were cleared

Record Data Reduction Proxy (DRP) histogram on how long back savings were
automatically cleared because the system clock moved back. If this happens
too frequently, then we will need to add a mechanism to get around this.
This would also help in debugging 0% savings feedback reports.

BUG=658354

Committed: https://crrev.com/2c3ecceea409b42c873949bac0aa1acaa80451e3
Cr-Commit-Position: refs/heads/master@{#438011}
==========

[commit-bot: I haz the power, 2016-12-13 03:07:56]

Patchset 4 (id:??) landed as
https://crrev.com/2c3ecceea409b42c873949bac0aa1acaa80451e3
Cr-Commit-Position: refs/heads/master@{#438011}