Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2639533003:
[Content suggestions] Report updates in the UI to UMA. (Closed)
Description[Content suggestions] Report updates in the UI to UMA.
Content suggestions on the Android New Tab Page can get updated if the
user has not seen them yet.
This CL adds a histogram to reason about how often are these updates
successful.
BUG=674163
Review-Url: https://codereview.chromium.org/2639533003
Cr-Commit-Position: refs/heads/master@{#444896}
Committed: https://chromium.googlesource.com/chromium/src/+/ede80b706285aa0052cf154d89c49f081a74b9a5
Patch Set 1 #
Total comments: 9
Patch Set 2 : Comments #1 #
Total comments: 11
Patch Set 3 : Comments #2 #Patch Set 4 : Rebase #Patch Set 5 : Fix errors #Patch Set 6 : Make the enum more granular #Patch Set 7 : Fix errors #2 #
Total comments: 3
Depends on Patchset: Messages
Total messages: 30 (18 generated)
jkrcal@chromium.org changed reviewers: + dgn@chromium.org, rkaplow@chromium.org
Nicolas, could you PTAL? Robert, could you PTAL at histograms.xml?
lgtm https://codereview.chromium.org/2639533003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java (right): https://codereview.chromium.org/2639533003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:36: public static final int UI_UPDATE_REPLACE_ALL = 1; i noticed the name of this is a bit different from the histogram enum name. might be bestg to keep a little more consistant https://codereview.chromium.org/2639533003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:40: public static final int UI_UPDATE_COUNT = 5; may want a comment calling out that UI_UPDATE_COUNT needs to be last. https://codereview.chromium.org/2639533003/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2639533003/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:107092: +<enum name="UIUpdateStatus" type="int"> nit, might add NewTabPage to the enum name
https://codereview.chromium.org/2639533003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java (right): https://codereview.chromium.org/2639533003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:361: "NewTabPage.ContentSuggestions.UIUpdateStatus", can this be a constant? or wrapped in a method? if you go with a method that wraps the recordEnumeratedHistogram call, can you also please make the int values of the enum in an @IntDef please? (like NTPLayoutResult in NewTabPageUma for example) https://codereview.chromium.org/2639533003/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2639533003/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:107092: +<enum name="UIUpdateStatus" type="int"> On 2017/01/17 23:12:12, rkaplow wrote: > nit, might add NewTabPage to the enum name or ContentSuggestions, since we won't always stay on the NTP
Thanks for the comments! I waited for another related CL to finish and updated this one. PTAL, again! https://codereview.chromium.org/2639533003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java (right): https://codereview.chromium.org/2639533003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:36: public static final int UI_UPDATE_REPLACE_ALL = 1; On 2017/01/17 23:12:12, rkaplow wrote: > i noticed the name of this is a bit different from the histogram enum name. > might be bestg to keep a little more consistant Tried to make it consistent, now. https://codereview.chromium.org/2639533003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:40: public static final int UI_UPDATE_COUNT = 5; On 2017/01/17 23:12:12, rkaplow wrote: > may want a comment calling out that UI_UPDATE_COUNT needs to be last. Done. https://codereview.chromium.org/2639533003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:361: "NewTabPage.ContentSuggestions.UIUpdateStatus", On 2017/01/18 11:27:55, dgn wrote: > can this be a constant? or wrapped in a method? if you go with a method that > wraps the recordEnumeratedHistogram call, can you also please make the int > values of the enum in an @IntDef please? (like NTPLayoutResult in NewTabPageUma > for example) Thanks, done. Moved to NewTabPageUma.java. https://codereview.chromium.org/2639533003/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2639533003/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:107092: +<enum name="UIUpdateStatus" type="int"> On 2017/01/18 11:27:55, dgn wrote: > On 2017/01/17 23:12:12, rkaplow wrote: > > nit, might add NewTabPage to the enum name > > or ContentSuggestions, since we won't always stay on the NTP Done.
https://codereview.chromium.org/2639533003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageUma.java (right): https://codereview.chromium.org/2639533003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageUma.java:102: UI_UPDATE_FAIL_ALL_SEEN, UI_UPDATE_FAIL_DISABLED, NUM_UI_UPDATE_RESULTS}) I wouldn't put NUM_UI_UPDATE_RESULTS there, it's not a valid value for the enum, right? https://codereview.chromium.org/2639533003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java (right): https://codereview.chromium.org/2639533003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:34: nit: no need for the empty lines https://codereview.chromium.org/2639533003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:359: NewTabPageUma.recordUIUpdateResult(NewTabPageUma.UI_UPDATE_SUCCESS_1_OR_2_SEEN); why use 2 as an arbitrary cutoff? If this is to understand if it is the right value to pick, doesn't it make more sense to have an histogram with the actual number seen rather than just 3 buckets? https://codereview.chromium.org/2639533003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:368: NewTabPageUma.recordUIUpdateResult(NewTabPageUma.UI_UPDATE_SUCCESS_FIRST_TIME); if replaceExisting=false, we are doing a fetch more, is that intended that it reports FIRST_TIME?
Thanks, PTAL, again! https://codereview.chromium.org/2639533003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageUma.java (right): https://codereview.chromium.org/2639533003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageUma.java:102: UI_UPDATE_FAIL_ALL_SEEN, UI_UPDATE_FAIL_DISABLED, NUM_UI_UPDATE_RESULTS}) On 2017/01/19 18:46:09, dgn wrote: > I wouldn't put NUM_UI_UPDATE_RESULTS there, it's not a valid value for the enum, > right? Done. (I thought so but I wanted to minimize no. of comments' rounds so I sticked to the what was there for NTPLayoutResult ;)) https://codereview.chromium.org/2639533003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java (right): https://codereview.chromium.org/2639533003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:34: On 2017/01/19 18:46:09, dgn wrote: > nit: no need for the empty lines Done. https://codereview.chromium.org/2639533003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:359: NewTabPageUma.recordUIUpdateResult(NewTabPageUma.UI_UPDATE_SUCCESS_1_OR_2_SEEN); On 2017/01/19 18:46:10, dgn wrote: > why use 2 as an arbitrary cutoff? If this is to understand if it is the right > value to pick, doesn't it make more sense to have an histogram with the actual > number seen rather than just 3 buckets? Having the exact number is a bit tricky because of various sections and fetchMore(). I could list first 10 and then have one overflow bucket (is it what you suggest?) However, I believe that these 3 buckets will tell us all we need. Namely whether our assumptions are correct (that in most "update" cases users see none or very few suggestions and hence, the update is efficient). https://codereview.chromium.org/2639533003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:368: NewTabPageUma.recordUIUpdateResult(NewTabPageUma.UI_UPDATE_SUCCESS_FIRST_TIME); On 2017/01/19 18:46:10, dgn wrote: > if replaceExisting=false, we are doing a fetch more, is that intended that it > reports FIRST_TIME? Yes, it is intended to report to the same bucket. I renamed the bucket to make it clearer.
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...
lgtm https://codereview.chromium.org/2639533003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java (right): https://codereview.chromium.org/2639533003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:359: NewTabPageUma.recordUIUpdateResult(NewTabPageUma.UI_UPDATE_SUCCESS_1_OR_2_SEEN); On 2017/01/19 19:05:59, jkrcal wrote: > On 2017/01/19 18:46:10, dgn wrote: > > why use 2 as an arbitrary cutoff? If this is to understand if it is the right > > value to pick, doesn't it make more sense to have an histogram with the actual > > number seen rather than just 3 buckets? > > Having the exact number is a bit tricky because of various sections and > fetchMore(). > I could list first 10 and then have one overflow bucket (is it what you > suggest?) > > However, I believe that these 3 buckets will tell us all we need. Namely whether > our assumptions are correct (that in most "update" cases users see none or very > few suggestions and hence, the update is efficient). Acknowledged.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
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...
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...
Thanks, Nicolas! Robert, could you PTAL, again? https://codereview.chromium.org/2639533003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java (right): https://codereview.chromium.org/2639533003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:359: NewTabPageUma.recordUIUpdateResult(NewTabPageUma.UI_UPDATE_SUCCESS_1_OR_2_SEEN); On 2017/01/19 19:50:00, dgn wrote: > On 2017/01/19 19:05:59, jkrcal wrote: > > On 2017/01/19 18:46:10, dgn wrote: > > > why use 2 as an arbitrary cutoff? If this is to understand if it is the > right > > > value to pick, doesn't it make more sense to have an histogram with the > actual > > > number seen rather than just 3 buckets? > > > > Having the exact number is a bit tricky because of various sections and > > fetchMore(). > > I could list first 10 and then have one overflow bucket (is it what you > > suggest?) > > > > However, I believe that these 3 buckets will tell us all we need. Namely > whether > > our assumptions are correct (that in most "update" cases users see none or > very > > few suggestions and hence, the update is efficient). > > Acknowledged. In the end, I've expanded the enum a bit to get more precise info.
> Robert, could you PTAL, again? Sorry for the spam, I've forgotten you have already lgtm'ed :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
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...
The CQ bit was unchecked by jkrcal@chromium.org
The CQ bit was checked by jkrcal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rkaplow@chromium.org, dgn@chromium.org Link to the patchset: https://codereview.chromium.org/2639533003/#ps120001 (title: "Fix errors #2")
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": 120001, "attempt_start_ts": 1484868364569150,
"parent_rev": "ec6801495d0b7eb78429671cba5e70ca5704477f", "commit_rev":
"ede80b706285aa0052cf154d89c49f081a74b9a5"}
Message was sent while issue was closed.
Description was changed from ========== [Content suggestions] Report updates in the UI to UMA. Content suggestions on the Android New Tab Page can get updated if the user has not seen them yet. This CL adds a histogram to reason about how often are these updates successful. BUG=674163 ========== to ========== [Content suggestions] Report updates in the UI to UMA. Content suggestions on the Android New Tab Page can get updated if the user has not seen them yet. This CL adds a histogram to reason about how often are these updates successful. BUG=674163 Review-Url: https://codereview.chromium.org/2639533003 Cr-Commit-Position: refs/heads/master@{#444896} Committed: https://chromium.googlesource.com/chromium/src/+/ede80b706285aa0052cf154d89c4... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/ede80b706285aa0052cf154d89c4...
Message was sent while issue was closed.
https://codereview.chromium.org/2639533003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java (right): https://codereview.chromium.org/2639533003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java:359: NewTabPageUma.recordUIUpdateResult(NewTabPageUma.UI_UPDATE_SUCCESS_1_OR_2_SEEN); On 2017/01/19 20:25:38, jkrcal wrote: > On 2017/01/19 19:50:00, dgn wrote: > > On 2017/01/19 19:05:59, jkrcal wrote: > > > On 2017/01/19 18:46:10, dgn wrote: > > > > why use 2 as an arbitrary cutoff? If this is to understand if it is the > > right > > > > value to pick, doesn't it make more sense to have an histogram with the > > actual > > > > number seen rather than just 3 buckets? > > > > > > Having the exact number is a bit tricky because of various sections and > > > fetchMore(). > > > I could list first 10 and then have one overflow bucket (is it what you > > > suggest?) > > > > > > However, I believe that these 3 buckets will tell us all we need. Namely > > whether > > > our assumptions are correct (that in most "update" cases users see none or > > very > > > few suggestions and hence, the update is efficient). > > > > Acknowledged. > > In the end, I've expanded the enum a bit to get more precise info. Ugh... I'm not comfortable with this. The additional buckets make it looks like 2 orthogonal metrics bundled in one: how the update went (fail all seen | fail disabled | success replaced | success appended) + how many suggestions were kept in the success_replaced case. Aren't these 2 aspects going to be compared only among themselves? comparing update_fail_disabled with success_2_seen makes little sense IMO. So to clarify, I was seeing that more as a enum histogram for the status + a linear histogram for the number of items https://codereview.chromium.org/2639533003/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageUma.java (right): https://codereview.chromium.org/2639533003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageUma.java:110: /** Update successful, 1 previous content suggestion have been seen (and kept). */ s/have/has https://codereview.chromium.org/2639533003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageUma.java:114: /** Update successful, 2 previous content suggestions have been seen (and kept). */ s/2/3 https://codereview.chromium.org/2639533003/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageUma.java:116: /** Update successful, more than 2 previous content suggestions have been seen (and kept). */ s/2/3 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
