predictors: Add ResourcePrefetchPredictor database readiness histogram.

predictors: Add ResourcePrefetchPredictor database readiness histogram.

Predictor needs time to learn about user favorites pages. Special age metric is
introduced to show that the predictor database is large enough. Metric is
"large" when most users have learned about most of their pages, and "small"
otherwise.

BUG=680049
Review-Url: https://codereview.chromium.org/2625063002
Cr-Commit-Position: refs/heads/master@{#443521}
Committed: https://chromium.googlesource.com/chromium/src/+/27032ce5253ef29711946ce32a68f3ce0ab6298f

[alexilin, 2017-01-11 10:06:15]

alexilin@chromium.org changed reviewers:
+ lizeb@chromium.org

[alexilin, 2017-01-11 10:06:16]

Hey! I've started to implement histograms for upcoming finch experiment.

Top-level question: is it okay to add histograms one by one? Would it be better
to make one CL with all hists?

https://codereview.chromium.org/2625063002/diff/1/chrome/browser/predictors/r...
File chrome/browser/predictors/resource_prefetch_predictor.h (left):

https://codereview.chromium.org/2625063002/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor.h:314: // Reports overall
page load time.
Oops! These are undefined and unused methods.

[alexilin, 2017-01-11 17:20:56]

From offline discussion we've decided 
- to filter users with no sufficient history 
- to don't count db entry if it doesn't contain prefetchable resources.

[Benoit L, 2017-01-12 15:34:55]

lgtm, thanks.

https://codereview.chromium.org/2625063002/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2625063002/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:53231: +<histogram
name="ResourcePrefetchPredictor.DatabaseReadiness">
nit: Since the reported value is a percentage, add the attribute
units="%"

https://codereview.chromium.org/2625063002/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:53236: +    for last X entries in
history, sampled with some probability on each
nit: Please add that this metric is only gathered when a minimum number of
visits is reached.

[alexilin, 2017-01-12 15:59:13]

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

[alexilin, 2017-01-12 15:59:14]

isherman: please review histograms.xml, thanks!

[Ilya Sherman, 2017-01-12 21:10:39]

https://codereview.chromium.org/2625063002/diff/80001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor.cc (right):

https://codereview.chromium.org/2625063002/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:617: // Report
readiness metric with 20% probability.
What's the reason for sampling?

https://codereview.chromium.org/2625063002/diff/80001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2625063002/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:53238: +    visits is reached.
Hmm, I find the wording of this description a bit hard to follow.  Here's a
suggested rewording -- hopefully I understood correctly and kept the semantics
the same!  Please feel free to refine further:

"This metric measures the coverage of the predictor database: On each
navigation, it records what percentage of the top X history entries are found in
the predictor DB.  Caveats: (1) This metric is only recorded once the user has
accumulated enough browsing history, implemented as a minimum threshold of total
page visits by the user.  (2) This metric is sampled, so it's actually only
recorded for 1 out of every K navigations."

If you omit the sampling as I suggest, then I'd drop the "Caveats: (1)" text and
the second caveat.

[alexilin, 2017-01-12 22:53:13]

https://codereview.chromium.org/2625063002/diff/80001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor.cc (right):

https://codereview.chromium.org/2625063002/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:617: // Report
readiness metric with 20% probability.
On 2017/01/12 21:10:39, Ilya Sherman wrote:
> What's the reason for sampling?

Because of relatively high computation cost. We need to make round trip from UI
thread to DB and back for the top hosts list + to search top hosts in predictor
db cache.
I don't think that the sampling will cause any analysis issues.

https://codereview.chromium.org/2625063002/diff/80001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2625063002/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:53238: +    visits is reached.
On 2017/01/12 21:10:39, Ilya Sherman wrote:
> Hmm, I find the wording of this description a bit hard to follow.  Here's a
> suggested rewording -- hopefully I understood correctly and kept the semantics
> the same!  Please feel free to refine further:
> 
> "This metric measures the coverage of the predictor database: On each
> navigation, it records what percentage of the top X history entries are found
in
> the predictor DB.  Caveats: (1) This metric is only recorded once the user has
> accumulated enough browsing history, implemented as a minimum threshold of
total
> page visits by the user.  (2) This metric is sampled, so it's actually only
> recorded for 1 out of every K navigations."
> 
> If you omit the sampling as I suggest, then I'd drop the "Caveats: (1)" text
and
> the second caveat.

Thanks Ilya! I can't say better than you suggested.
Let's discuss the sampling problem in the other comment.

[alexilin, 2017-01-12 22:58:02]

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

[commit-bot: I haz the power, 2017-01-12 22:58:44]

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

[Ilya Sherman, 2017-01-12 23:08:08]

Metrics LGTM

https://codereview.chromium.org/2625063002/diff/80001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor.cc (right):

https://codereview.chromium.org/2625063002/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:617: // Report
readiness metric with 20% probability.
On 2017/01/12 22:53:13, alexilin wrote:
> On 2017/01/12 21:10:39, Ilya Sherman wrote:
> > What's the reason for sampling?
> 
> Because of relatively high computation cost. We need to make round trip from
UI
> thread to DB and back for the top hosts list + to search top hosts in
predictor
> db cache.
> I don't think that the sampling will cause any analysis issues.

Okay, fair enough!

[commit-bot: I haz the power, 2017-01-13 01:25:57]

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

[commit-bot: I haz the power, 2017-01-13 01:25:58]

Dry run: This issue passed the CQ dry run.

[alexilin, 2017-01-13 09:22:31]

The CQ bit was checked by alexilin@chromium.org

[alexilin, 2017-01-13 09:22:31]

The patchset sent to the CQ was uploaded after l-g-t-m from lizeb@chromium.org
Link to the patchset: https://codereview.chromium.org/2625063002/#ps100001
(title: "Update histogram description.")

[commit-bot: I haz the power, 2017-01-13 09:22:44]

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-01-13 09:26:47]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1484299351222390,
"parent_rev": "2ee8b2c6aee296fd97c2064181aa77a240c98477", "commit_rev":
"27032ce5253ef29711946ce32a68f3ce0ab6298f"}

[commit-bot: I haz the power, 2017-01-13 09:27:31]

Description was changed from

==========
predictors: Add ResourcePrefetchPredictor database readiness histogram.

Predictor needs time to learn about user favorites pages. Special age metric is
introduced to show that the predictor database is large enough. Metric is
"large" when most users have learned about most of their pages, and "small"
otherwise.

BUG=680049
==========

to

==========
predictors: Add ResourcePrefetchPredictor database readiness histogram.

Predictor needs time to learn about user favorites pages. Special age metric is
introduced to show that the predictor database is large enough. Metric is
"large" when most users have learned about most of their pages, and "small"
otherwise.

BUG=680049

Review-Url: https://codereview.chromium.org/2625063002
Cr-Commit-Position: refs/heads/master@{#443521}
Committed:
https://chromium.googlesource.com/chromium/src/+/27032ce5253ef29711946ce32a68...
==========

[commit-bot: I haz the power, 2017-01-13 09:27:32]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/27032ce5253ef29711946ce32a68...