Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2810053004:
[Remote suggestions] Report time distribution of triggers (Closed)
Description[Remote suggestions] Report time distribution of triggers
When changing scheduling intervals for soft background fetches, we had
to guess blindly what the impact on traffic this will have.
This CL records the distribution of:
- when first trigger comes (not necessarily successful), and
- when the (first) successful trigger comes.
The former helps us to evaluate impact of scheduling changes. The latter
helps us to understand what are the real fetching intervals in the wild.
BUG=710828
Review-Url: https://codereview.chromium.org/2810053004
Cr-Commit-Position: refs/heads/master@{#464595}
Committed: https://chromium.googlesource.com/chromium/src/+/51a10acb782ef180630115d21190b7c70cb44944
Patch Set 1 #
Total comments: 10
Patch Set 2 : Marc's comments #
Total comments: 4
Patch Set 3 : Marc's comments #2 #Patch Set 4 : Minor polish #
Total comments: 10
Patch Set 5 : Ilya's comments #
Messages
Total messages: 17 (7 generated)
The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
jkrcal@chromium.org changed reviewers: + isherman@chromium.org, treib@chromium.org
Marc, could you PTAL? Ilya, could you PTAL at histograms.xml?
https://codereview.chromium.org/2810053004/diff/1/components/ntp_snippets/rem... File components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc (right): https://codereview.chromium.org/2810053004/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc:150: const base::TimeDelta& time_until_first_failed_trigger) { s/time_until_first_failed_trigger/time_until_first_trigger/ (This records any, not just failed, right?) And again, TimeDelta is fine to pass by value. https://codereview.chromium.org/2810053004/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc:502: const base::Time last_fetch_attempt_time = base::Time::FromInternalValue( What exactly does fetch *attempt* mean? (Does it matter for the metrics?) https://codereview.chromium.org/2810053004/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc:504: if (!time_until_first_trigger_reported_) { Should we exclude PERSISTENT_SCHEDULER_WAKE_UP here? https://codereview.chromium.org/2810053004/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc:519: ReportTimeUntilFirstSuccessfulTrigger( and here? https://codereview.chromium.org/2810053004/diff/1/components/ntp_snippets/rem... File components/ntp_snippets/remote/remote_suggestions_scheduler_impl.h (right): https://codereview.chromium.org/2810053004/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/remote_suggestions_scheduler_impl.h:97: const base::Time& last_fetch_attempt_time); nit: Time is essentially an int64, so it's fine to pass by value.
Thanks, Marc! PTAL again. https://codereview.chromium.org/2810053004/diff/1/components/ntp_snippets/rem... File components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc (right): https://codereview.chromium.org/2810053004/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc:150: const base::TimeDelta& time_until_first_failed_trigger) { On 2017/04/12 12:48:31, Marc Treib wrote: > s/time_until_first_failed_trigger/time_until_first_trigger/ > (This records any, not just failed, right?) > > And again, TimeDelta is fine to pass by value. Done. https://codereview.chromium.org/2810053004/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc:502: const base::Time last_fetch_attempt_time = base::Time::FromInternalValue( On 2017/04/12 12:48:31, Marc Treib wrote: > What exactly does fetch *attempt* mean? (Does it matter for the metrics?) It is the last time we sent a request to the server. We do not care whether the request was successful or not so that we continue with the regular schedule and do not burn the server by frequent retries whenever if it is overloaded. This time is used as reference for ShouldRefetchInTheBackgroundNow() so this is also the right reference time for the metrics. https://codereview.chromium.org/2810053004/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc:504: if (!time_until_first_trigger_reported_) { On 2017/04/12 12:48:31, Marc Treib wrote: > Should we exclude PERSISTENT_SCHEDULER_WAKE_UP here? Good point, done! https://codereview.chromium.org/2810053004/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc:519: ReportTimeUntilFirstSuccessfulTrigger( On 2017/04/12 12:48:31, Marc Treib wrote: > and here? Good point! I've introduced another histogram for persistent fetches because we never really understood how they get triggered out there. https://codereview.chromium.org/2810053004/diff/1/components/ntp_snippets/rem... File components/ntp_snippets/remote/remote_suggestions_scheduler_impl.h (right): https://codereview.chromium.org/2810053004/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/remote_suggestions_scheduler_impl.h:97: const base::Time& last_fetch_attempt_time); On 2017/04/12 12:48:31, Marc Treib wrote: > nit: Time is essentially an int64, so it's fine to pass by value. Done.
lgtm Thanks! https://codereview.chromium.org/2810053004/diff/20001/components/ntp_snippets... File components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc (right): https://codereview.chromium.org/2810053004/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc:148: void ReportTimeUntilFirstTrigger(UserClassifier::UserClass user_class, optional: s/FirstTrigger/FirstSoftTrigger/ ? https://codereview.chromium.org/2810053004/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc:176: void ReportTimeUntilFirstSuccessfulTrigger( similar here
Thanks, done! https://codereview.chromium.org/2810053004/diff/20001/components/ntp_snippets... File components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc (right): https://codereview.chromium.org/2810053004/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc:148: void ReportTimeUntilFirstTrigger(UserClassifier::UserClass user_class, On 2017/04/12 13:43:30, Marc Treib wrote: > optional: s/FirstTrigger/FirstSoftTrigger/ ? Done. https://codereview.chromium.org/2810053004/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc:176: void ReportTimeUntilFirstSuccessfulTrigger( On 2017/04/12 13:43:30, Marc Treib wrote: > similar here Actually renamed even better to match the PersistentFetch below.
https://codereview.chromium.org/2810053004/diff/60001/components/ntp_snippets... File components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc (right): https://codereview.chromium.org/2810053004/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc:153: "NewTabPage.ContentSuggestions.TimeUntilFirstSoftTrigger_RareNTPUser", Optional: You're welcome to use dot instead of underscore for the suffixes, and specify separator="." in histograms.xml https://codereview.chromium.org/2810053004/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc:156: /*bucket_count=*/100); Do you need 100 buckets, or would 50 suffice? 50 is the more typical bucket count, and using extra buckets has a memory usage cost for users. (Applies to all of the histograms.) https://codereview.chromium.org/2810053004/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2810053004/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:41671: +<histogram name="NewTabPage.ContentSuggestions.TimeUntilFirstSoftTrigger" nit: Please add base="true" to this histogram name, as it seems you never emit to it directly (as opposed to the suffixed versions) https://codereview.chromium.org/2810053004/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:41677: + interval elapses, or after this moment. This metric is recorded once per I don't quite understand this description. One concern based on my unclear understanding: Is it possible for the values recorded to this histogram to be negative, or are they guaranteed to be non-negative? UMA histograms unfortunately do not support negative values. https://codereview.chromium.org/2810053004/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:41697: + Android: The time since the last fetch recorded upon a soft fetch. This is nit: Please add a comma before "recorded"; ditto above.
Thanks, Ilya! PTAL, again. https://codereview.chromium.org/2810053004/diff/60001/components/ntp_snippets... File components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc (right): https://codereview.chromium.org/2810053004/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc:153: "NewTabPage.ContentSuggestions.TimeUntilFirstSoftTrigger_RareNTPUser", On 2017/04/12 22:53:42, Ilya Sherman wrote: > Optional: You're welcome to use dot instead of underscore for the suffixes, and > specify separator="." in histograms.xml Done. https://codereview.chromium.org/2810053004/diff/60001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc:156: /*bucket_count=*/100); On 2017/04/12 22:53:42, Ilya Sherman wrote: > Do you need 100 buckets, or would 50 suffice? 50 is the more typical bucket > count, and using extra buckets has a memory usage cost for users. (Applies to > all of the histograms.) Done. (I did it consistently with a similar histogram NewTabPage.ContentSuggestions.TimeSinceSuggestionFetched but neither consistency, nor 100 buckets are actually needed) https://codereview.chromium.org/2810053004/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2810053004/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:41671: +<histogram name="NewTabPage.ContentSuggestions.TimeUntilFirstSoftTrigger" On 2017/04/12 22:53:42, Ilya Sherman wrote: > nit: Please add base="true" to this histogram name, as it seems you never emit > to it directly (as opposed to the suffixed versions) Oh, cool! I did not know this parameter. Done. https://codereview.chromium.org/2810053004/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:41677: + interval elapses, or after this moment. This metric is recorded once per On 2017/04/12 22:53:42, Ilya Sherman wrote: > I don't quite understand this description. One concern based on my unclear > understanding: Is it possible for the values recorded to this histogram to be > negative, or are they guaranteed to be non-negative? UMA histograms > unfortunately do not support negative values. Yes, this is guaranteed to be non-negative. I've also rephrased the description. https://codereview.chromium.org/2810053004/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:41697: + Android: The time since the last fetch recorded upon a soft fetch. This is On 2017/04/12 22:53:42, Ilya Sherman wrote: > nit: Please add a comma before "recorded"; ditto above. Done.
Metrics LGTM, thanks.
The CQ bit was checked by jkrcal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org Link to the patchset: https://codereview.chromium.org/2810053004/#ps80001 (title: "Ilya's comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1492120027567370,
"parent_rev": "71dfbd1f2228ae70efcea0656b3555ab539430fd", "commit_rev":
"51a10acb782ef180630115d21190b7c70cb44944"}
Message was sent while issue was closed.
Description was changed from ========== [Remote suggestions] Report time distribution of triggers When changing scheduling intervals for soft background fetches, we had to guess blindly what the impact on traffic this will have. This CL records the distribution of: - when first trigger comes (not necessarily successful), and - when the (first) successful trigger comes. The former helps us to evaluate impact of scheduling changes. The latter helps us to understand what are the real fetching intervals in the wild. BUG=710828 ========== to ========== [Remote suggestions] Report time distribution of triggers When changing scheduling intervals for soft background fetches, we had to guess blindly what the impact on traffic this will have. This CL records the distribution of: - when first trigger comes (not necessarily successful), and - when the (first) successful trigger comes. The former helps us to evaluate impact of scheduling changes. The latter helps us to understand what are the real fetching intervals in the wild. BUG=710828 Review-Url: https://codereview.chromium.org/2810053004 Cr-Commit-Position: refs/heads/master@{#464595} Committed: https://chromium.googlesource.com/chromium/src/+/51a10acb782ef180630115d21190... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/51a10acb782ef180630115d21190... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
