Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1996473002:
Track age and score of snippets that are clicked (along with position). (Closed)
|
Created:
4 years, 7 months ago by jkrcal Modified:
4 years, 7 months ago CC:
chromium-reviews, mvanouwerkerk+watch_chromium.org, mcwilliams+watch_chromium.org, asvitkine+watch_chromium.org, dgn+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionTrack age and score of snippets that are clicked (along with position).
In order to see how CRT differs based on freshness and on the overall
score of the snippets, this CL tracks these values for clicks.
A future CL will track the same values for impressions.
BUG=608365
Committed: https://crrev.com/834c35c0f63a277273b5e88d6353082cbb3e7c22
Cr-Commit-Position: refs/heads/master@{#395835}
Patch Set 1 #Patch Set 2 : Minor fixes #Patch Set 3 : Minor polish #
Total comments: 6
Patch Set 4 : After code review #1 #Patch Set 5 : Documenting histograms suffixes #
Total comments: 12
Patch Set 6 : After code review #2 #
Total comments: 6
Patch Set 7 : After code review #3 #
Total comments: 1
Messages
Total messages: 33 (10 generated)
jkrcal@chromium.org changed reviewers: + asvitkine@chromium.org, tedchoc@chromium.org, treib@chromium.org
Ted: PTAL at the java files. Marc: PTAL at the cc bridge file. Alexei: PTAL at histograms.xml.
Bridge LGTM. I also took the liberty of looking at the java files :) https://codereview.chromium.org/1996473002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/1996473002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:51: private long mTimestamp; Please give this a better name - timestamp of what? Also, since it's a generic type rather than some sort of Time type: What units? https://codereview.chromium.org/1996473002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:109: String suffix = "@" + position; The histograms.xml file has a notion of suffixes, you should probably use that.
Thanks, Marc. I've reflected your comments. https://codereview.chromium.org/1996473002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/1996473002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:51: private long mTimestamp; On 2016/05/19 08:18:40, Marc Treib wrote: > Please give this a better name - timestamp of what? > Also, since it's a generic type rather than some sort of Time type: What units? Done. A bit verbose now. I hope it fits in the java style. https://codereview.chromium.org/1996473002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:109: String suffix = "@" + position; On 2016/05/19 08:18:40, Marc Treib wrote: > The histograms.xml file has a notion of suffixes, you should probably use that. Done. Did not notice this, thanks!
https://codereview.chromium.org/1996473002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/1996473002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:51: private long mTimestamp; On 2016/05/19 15:47:19, jkrcal wrote: > On 2016/05/19 08:18:40, Marc Treib wrote: > > Please give this a better name - timestamp of what? > > Also, since it's a generic type rather than some sort of Time type: What > units? > > Done. A bit verbose now. I hope it fits in the java style. Ah, I just noticed that you had just copied the name from SnippetArticle, so it wasn't even "your fault". That should really have a better name as well :D (but you don't need to change that in this CL). https://codereview.chromium.org/1996473002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/1996473002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:109: String suffix = "_" + position; I think it should be "." rather than "_"?
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/patch-status/1996473002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1996473002/80001
https://codereview.chromium.org/1996473002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/1996473002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:51: private long mTimestamp; On 2016/05/19 16:07:09, Marc Treib wrote: > On 2016/05/19 15:47:19, jkrcal wrote: > > On 2016/05/19 08:18:40, Marc Treib wrote: > > > Please give this a better name - timestamp of what? > > > Also, since it's a generic type rather than some sort of Time type: What > > units? > > > > Done. A bit verbose now. I hope it fits in the java style. > > Ah, I just noticed that you had just copied the name from SnippetArticle, so it > wasn't even "your fault". That should really have a better name as well :D (but > you don't need to change that in this CL). Ok. https://codereview.chromium.org/1996473002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/1996473002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:109: String suffix = "_" + position; On 2016/05/19 16:07:10, Marc Treib wrote: > I think it should be "." rather than "_"? histograms.xml:41 supports my version. The documentation may be outdated, though. I need to test it properly.
https://codereview.chromium.org/1996473002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/1996473002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:109: String suffix = "_" + position; On 2016/05/19 16:07:10, Marc Treib wrote: > I think it should be "." rather than "_"? This is correct, but you can also use "." if you prefer by specifying separator="." in the xml. https://codereview.chromium.org/1996473002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1996473002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:33628: +<histogram name="NewTabPage.Snippets.CardClickedAge"> Nit: Add units="minutes" https://codereview.chromium.org/1996473002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:33632: + the host website of the content. The age is measured in minutes from the This says you measure it in minutes, but your code is recording milliseconds. Which is it? If you actually prefer to record minutes, you can just use a custom counts histogram and record minutes yourself, instead of a times histogram.
https://codereview.chromium.org/1996473002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/1996473002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:104: long age = System.currentTimeMillis() - mPublishTimestampMilliseconds; FWIW, currentTimeMillis states to not use it for time deltas. """ This method shouldn't be used for measuring timeouts or other elapsed time measurements, as changing the system time can affect the results. Use nanoTime() for that. """ I think you are probably ok, but you might want to use a Math.max(age, 1) to ensure you're getting something sane (and to handle any wonky user behavior). Unless recordCustomTimesHistogram rejects numbers outside of its valid range (no idea honestly). https://codereview.chromium.org/1996473002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:111: recordScore("NewTabPage.Snippets.CardClickedScore" + suffix, mScore); do you want to record it for all buckets or just the narrowest range? i.e. should you have a break here? So if you click item 0, then as written you'll record entries in NewTabPage.Snippets.CardClickedAge_0 NewTabPage.Snippets.CardClickedAge_2 NewTabPage.Snippets.CardClickedAge_4 Not sure what exactly that gains you or what you're able to tell. I would have expected that you'd want to see how the age and score affect the position buckets.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
Thanks for all the comments! PTAL again. https://codereview.chromium.org/1996473002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/1996473002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:104: long age = System.currentTimeMillis() - mPublishTimestampMilliseconds; On 2016/05/19 16:52:50, Ted C wrote: > FWIW, currentTimeMillis states to not use it for time deltas. > > """ > This method shouldn't be used for measuring timeouts or other elapsed time > measurements, as changing the system time can affect the results. Use nanoTime() > for that. > """ > > I think you are probably ok, but you might want to use a Math.max(age, 1) to > ensure you're getting something sane (and to handle any wonky user behavior). > Unless recordCustomTimesHistogram rejects numbers outside of its valid range (no > idea honestly). Heh, I am not used yet to look at android documentation. This line is missing in Oracle's documentation :) Anyway, nanoTime() is no help here as I am comparing to an externally provided timestamp. Good point anyway. I will not record negative values as they provide no insight. https://codereview.chromium.org/1996473002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:109: String suffix = "_" + position; On 2016/05/19 16:42:43, Alexei Svitkine wrote: > On 2016/05/19 16:07:10, Marc Treib wrote: > > I think it should be "." rather than "_"? > > This is correct, but you can also use "." if you prefer by specifying > separator="." in the xml. Thanks for the explanation! (I am fine with "_") https://codereview.chromium.org/1996473002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:111: recordScore("NewTabPage.Snippets.CardClickedScore" + suffix, mScore); On 2016/05/19 16:52:50, Ted C wrote: > do you want to record it for all buckets or just the narrowest range? i.e. > should you have a break here? > > So if you click item 0, then as written you'll record entries in > > NewTabPage.Snippets.CardClickedAge_0 > NewTabPage.Snippets.CardClickedAge_2 > NewTabPage.Snippets.CardClickedAge_4 > > Not sure what exactly that gains you or what you're able to tell. I would have > expected that you'd want to see how the age and score affect the position > buckets. Well, you are probably right that your suggested way makes it easier to interpret the data. I've changed it. https://codereview.chromium.org/1996473002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1996473002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:33628: +<histogram name="NewTabPage.Snippets.CardClickedAge"> On 2016/05/19 16:42:43, Alexei Svitkine wrote: > Nit: Add units="minutes" Done (milliseconds). https://codereview.chromium.org/1996473002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:33632: + the host website of the content. The age is measured in minutes from the On 2016/05/19 16:42:43, Alexei Svitkine wrote: > This says you measure it in minutes, but your code is recording milliseconds. > Which is it? > > If you actually prefer to record minutes, you can just use a custom counts > histogram and record minutes yourself, instead of a times histogram. Corrected.
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/patch-status/1996473002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1996473002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/1996473002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/1996473002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:173: long maxAge = TimeUnit.HOURS.toMillis(72); Nit: Add a comment that if this were changed, the histogram should be renamed since it will change the shape of buckets. https://codereview.chromium.org/1996473002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:174: age = Math.min(age, maxAge); Nit: This is not necessary - it's done already internally by the API. https://codereview.chromium.org/1996473002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1996473002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:33638: +<histogram name="NewTabPage.Snippets.CardClickedScore"> Nit: units="score"
lgtm
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/patch-status/1996473002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1996473002/120001
Thanks, everybody! Alexei: I've actually changed my mind and switched back to minutes as the unit to track the age in the histogram. I am sorry for the back-and-forths! I use recordCustomCountHistogram as you suggested. I am wondering about |max| and |numBuckets|: does custom count histogram use exponentially-sized buckets or linearly-sized ones? https://codereview.chromium.org/1996473002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java (right): https://codereview.chromium.org/1996473002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:173: long maxAge = TimeUnit.HOURS.toMillis(72); On 2016/05/20 14:47:58, Alexei Svitkine (OOO til Tues) wrote: > Nit: Add a comment that if this were changed, the histogram should be renamed > since it will change the shape of buckets. Done. https://codereview.chromium.org/1996473002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:174: age = Math.min(age, maxAge); On 2016/05/20 14:47:58, Alexei Svitkine (OOO til Tues) wrote: > Nit: This is not necessary - it's done already internally by the API. Done. https://codereview.chromium.org/1996473002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1996473002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:33638: +<histogram name="NewTabPage.Snippets.CardClickedScore"> On 2016/05/20 14:47:58, Alexei Svitkine (OOO til Tues) wrote: > Nit: units="score" Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/1996473002/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1996473002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:33671: +<histogram name="NewTabPage.Snippets.CardClickedAge" units="min"> Nit: minutes
Thanks! Alexei, let me repeat the quick question: does custom count histogram use exponentially-sized buckets or linearly-sized ones? Jan On Tue, May 24, 2016 at 7:22 PM <asvitkine@chromium.org> wrote: > lgtm > > > > > > https://codereview.chromium.org/1996473002/diff/120001/tools/metrics/histogra... > File tools/metrics/histograms/histograms.xml (right): > > > https://codereview.chromium.org/1996473002/diff/120001/tools/metrics/histogra... > tools/metrics/histograms/histograms.xml:33671: +<histogram > name="NewTabPage.Snippets.CardClickedAge" units="min"> > Nit: minutes > > https://codereview.chromium.org/1996473002/ > -- 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.
Sorry for missing the question. Custom count uses exponential buckets. On Tue, May 24, 2016 at 3:04 PM, Jan Krčál <jkrcal@google.com> wrote: > Thanks! > > Alexei, let me repeat the quick question: > does custom count histogram use exponentially-sized buckets or linearly-sized > ones? > > Jan > > On Tue, May 24, 2016 at 7:22 PM <asvitkine@chromium.org> wrote: > >> lgtm >> >> >> >> >> >> https://codereview.chromium.org/1996473002/diff/120001/tools/metrics/histogra... >> File tools/metrics/histograms/histograms.xml (right): >> >> >> https://codereview.chromium.org/1996473002/diff/120001/tools/metrics/histogra... >> tools/metrics/histograms/histograms.xml:33671: +<histogram >> name="NewTabPage.Snippets.CardClickedAge" units="min"> >> Nit: minutes >> >> https://codereview.chromium.org/1996473002/ >> > -- 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.
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, tedchoc@chromium.org Link to the patchset: https://codereview.chromium.org/1996473002/#ps120001 (title: "After code review #3")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1996473002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1996473002/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Track age and score of snippets that are clicked (along with position). In order to see how CRT differs based on freshness and on the overall score of the snippets, this CL tracks these values for clicks. A future CL will track the same values for impressions. BUG=608365 ========== to ========== Track age and score of snippets that are clicked (along with position). In order to see how CRT differs based on freshness and on the overall score of the snippets, this CL tracks these values for clicks. A future CL will track the same values for impressions. BUG=608365 Committed: https://crrev.com/834c35c0f63a277273b5e88d6353082cbb3e7c22 Cr-Commit-Position: refs/heads/master@{#395835} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/834c35c0f63a277273b5e88d6353082cbb3e7c22 Cr-Commit-Position: refs/heads/master@{#395835} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
