Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
164753005:
Add histograms for the Contextual Search field trial. (Closed)
|
Created:
6 years, 10 months ago by pedro (no code reviews) Modified:
6 years, 10 months ago CC:
chromium-reviews, Ilya Sherman, jar (doing other things), asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdd histograms for the Contextual Search field trial.
Related change: https://chrome-internal-review.googlesource.com/#/c/154947/
BUG=341765
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=252261
Patch Set 1 #
Total comments: 12
Patch Set 2 : Addressing Donn's and Mark's comments #Patch Set 3 : Fixing code comments #Patch Set 4 : Sync and rebase #
Messages
Total messages: 12 (0 generated)
Hey Mark, could you please review this CL? Let me know if you need some more context on the Contextual Search project.
LGTM! Just a few suggestions for improved wording of the histograms titles. https://chromiumcodereview.appspot.com/164753005/diff/1/tools/metrics/histogr... File tools/metrics/histograms/histograms.xml (right): https://chromiumcodereview.appspot.com/164753005/diff/1/tools/metrics/histogr... tools/metrics/histograms/histograms.xml:17685: + <summary>The type of action taken in the Opt-peek-card.</summary> Nit: Maybe change to "The type of action taken when the Opt-in card is peeking." https://chromiumcodereview.appspot.com/164753005/diff/1/tools/metrics/histogr... tools/metrics/histograms/histograms.xml:17690: + <summary>The type of action taken in the Peek-card.</summary> Nit: Maybe change to "The type of action taken when the Search card is peeking." https://chromiumcodereview.appspot.com/164753005/diff/1/tools/metrics/histogr... tools/metrics/histograms/histograms.xml:17694: + <summary>The type of tap action taken after opting in.</summary> Nit: Maybe change to "The type of tap action taken by opted-in users." https://chromiumcodereview.appspot.com/164753005/diff/1/tools/metrics/histogr... tools/metrics/histograms/histograms.xml:17699: + <summary>The type of tap action taken before opting in or out.</summary> Nit: Maybe change to "The type of tap action taken by undecided users."
This approach look fine to me. Some minor comments below. Have you already checked in the logging code itself? If not, common practice is to get the logging code and histograms.xml reviewed and submitted at the same time (in the same changelist). That way we can glance at the UMA calls to verify that they look like they're doing what they're supposed to do given the histogram descriptions. --mark https://codereview.chromium.org/164753005/diff/1/tools/metrics/histograms/his... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/164753005/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:17680: + <summary>The type of action taken in the Opt-card.</summary> If someone clicks Learn More, does the card disappear. If not, when the user returns to the card and clicks something else, does it get recorded? https://codereview.chromium.org/164753005/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:17694: + <summary>The type of tap action taken after opting in.</summary> If I take a bunch of times, all of which are ignored or invalid, are these all logged, is just the first tap action taken after opting in recorded?
Mark, thanks for the review. I haven't checked in the code that uses the histograms yet. I have the CL almost ready but it's a Chrome for Android CL so I cannot submit everything in a single CL. Do you want me to send you a link that CL? https://codereview.chromium.org/164753005/diff/1/tools/metrics/histograms/his... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/164753005/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:17680: + <summary>The type of action taken in the Opt-card.</summary> On 2014/02/14 16:21:33, Mark P wrote: > If someone clicks Learn More, does the card disappear. If not, when the user > returns to the card and clicks something else, does it get recorded? Good question. When the user clicks on the "learn more" we'll open a new tab in the foreground. The card will not disappear per se, but it won't be visible (a new tab will open on top of it). After closing the "learn more" tab, the user will see the Opt Card again and will have to perform an action to make the card "disappear" (he will have to choose to opt-in, opt-out, or close the opt card) and this action will be recorded. https://codereview.chromium.org/164753005/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:17685: + <summary>The type of action taken in the Opt-peek-card.</summary> On 2014/02/14 02:48:23, Donn Denman wrote: > Nit: Maybe change to "The type of action taken when the Opt-in card is peeking." Done. https://codereview.chromium.org/164753005/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:17690: + <summary>The type of action taken in the Peek-card.</summary> On 2014/02/14 02:48:23, Donn Denman wrote: > Nit: Maybe change to "The type of action taken when the Search card is peeking." Done. https://codereview.chromium.org/164753005/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:17694: + <summary>The type of tap action taken after opting in.</summary> On 2014/02/14 16:21:33, Mark P wrote: > If I take a bunch of times, all of which are ignored or invalid, are these all > logged, is just the first tap action taken after opting in recorded? All taps will be logged, not just the first one. https://codereview.chromium.org/164753005/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:17694: + <summary>The type of tap action taken after opting in.</summary> On 2014/02/14 02:48:23, Donn Denman wrote: > Nit: Maybe change to "The type of tap action taken by opted-in users." Done. https://codereview.chromium.org/164753005/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:17699: + <summary>The type of tap action taken before opting in or out.</summary> On 2014/02/14 02:48:23, Donn Denman wrote: > Nit: Maybe change to "The type of tap action taken by undecided users." Done.
Mark, I included a link in the description to the related change where I'm using the histograms.
histograms.xml lgtm Sorry for the delay. I lost this in my inbox. And thanks for understanding my originally comments--I re-read my earlier comments and had a little trouble understanding what I was trying to say (due to mistakenly substituting one word for another, like a typo only bigger). --mark
The CQ bit was checked by pedrosimonetti@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pedrosimonetti@chromium.org/164753005/...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pedrosimonetti@chromium.org/164753005/...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pedrosimonetti@chromium.org/164753005/...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pedrosimonetti@chromium.org/164753005/...
Message was sent while issue was closed.
Change committed as 252261 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
