Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2668783002:
Added histograms for Custom Feedback UI (Closed)
DescriptionAdded histograms for Custom Feedback UI
Custom feedback was added to the internal build due to First Party APIs.
However, there is very little reason to keep what we track off of a
public commit. As a result, the logs are kept here instead.
BUG=676476
Review-Url: https://codereview.chromium.org/2668783002
Cr-Commit-Position: refs/heads/master@{#447815}
Committed: https://chromium.googlesource.com/chromium/src/+/c0dfdbfdb6f8c3c5b2dd463691ef7b00508d5b5e
Patch Set 1 #
Total comments: 7
Patch Set 2 : Fixed based of mpearson's comments #
Total comments: 4
Patch Set 3 : Clarified first option by deleting fluff #
Total comments: 2
Messages
Total messages: 23 (13 generated)
The CQ bit was checked by jwanda@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: This issue passed the CQ dry run.
Hello Mark, I'm currently working on Custom Feedback UI for Chrome. Right now this involves code that is mostly private but the metrics themselves don't have to since it's only hidden due to some first party APIs. When you have the chance, could you look at this to see if it is up to spec?
Description was changed from ========== Added histograms for Custom Feedback UI Custom feedback was added to the internal build due to First Party APIs. However, there is very little reason to keep what we track off of a public commit. As a result, the logs are kept here instead. BUG=676476 ========== to ========== Added histograms for Custom Feedback UI Custom feedback was added to the internal build due to First Party APIs. However, there is very little reason to keep what we track off of a public commit. As a result, the logs are kept here instead. BUG=676476 ==========
jwanda@chromium.org changed reviewers: + mpearson@chromium.org
On 2017/01/31 23:17:30, JJ wrote: > Hello Mark, > > I'm currently working on Custom Feedback UI for Chrome. Right now this involves > code that is mostly private but the metrics themselves don't have to since it's > only hidden due to some first party APIs. When you have the chance, could you > look at this to see if it is up to spec? Can you please put a link to the code that records these metrics in the changelist description? If it turns out I have access to it, I'd like to review the usage of the metrics logging calls. I'm especially going to look for the proper labels by the enum declarations https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo... I'll also look to see how you construct the enum to emit to the Android.CustomFeedback.Option histogram. thanks, mark https://codereview.chromium.org/2668783002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2668783002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:454: + This is recorded before the user submits feedback. the PSD. the PSD? https://codereview.chromium.org/2668783002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:454: + This is recorded before the user submits feedback. the PSD. This second sentence somewhat implies this is only recorded once, right before the user submits feedback. The first sentence implies it's recorded every time the user selects a button. Which is it? https://codereview.chromium.org/2668783002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:458: +<histogram name="Android.CustomFeedback.Option" enum="AndroidFeedbackOption"> Option is vague and also doesn't seem appropriate here. Have you considered something like CategoryDetails? https://codereview.chromium.org/2668783002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:461: + Recorded when a user selects an option related to their problem. please say somewhere that this is in the custom feedback UI. I know it's redundant with the histogram name, but it still helps if this description reads similar to the above one.
So in relation to the internal cl, here it is (just about to upload the cl to update the name) https://chrome-internal-review.googlesource.com/c/318785/1 https://codereview.chromium.org/2668783002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2668783002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:454: + This is recorded before the user submits feedback. the PSD. On 2017/02/01 19:53:53, Mark P wrote: > This second sentence somewhat implies this is only recorded once, right before > the user submits feedback. The first sentence implies it's recorded every time > the user selects a button. Which is it? Sorry, I can see the confusion, I've changed it up. Reading the markdown it implies that this purpose is to talk about the recording, should I also put why we need it within the summary? It would be something like "We use this data and Android.CustomFeedback.Option to determine dropoff rate and what categories users press before coming to a final decision." https://codereview.chromium.org/2668783002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:458: +<histogram name="Android.CustomFeedback.Option" enum="AndroidFeedbackOption"> On 2017/02/01 19:53:53, Mark P wrote: > Option is vague and also doesn't seem appropriate here. Have you considered > something like CategoryDetails? That's a fantastic name. Thank you so much. https://codereview.chromium.org/2668783002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:461: + Recorded when a user selects an option related to their problem. On 2017/02/01 19:53:53, Mark P wrote: > please say somewhere that this is in the custom feedback UI. I know it's > redundant with the histogram name, but it still helps if this description reads > similar to the above one. Done.
https://codereview.chromium.org/2668783002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2668783002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:454: + This is recorded regardless if the user submits feedback. If the user does On 2017/02/01 19:53:53, Mark P wrote: > This second sentence somewhat implies this is only recorded once, right before > the user submits feedback. The first sentence implies it's recorded every time > the user selects a button. Which is it? https://codereview.chromium.org/2668783002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:456: + within their feedback. On 2017/02/01 20:16:56, JJ wrote: > On 2017/02/01 19:53:53, Mark P wrote: > > This second sentence somewhat implies this is only recorded once, right before > > the user submits feedback. The first sentence implies it's recorded every > time > > the user selects a button. Which is it? > > Sorry, I can see the confusion, I've changed it up. > Reading the markdown it implies that this purpose is to talk about the > recording, should I also put why we need it within the summary? It would be > something like "We use this data and Android.CustomFeedback.Option to determine > dropoff rate and what categories users press before coming to a final decision." Justification / purpose is not necessary. If you're willing to make the justification public and think it'd be helpful for other people reading the histogram, then it's fine to add it. The key thing to talk about is have a description that's understandable to everyone and clear about what it's recording / when it's emitted https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo... You seem to have missed my other comment on this histogram. I'll paste it into this patchset.
Alright fixed the previous comments. Hopefully it looks better now. https://codereview.chromium.org/2668783002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2668783002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:454: + This is recorded regardless if the user submits feedback. If the user does On 2017/02/02 00:28:39, Mark P (away thru Feb 5 2017) wrote: > On 2017/02/01 19:53:53, Mark P wrote: > > This second sentence somewhat implies this is only recorded once, right before > > the user submits feedback. The first sentence implies it's recorded every > time > > the user selects a button. Which is it? > Hm, I thought the change would clarify this. So it's recorded every time. To clarify it this time I deleted the two sentences as they seem to be fluff and don't talk specifically about the recording and it ended up confusing more than helping. https://codereview.chromium.org/2668783002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:456: + within their feedback. On 2017/02/02 00:28:39, Mark P (away thru Feb 5 2017) wrote: > On 2017/02/01 20:16:56, JJ wrote: > > On 2017/02/01 19:53:53, Mark P wrote: > > > This second sentence somewhat implies this is only recorded once, right > before > > > the user submits feedback. The first sentence implies it's recorded every > > time > > > the user selects a button. Which is it? > > > > Sorry, I can see the confusion, I've changed it up. > > Reading the markdown it implies that this purpose is to talk about the > > recording, should I also put why we need it within the summary? It would be > > something like "We use this data and Android.CustomFeedback.Option to > determine > > dropoff rate and what categories users press before coming to a final > decision." > > Justification / purpose is not necessary. If you're willing to make the > justification public and think it'd be helpful for other people reading the > histogram, then it's fine to add it. > > The key thing to talk about is have a description that's understandable to > everyone and clear about what it's recording / when it's emitted > https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo... > > You seem to have missed my other comment on this histogram. I'll paste it into > this patchset. Alright, thanks. Just making sure. I'm going to opt out of measuring it.
possible lgtm with caveat below https://codereview.chromium.org/2668783002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2668783002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:463: + Custom Feedback UI. Is it possible they select an option without selecting a category first? If not, then this lgtm. If so, then what do you end up emitting here?
On 2017/02/02 05:07:50, Mark P (away thru Feb 5 2017) wrote: > possible lgtm with caveat below > > https://codereview.chromium.org/2668783002/diff/40001/tools/metrics/histogram... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/2668783002/diff/40001/tools/metrics/histogram... > tools/metrics/histograms/histograms.xml:463: + Custom Feedback UI. > Is it possible they select an option without selecting a category first? > If not, then this lgtm. > If so, then what do you end up emitting here? Caveat is addressed! Thanks so much for the help! First time doing anything with histograms so I appreciate the time you took to help out and clarify all this information.
The CQ bit was checked by jwanda@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: This issue passed the CQ dry run.
The CQ bit was checked by jwanda@chromium.org
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": 40001, "attempt_start_ts": 1486062701283060,
"parent_rev": "9756c20941139177c70b11e97e0d7aca7031daf8", "commit_rev":
"c0dfdbfdb6f8c3c5b2dd463691ef7b00508d5b5e"}
Message was sent while issue was closed.
Description was changed from ========== Added histograms for Custom Feedback UI Custom feedback was added to the internal build due to First Party APIs. However, there is very little reason to keep what we track off of a public commit. As a result, the logs are kept here instead. BUG=676476 ========== to ========== Added histograms for Custom Feedback UI Custom feedback was added to the internal build due to First Party APIs. However, there is very little reason to keep what we track off of a public commit. As a result, the logs are kept here instead. BUG=676476 Review-Url: https://codereview.chromium.org/2668783002 Cr-Commit-Position: refs/heads/master@{#447815} Committed: https://chromium.googlesource.com/chromium/src/+/c0dfdbfdb6f8c3c5b2dd463691ef... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/c0dfdbfdb6f8c3c5b2dd463691ef...
Message was sent while issue was closed.
(Pressed the wrong button, wanted to make sure you knew I did reply to the comment) https://codereview.chromium.org/2668783002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2668783002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:463: + Custom Feedback UI. On 2017/02/02 05:07:50, Mark P (away thru Feb 5 2017) wrote: > Is it possible they select an option without selecting a category first? > If not, then this lgtm. > If so, then what do you end up emitting here? It is not possible to select an option without selecting a category. |
