Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2419013003:
[NTP Snippets] Report the share of the UI lang vs. the top non-UI lang (Closed)
Description[NTP Snippets] Report the share of the UI lang vs. the top non-UI lang
In requests to content suggestions server, we send the frequency of use
of the UI language and the top non-UI language.
This CL introduces reporting of the ratio of these two frequencies.
This allows to see how often (and how much) the UI language is
inappropriate.
BUG=656011
Committed: https://crrev.com/caa3c1cbea3a8de6603ad2f1e59d14224f4167f1
Cr-Commit-Position: refs/heads/master@{#425656}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Comments of Marc and Alexei #Patch Set 3 : Pretty-print histograms.xml #
Messages
Total messages: 21 (12 generated)
jkrcal@chromium.org changed reviewers: + asvitkine@chromium.org, treib@chromium.org
Marc, could you PTAL? Alexei, could you PTAL at histograms.xml?
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...
https://codereview.chromium.org/2419013003/diff/1/components/ntp_snippets/rem... File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2419013003/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:554: UMA_HISTOGRAM_COUNTS_1000( I think you probably want to use UMA_HISTOGRAM_PERCENT here. Otherwise, you'll get an exponentially scaled set of buckets.
https://codereview.chromium.org/2419013003/diff/1/components/ntp_snippets/rem... File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2419013003/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:550: DCHECK(params->other_top_language.frequency > 0) DCHECK_GT https://codereview.chromium.org/2419013003/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:553: params->ui_language.frequency / params->other_top_language.frequency; Should this be "ui_freq / (ui_freq + other_freq)" ?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks to both of you! PTAL, again. https://codereview.chromium.org/2419013003/diff/1/components/ntp_snippets/rem... File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2419013003/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:550: DCHECK(params->other_top_language.frequency > 0) On 2016/10/14 15:32:56, Marc Treib wrote: > DCHECK_GT Done. https://codereview.chromium.org/2419013003/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:553: params->ui_language.frequency / params->other_top_language.frequency; On 2016/10/14 15:32:56, Marc Treib wrote: > Should this be "ui_freq / (ui_freq + other_freq)" ? Well, I meant it as it was. Your suggestion actually makes the data more standard an easier to interpret :) Done. https://codereview.chromium.org/2419013003/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:554: UMA_HISTOGRAM_COUNTS_1000( On 2016/10/14 15:30:15, Alexei Svitkine (slow) wrote: > I think you probably want to use UMA_HISTOGRAM_PERCENT here. Otherwise, you'll > get an exponentially scaled set of buckets. This is what I wanted because the result was not supposed to be on the range of 0-100. Together with the change of Marc above, I can make it PERCENT. Which makes more sense.
lgtm
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
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, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2419013003/#ps40001 (title: "Pretty-print histograms.xml")
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.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [NTP Snippets] Report the share of the UI lang vs. the top non-UI lang In requests to content suggestions server, we send the frequency of use of the UI language and the top non-UI language. This CL introduces reporting of the ratio of these two frequencies. This allows to see how often (and how much) the UI language is inappropriate. BUG=656011 ========== to ========== [NTP Snippets] Report the share of the UI lang vs. the top non-UI lang In requests to content suggestions server, we send the frequency of use of the UI language and the top non-UI language. This CL introduces reporting of the ratio of these two frequencies. This allows to see how often (and how much) the UI language is inappropriate. BUG=656011 Committed: https://crrev.com/caa3c1cbea3a8de6603ad2f1e59d14224f4167f1 Cr-Commit-Position: refs/heads/master@{#425656} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/caa3c1cbea3a8de6603ad2f1e59d14224f4167f1 Cr-Commit-Position: refs/heads/master@{#425656} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
