Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2509663002:
Add a histogram for recording content suggestions local usage time stats. (Closed)
|
Created:
4 years, 1 month ago by markusheintz_ Modified:
4 years, 1 month ago CC:
chromium-reviews, asvitkine+watch_chromium.org, ntp-dev+reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a histogram for recording content suggestions local usage time stats.
BUG=665873
Committed: https://crrev.com/43da4b6d12166dc10f397b2f09ace9fd4a7bb33e
Cr-Commit-Position: refs/heads/master@{#432662}
Patch Set 1 #Patch Set 2 : Fix a comment #
Total comments: 19
Patch Set 3 : Address comments #Patch Set 4 : Fix typo #Patch Set 5 : Address comments #
Total comments: 8
Patch Set 6 : Address comments #
Total comments: 6
Patch Set 7 : Updated historgram description #
Messages
Total messages: 33 (12 generated)
Fix a comment
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 ========== Add a histogram for recording content suggestions local usage time stats. BUG= ========== to ========== Add a histogram for recording content suggestions local usage time stats. BUG=665873 ==========
markusheintz@chromium.org changed reviewers: + treib@chromium.org
Marc could you take a look at this CL ? Thanks
https://codereview.chromium.org/2509663002/diff/20001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_metrics.cc (right): https://codereview.chromium.org/2509663002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_metrics.cc:166: // bucket is increased. Mention that "usage" means "impression" rather than "click"? https://codereview.chromium.org/2509663002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_metrics.cc:168: const int kBucketSize = 20; // min nit: kBucketSizeMins and remove the comment? https://codereview.chromium.org/2509663002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_metrics.cc:173: now.LocalExplode(&now_exploded); nit: You could just write base::Time::Now().LocalExplode(...) https://codereview.chromium.org/2509663002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_metrics.cc:174: size_t bucket = (now_exploded.hour * 60 + now_exploded.minute) / kBucketSize; Why size_t rather than int? https://codereview.chromium.org/2509663002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_metrics.cc:179: kNumBuckets); nit: run "git cl format" https://codereview.chromium.org/2509663002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_metrics.cc:212: // When the first suggestion of all snippets is shown, then we count this as a "When the first of all suggestions is shown, ..."? Let's try not to use "snippet" anymore. https://codereview.chromium.org/2509663002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_metrics.cc:214: // TODO(markusheintz): Use position 0 once http://crbug.com/664539 is fixed. It'll be fixed very soon, see https://codereview.chromium.org/2504963002, so please use 0 already :) https://codereview.chromium.org/2509663002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2509663002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:37617: + into 20 min buckest. The local usage is recorded by increasing the count of s/buckest/buckets/ https://codereview.chromium.org/2509663002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:37618: + the corresponding bucket. I think this description could be a bit clearer. How about: Android: The time of day (in the local timezone) when the user used content suggestions. Recorded when the user scrolls below the fold to look at suggestions, at most once per NTP.
https://codereview.chromium.org/2509663002/diff/20001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_metrics.cc (right): https://codereview.chromium.org/2509663002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_metrics.cc:166: // bucket is increased. On 2016/11/16 14:14:29, Marc Treib wrote: > Mention that "usage" means "impression" rather than "click"? It will be more that just the impression, i'll also count how often an article was opened and other usages. I'll add counting article open events. https://codereview.chromium.org/2509663002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_metrics.cc:168: const int kBucketSize = 20; // min On 2016/11/16 14:14:29, Marc Treib wrote: > nit: kBucketSizeMins and remove the comment? Done. https://codereview.chromium.org/2509663002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_metrics.cc:173: now.LocalExplode(&now_exploded); On 2016/11/16 14:14:29, Marc Treib wrote: > nit: You could just write > base::Time::Now().LocalExplode(...) Done. https://codereview.chromium.org/2509663002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_metrics.cc:174: size_t bucket = (now_exploded.hour * 60 + now_exploded.minute) / kBucketSize; On 2016/11/16 14:14:29, Marc Treib wrote: > Why size_t rather than int? bucket size can't be negative. https://codereview.chromium.org/2509663002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_metrics.cc:179: kNumBuckets); On 2016/11/16 14:14:29, Marc Treib wrote: > nit: run "git cl format" Done. https://codereview.chromium.org/2509663002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_metrics.cc:212: // When the first suggestion of all snippets is shown, then we count this as a On 2016/11/16 14:14:29, Marc Treib wrote: > "When the first of all suggestions is shown, ..."? Let's try not to use > "snippet" anymore. Done. https://codereview.chromium.org/2509663002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_metrics.cc:214: // TODO(markusheintz): Use position 0 once http://crbug.com/664539 is fixed. On 2016/11/16 14:14:29, Marc Treib wrote: > It'll be fixed very soon, see https://codereview.chromium.org/2504963002, so > please use 0 already :) Done.
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...
Fix typo
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...
https://codereview.chromium.org/2509663002/diff/20001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_metrics.cc (right): https://codereview.chromium.org/2509663002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_metrics.cc:166: // bucket is increased. On 2016/11/16 14:52:13, markusheintz_ wrote: > On 2016/11/16 14:14:29, Marc Treib wrote: > > Mention that "usage" means "impression" rather than "click"? > > It will be more that just the impression, i'll also count how often an article > was opened and other usages. I'll add counting article open events. Then all the more important to define what we consider a "usage" :) Is impression + click all that will be tracked in this histogram? If not, it'd probably be best to add everything else in this CL too - it's considered bad practice to change the definition of a histogram after it's been introduced. https://codereview.chromium.org/2509663002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_metrics.cc:174: size_t bucket = (now_exploded.hour * 60 + now_exploded.minute) / kBucketSize; On 2016/11/16 14:52:13, markusheintz_ wrote: > On 2016/11/16 14:14:29, Marc Treib wrote: > > Why size_t rather than int? > > bucket size can't be negative. Nothing here can be negative, yet everything else is an int. :) Per the style guide, we don't use unsigned types simply because the value shouldn't be negative: https://google.github.io/styleguide/cppguide.html#Integer_Types https://codereview.chromium.org/2509663002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2509663002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:37618: + the corresponding bucket. On 2016/11/16 14:14:29, Marc Treib wrote: > I think this description could be a bit clearer. How about: > > Android: The time of day (in the local timezone) when the user used content > suggestions. Recorded when the user scrolls below the fold to look at > suggestions, at most once per NTP. Still open; also needs to be updated to also mention clicks.
One more thing I just thought of: Should all this be for Articles only, or for all categories, including e.g. bookmarks?
Address comments
On 2016/11/16 15:30:35, Marc Treib wrote: > One more thing I just thought of: Should all this be for Articles only, or for > all categories, including e.g. bookmarks? I though maybe it's good to collect the overall usage time. But at the moment I'm actually only interested in the ARTICLES category. I changed the code
markusheintz@chromium.org changed reviewers: + holte@chromium.org
The comment on histograms.xml (in patch set 2) still isn't addressed. https://codereview.chromium.org/2509663002/diff/80001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_metrics.cc (right): https://codereview.chromium.org/2509663002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_metrics.cc:55: KnownCategories GetKnownCategory(Category category) { Per the comments below, this isn't necessary. (I wouldn't like to pass around a KnownCategories instance that isn't necessarily one of the actual values...) https://codereview.chromium.org/2509663002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_metrics.cc:214: // When the first of all suggestions is shown, then we count this as a nit: Now it's not the first of all suggestions anymore :) https://codereview.chromium.org/2509663002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_metrics.cc:216: if (GetKnownCategory(category) == KnownCategories::ARTICLES && category.IsKnownCategory(KnownCategories::ARTICLES) https://codereview.chromium.org/2509663002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_metrics.cc:245: if (GetKnownCategory(category) == KnownCategories::ARTICLES) { Also here: category.IsKnownCategory(KnownCategories::ARTICLES)
Address comments
histogram comment addressed https://codereview.chromium.org/2509663002/diff/80001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_metrics.cc (right): https://codereview.chromium.org/2509663002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_metrics.cc:55: KnownCategories GetKnownCategory(Category category) { On 2016/11/16 16:00:55, Marc Treib wrote: > Per the comments below, this isn't necessary. (I wouldn't like to pass around a > KnownCategories instance that isn't necessarily one of the actual values...) Done. https://codereview.chromium.org/2509663002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_metrics.cc:214: // When the first of all suggestions is shown, then we count this as a On 2016/11/16 16:00:55, Marc Treib wrote: > nit: Now it's not the first of all suggestions anymore :) Done. https://codereview.chromium.org/2509663002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_metrics.cc:216: if (GetKnownCategory(category) == KnownCategories::ARTICLES && On 2016/11/16 16:00:55, Marc Treib wrote: > category.IsKnownCategory(KnownCategories::ARTICLES) Done. https://codereview.chromium.org/2509663002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_metrics.cc:245: if (GetKnownCategory(category) == KnownCategories::ARTICLES) { On 2016/11/16 16:00:55, Marc Treib wrote: > Also here: > category.IsKnownCategory(KnownCategories::ARTICLES) Done.
On 2016/11/16 16:09:42, markusheintz_ wrote: > histogram comment addressed > > https://codereview.chromium.org/2509663002/diff/80001/components/ntp_snippets... > File components/ntp_snippets/content_suggestions_metrics.cc (right): > > https://codereview.chromium.org/2509663002/diff/80001/components/ntp_snippets... > components/ntp_snippets/content_suggestions_metrics.cc:55: KnownCategories > GetKnownCategory(Category category) { > On 2016/11/16 16:00:55, Marc Treib wrote: > > Per the comments below, this isn't necessary. (I wouldn't like to pass around > a > > KnownCategories instance that isn't necessarily one of the actual values...) > > Done. > > https://codereview.chromium.org/2509663002/diff/80001/components/ntp_snippets... > components/ntp_snippets/content_suggestions_metrics.cc:214: // When the first of > all suggestions is shown, then we count this as a > On 2016/11/16 16:00:55, Marc Treib wrote: > > nit: Now it's not the first of all suggestions anymore :) > > Done. > > https://codereview.chromium.org/2509663002/diff/80001/components/ntp_snippets... > components/ntp_snippets/content_suggestions_metrics.cc:216: if > (GetKnownCategory(category) == KnownCategories::ARTICLES && > On 2016/11/16 16:00:55, Marc Treib wrote: > > category.IsKnownCategory(KnownCategories::ARTICLES) > > Done. > > https://codereview.chromium.org/2509663002/diff/80001/components/ntp_snippets... > components/ntp_snippets/content_suggestions_metrics.cc:245: if > (GetKnownCategory(category) == KnownCategories::ARTICLES) { > On 2016/11/16 16:00:55, Marc Treib wrote: > > Also here: > > category.IsKnownCategory(KnownCategories::ARTICLES) > > Done. updated histogram desc
LGTM with some more nits https://codereview.chromium.org/2509663002/diff/100001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_metrics.cc (right): https://codereview.chromium.org/2509663002/diff/100001/components/ntp_snippet... components/ntp_snippets/content_suggestions_metrics.cc:44: "NewTabPage.ContentSuggestions.UsageTimeLocal"; Maybe .ArticlesUsageTimeLocal since it's for articles only now? https://codereview.chromium.org/2509663002/diff/100001/components/ntp_snippet... components/ntp_snippets/content_suggestions_metrics.cc:178: LOG(ERROR) << " ****************** Zine Usage"; Remove this before landing :) https://codereview.chromium.org/2509663002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2509663002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:37618: + suggestions, at most once per NTP. Not accurate anymore, since it's also recorded for clicks.
https://codereview.chromium.org/2509663002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2509663002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:37618: + suggestions, at most once per NTP. On 2016/11/16 16:14:02, Marc Treib wrote: > Not accurate anymore, since it's also recorded for clicks. Ah, already addressed now :)
https://codereview.chromium.org/2509663002/diff/100001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_metrics.cc (right): https://codereview.chromium.org/2509663002/diff/100001/components/ntp_snippet... components/ntp_snippets/content_suggestions_metrics.cc:44: "NewTabPage.ContentSuggestions.UsageTimeLocal"; On 2016/11/16 16:14:02, Marc Treib wrote: > Maybe .ArticlesUsageTimeLocal since it's for articles only now? Done. https://codereview.chromium.org/2509663002/diff/100001/components/ntp_snippet... components/ntp_snippets/content_suggestions_metrics.cc:178: LOG(ERROR) << " ****************** Zine Usage"; On 2016/11/16 16:14:02, Marc Treib wrote: > Remove this before landing :) Done. sry that line sneaked back in.
Steven could you take a look at the histograms.xml changes? Thanks
lgtm
The CQ bit was checked by markusheintz@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add a histogram for recording content suggestions local usage time stats. BUG=665873 ========== to ========== Add a histogram for recording content suggestions local usage time stats. BUG=665873 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Add a histogram for recording content suggestions local usage time stats. BUG=665873 ========== to ========== Add a histogram for recording content suggestions local usage time stats. BUG=665873 Committed: https://crrev.com/43da4b6d12166dc10f397b2f09ace9fd4a7bb33e Cr-Commit-Position: refs/heads/master@{#432662} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/43da4b6d12166dc10f397b2f09ace9fd4a7bb33e Cr-Commit-Position: refs/heads/master@{#432662} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
