Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2574073003:
[NTPSnippets] Records the time since last successful bg fetch. (Closed)
|
Created:
4 years ago by markusheintz_ Modified:
4 years ago CC:
chromium-reviews, asvitkine+watch_chromium.org, ntp-dev+reviews_chromium.org, jkrcal Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[NTPSnippets] Records the time since last successful bg fetch. Therefore a new UMA histogram is added.
BUG=665873
Committed: https://crrev.com/ad98d7dc609cf909ba2594267071253f83c2cc2f
Cr-Commit-Position: refs/heads/master@{#438823}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Address comments treib #
Total comments: 1
Patch Set 3 : Rebase & address last comment treib #
Messages
Total messages: 27 (13 generated)
The CQ bit was checked by markusheintz@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...
Description was changed from ========== [NTPSnippets] Adds a histogram for recording the remote content suggestion age since last backgroun fetch. BUG=665873 ========== to ========== [NTPSnippets] Adds a histogram for recording the remote content suggestion age since last backgroun fetch. BUG=665873 ==========
markusheintz@chromium.org changed reviewers: + treib@chromium.org
Marc PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
markusheintz@chromium.org changed reviewers: + holte@chromium.org
On 2016/12/14 15:16:00, markusheintz_ wrote: > Marc PTAL Adding Steven for reviewing the histograms.xml changes. Thanks
lgtm
Sorry for the delay. Bunch of naming/description nits, otherwise looks good! https://codereview.chromium.org/2574073003/diff/1/components/ntp_snippets/con... File components/ntp_snippets/content_suggestions_metrics.cc (right): https://codereview.chromium.org/2574073003/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_metrics.cc:55: "NewTabPage.ContentSuggestions.BackgroundFetchAge"; nit: I'm not super happy with the term "Age" here - it's really just the time since the last background fetch. So how about ".TimeSinceLastBackgroundFetch"? https://codereview.chromium.org/2574073003/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_metrics.cc:274: // background fetch time. This is different from the actual content age as Also here: It's not the age of the suggestion, it's the time since the last fetch. This particular suggestion might potentially even be from an earlier fetch (if you discarded all newer suggestions, or if the latest fetch didn't return anything), or it might be from a newer foreground fetch. https://codereview.chromium.org/2574073003/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2574073003/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:38300: + content suggestions and displaying remote content suggestions to the user. nit: Since this doesn't directly correspond to a user action, the description should mention when exactly it is recorded. How about something like: Android: The time since the last successful background fetch of remote content suggestions. Recorded when the user looks at content suggestions on the NTP.
Ah, and one description nit: Try to keep the first line <= 72 chars (it'll become the git commit title, and get truncated otherwise).
Description was changed from ========== [NTPSnippets] Adds a histogram for recording the remote content suggestion age since last backgroun fetch. BUG=665873 ========== to ========== [NTPSnippets] Records the time since last successful bg fetch. Therefore a new UMA histogram is added. BUG=665873 ==========
Address comments treib
On 2016/12/15 12:26:52, Marc Treib wrote: > Ah, and one description nit: Try to keep the first line <= 72 chars (it'll > become the git commit title, and get truncated otherwise). done
https://codereview.chromium.org/2574073003/diff/1/components/ntp_snippets/con... File components/ntp_snippets/content_suggestions_metrics.cc (right): https://codereview.chromium.org/2574073003/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_metrics.cc:55: "NewTabPage.ContentSuggestions.BackgroundFetchAge"; On 2016/12/15 10:16:41, Marc Treib wrote: > nit: I'm not super happy with the term "Age" here - it's really just the time > since the last background fetch. So how about ".TimeSinceLastBackgroundFetch"? Excellent name suggestion!! I find it much better than mine. Thanks a lot! https://codereview.chromium.org/2574073003/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_metrics.cc:274: // background fetch time. This is different from the actual content age as On 2016/12/15 10:16:41, Marc Treib wrote: > Also here: It's not the age of the suggestion, it's the time since the last > fetch. This particular suggestion might potentially even be from an earlier > fetch (if you discarded all newer suggestions, or if the latest fetch didn't > return anything), or it might be from a newer foreground fetch. That's what I describe in the comment. I simplified and shortened to comment to make it more clear and be more consistent with the new naming. https://codereview.chromium.org/2574073003/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2574073003/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:38300: + content suggestions and displaying remote content suggestions to the user. On 2016/12/15 10:16:41, Marc Treib wrote: > nit: Since this doesn't directly correspond to a user action, the description > should mention when exactly it is recorded. How about something like: > Android: The time since the last successful background fetch of remote content > suggestions. Recorded when the user looks at content suggestions on the NTP. Thanks again a great suggestion. I find this easier to understand than mine. In general I think it does relate to a user action. The user pulling up the remote suggestions list is the action that trigger recording the time since last background fetch.
LGTM with one more optional suggestion, thanks! https://codereview.chromium.org/2574073003/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2574073003/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:38300: + content suggestions and displaying remote content suggestions to the user. On 2016/12/15 12:42:07, markusheintz_ wrote: > On 2016/12/15 10:16:41, Marc Treib wrote: > > nit: Since this doesn't directly correspond to a user action, the description > > should mention when exactly it is recorded. How about something like: > > Android: The time since the last successful background fetch of remote content > > suggestions. Recorded when the user looks at content suggestions on the NTP. > > Thanks again a great suggestion. I find this easier to understand than mine. > > In general I think it does relate to a user action. The user pulling up the > remote suggestions list is the action that trigger recording the time since last > background fetch. It is triggered by a user action, but "TimeSinceLastBackgroundFetch" is not a user action itself. That's why I insisted on a clear description :) https://codereview.chromium.org/2574073003/diff/20001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_metrics.cc (right): https://codereview.chromium.org/2574073003/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_metrics.cc:278: base::TimeDelta::FromSeconds(1), base::TimeDelta::FromDays(3), Should we make the max a bit bigger, to also capture "bad" cases where no background fetch has happened for a while?
On 2016/12/15 12:47:22, Marc Treib wrote: > LGTM with one more optional suggestion, thanks! > > https://codereview.chromium.org/2574073003/diff/1/tools/metrics/histograms/hi... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/2574073003/diff/1/tools/metrics/histograms/hi... > tools/metrics/histograms/histograms.xml:38300: + content suggestions and > displaying remote content suggestions to the user. > On 2016/12/15 12:42:07, markusheintz_ wrote: > > On 2016/12/15 10:16:41, Marc Treib wrote: > > > nit: Since this doesn't directly correspond to a user action, the > description > > > should mention when exactly it is recorded. How about something like: > > > Android: The time since the last successful background fetch of remote > content > > > suggestions. Recorded when the user looks at content suggestions on the NTP. > > > > Thanks again a great suggestion. I find this easier to understand than mine. > > > > In general I think it does relate to a user action. The user pulling up the > > remote suggestions list is the action that trigger recording the time since > last > > background fetch. > > It is triggered by a user action, but "TimeSinceLastBackgroundFetch" is not a > user action itself. That's why I insisted on a clear description :) > > https://codereview.chromium.org/2574073003/diff/20001/components/ntp_snippets... > File components/ntp_snippets/content_suggestions_metrics.cc (right): > > https://codereview.chromium.org/2574073003/diff/20001/components/ntp_snippets... > components/ntp_snippets/content_suggestions_metrics.cc:278: > base::TimeDelta::FromSeconds(1), base::TimeDelta::FromDays(3), > Should we make the max a bit bigger, to also capture "bad" cases where no > background fetch has happened for a while? Increased to 7.
Rebase & address last comment treib
The CQ bit was checked by markusheintz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from holte@chromium.org, treib@chromium.org Link to the patchset: https://codereview.chromium.org/2574073003/#ps40001 (title: "Rebase & address last comment treib")
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": 40001, "attempt_start_ts": 1481807426525690,
"parent_rev": "cd413627f339a46647bffd51cbbf3b78e54826db", "commit_rev":
"2c1727ba259ede92321be7862249663410018e25"}
Message was sent while issue was closed.
Description was changed from ========== [NTPSnippets] Records the time since last successful bg fetch. Therefore a new UMA histogram is added. BUG=665873 ========== to ========== [NTPSnippets] Records the time since last successful bg fetch. Therefore a new UMA histogram is added. BUG=665873 Review-Url: https://codereview.chromium.org/2574073003 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [NTPSnippets] Records the time since last successful bg fetch. Therefore a new UMA histogram is added. BUG=665873 Review-Url: https://codereview.chromium.org/2574073003 ========== to ========== [NTPSnippets] Records the time since last successful bg fetch. Therefore a new UMA histogram is added. BUG=665873 Committed: https://crrev.com/ad98d7dc609cf909ba2594267071253f83c2cc2f Cr-Commit-Position: refs/heads/master@{#438823} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/ad98d7dc609cf909ba2594267071253f83c2cc2f Cr-Commit-Position: refs/heads/master@{#438823} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
