Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
13180002:
Adding metrics for notification clicks and dismissals (Closed)
|
Created:
7 years, 9 months ago by vadimt Modified:
7 years, 8 months ago Reviewers:
rgustafson, miket_OOO, arv (Not doing code reviews), darin (slow to review), Ilya Sherman, jar (doing other things), skare_, sky CC:
chromium-reviews, MAD, jar (doing other things), Aaron Boodman, arv+watch_chromium.org, chromium-apps-reviews_chromium.org, Ilya Sherman, stromme, govind_chromium.com Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdding metrics for notification clicks and dismissals using metricsPrivate API.
For this, copied metrics.js library from file manager.
BUG=164227
TEST=Open chrome://user-actions, and then click at notifications' body, buttons or dismiss it. Observe that corresponding events get printed on the page. Check for both component and non-component extensions. Wait for several days and check that the data appears at the dashboard.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=191700
Patch Set 1 #
Total comments: 17
Patch Set 2 : Fixing chromeactions.txt #Patch Set 3 : Using Unix line endings for chromeactions.txt #
Total comments: 2
Patch Set 4 : Reviews #
Total comments: 4
Messages
Total messages: 32 (0 generated)
https://codereview.chromium.org/13180002/diff/1/chrome/browser/resources/goog... File chrome/browser/resources/google_now/metrics.js (right): https://codereview.chromium.org/13180002/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/metrics.js:13: // TODO(vadimt): Remove unused code. possible to unify between the two extensions?
https://codereview.chromium.org/13180002/diff/1/chrome/browser/resources/goog... File chrome/browser/resources/google_now/metrics.js (right): https://codereview.chromium.org/13180002/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/metrics.js:13: // TODO(vadimt): Remove unused code. On 2013/03/28 21:24:51, Travis Skare wrote: > possible to unify between the two extensions? Ideas? Copy from file_manager as a custom build step?
https://codereview.chromium.org/13180002/diff/1/chrome/browser/resources/goog... File chrome/browser/resources/google_now/metrics.js (right): https://codereview.chromium.org/13180002/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/metrics.js:11: // This file was adapted from What other metrics will you track with this? If you're just using recordUserAction, this seems unnecessarily large. https://codereview.chromium.org/13180002/diff/1/chrome/tools/chromeactions.txt File chrome/tools/chromeactions.txt (right): https://codereview.chromium.org/13180002/diff/1/chrome/tools/chromeactions.tx... chrome/tools/chromeactions.txt:292: 0x436c20892470d838 GoogleNow.MessageClicked Why are there so many other changes in this file??? I'm guessing these are the only relevant things? Also >> = tab? If so, make it spaces.
https://codereview.chromium.org/13180002/diff/1/chrome/browser/resources/goog... File chrome/browser/resources/google_now/metrics.js (right): https://codereview.chromium.org/13180002/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/metrics.js:11: // This file was adapted from On 2013/03/28 21:49:56, rgustafson wrote: > What other metrics will you track with this? If you're just using > recordUserAction, this seems unnecessarily large. I also plan to record errors (may be, using histograms). The file has a TODO item to remove dead code before release. https://codereview.chromium.org/13180002/diff/1/chrome/tools/chromeactions.txt File chrome/tools/chromeactions.txt (right): https://codereview.chromium.org/13180002/diff/1/chrome/tools/chromeactions.tx... chrome/tools/chromeactions.txt:292: 0x436c20892470d838 GoogleNow.MessageClicked On 2013/03/28 21:49:56, rgustafson wrote: > Why are there so many other changes in this file??? I'm guessing these are the > only relevant things? > > Also >> = tab? If so, make it spaces. This file is generated by extract_actions.py. Simply people who removed counters didn't re-run this script.
https://codereview.chromium.org/13180002/diff/1/chrome/tools/chromeactions.txt File chrome/tools/chromeactions.txt (right): https://codereview.chromium.org/13180002/diff/1/chrome/tools/chromeactions.tx... chrome/tools/chromeactions.txt:292: 0x436c20892470d838 GoogleNow.MessageClicked On 2013/03/28 22:05:05, vadimt wrote: > On 2013/03/28 21:49:56, rgustafson wrote: > > Why are there so many other changes in this file??? I'm guessing these are the > > only relevant things? > > > > Also >> = tab? If so, make it spaces. > > This file is generated by extract_actions.py. Simply people who removed counters > didn't re-run this script. Aside from exceptional circumstances, nothing should ever be removed from this file, only appended. Likely, you're running extract_actions.py differently from how it's intended to be run...
https://codereview.chromium.org/13180002/diff/1/chrome/tools/chromeactions.txt File chrome/tools/chromeactions.txt (right): https://codereview.chromium.org/13180002/diff/1/chrome/tools/chromeactions.tx... chrome/tools/chromeactions.txt:292: 0x436c20892470d838 GoogleNow.MessageClicked On 2013/03/28 22:19:50, Ilya Sherman wrote: > On 2013/03/28 22:05:05, vadimt wrote: > > On 2013/03/28 21:49:56, rgustafson wrote: > > > Why are there so many other changes in this file??? I'm guessing these are > the > > > only relevant things? > > > > > > Also >> = tab? If so, make it spaces. > > > > This file is generated by extract_actions.py. Simply people who removed > counters > > didn't re-run this script. > > Aside from exceptional circumstances, nothing should ever be removed from this > file, only appended. Likely, you're running extract_actions.py differently from > how it's intended to be run... Ran extract_actions.py as intented :), which fixed this.
https://codereview.chromium.org/13180002/diff/1/chrome/tools/chromeactions.txt File chrome/tools/chromeactions.txt (right): https://codereview.chromium.org/13180002/diff/1/chrome/tools/chromeactions.tx... chrome/tools/chromeactions.txt:292: 0x436c20892470d838 GoogleNow.MessageClicked On 2013/03/28 22:37:26, vadimt wrote: > On 2013/03/28 22:19:50, Ilya Sherman wrote: > > On 2013/03/28 22:05:05, vadimt wrote: > > > On 2013/03/28 21:49:56, rgustafson wrote: > > > > Why are there so many other changes in this file??? I'm guessing these are > > the > > > > only relevant things? > > > > > > > > Also >> = tab? If so, make it spaces. > > > > > > This file is generated by extract_actions.py. Simply people who removed > > counters > > > didn't re-run this script. > > > > Aside from exceptional circumstances, nothing should ever be removed from this > > file, only appended. Likely, you're running extract_actions.py differently > from > > how it's intended to be run... > > Ran extract_actions.py as intented :), which fixed this. It looks like there might still be an issue with line endings or something? The diff stats for that file read "+1549 lines, -1537 lines", which seems definitely wrong...
https://codereview.chromium.org/13180002/diff/1/chrome/tools/chromeactions.txt File chrome/tools/chromeactions.txt (right): https://codereview.chromium.org/13180002/diff/1/chrome/tools/chromeactions.tx... chrome/tools/chromeactions.txt:292: 0x436c20892470d838 GoogleNow.MessageClicked On 2013/03/28 22:39:44, Ilya Sherman wrote: > On 2013/03/28 22:37:26, vadimt wrote: > > On 2013/03/28 22:19:50, Ilya Sherman wrote: > > > On 2013/03/28 22:05:05, vadimt wrote: > > > > On 2013/03/28 21:49:56, rgustafson wrote: > > > > > Why are there so many other changes in this file??? I'm guessing these > are > > > the > > > > > only relevant things? > > > > > > > > > > Also >> = tab? If so, make it spaces. > > > > > > > > This file is generated by extract_actions.py. Simply people who removed > > > counters > > > > didn't re-run this script. > > > > > > Aside from exceptional circumstances, nothing should ever be removed from > this > > > file, only appended. Likely, you're running extract_actions.py differently > > from > > > how it's intended to be run... > > > > Ran extract_actions.py as intented :), which fixed this. > > It looks like there might still be an issue with line endings or something? The > diff stats for that file read "+1549 lines, -1537 lines", which seems definitely > wrong... Looks like Python working from Windows generates non-Unix line ends. I've converted to Unix line ends, even though I believe that landing CL does this automatically.
https://codereview.chromium.org/13180002/diff/1/chrome/tools/chromeactions.txt File chrome/tools/chromeactions.txt (right): https://codereview.chromium.org/13180002/diff/1/chrome/tools/chromeactions.tx... chrome/tools/chromeactions.txt:292: 0x436c20892470d838 GoogleNow.MessageClicked On 2013/03/28 22:48:38, vadimt wrote: > On 2013/03/28 22:39:44, Ilya Sherman wrote: > > On 2013/03/28 22:37:26, vadimt wrote: > > > On 2013/03/28 22:19:50, Ilya Sherman wrote: > > > > On 2013/03/28 22:05:05, vadimt wrote: > > > > > On 2013/03/28 21:49:56, rgustafson wrote: > > > > > > Why are there so many other changes in this file??? I'm guessing these > > are > > > > the > > > > > > only relevant things? > > > > > > > > > > > > Also >> = tab? If so, make it spaces. > > > > > > > > > > This file is generated by extract_actions.py. Simply people who removed > > > > counters > > > > > didn't re-run this script. > > > > > > > > Aside from exceptional circumstances, nothing should ever be removed from > > this > > > > file, only appended. Likely, you're running extract_actions.py > differently > > > from > > > > how it's intended to be run... > > > > > > Ran extract_actions.py as intented :), which fixed this. > > > > It looks like there might still be an issue with line endings or something? > The > > diff stats for that file read "+1549 lines, -1537 lines", which seems > definitely > > wrong... > > Looks like Python working from Windows generates non-Unix line ends. > I've converted to Unix line ends, even though I believe that landing CL does > this automatically. Thanks. It would be great if you could fix the Python script as well, so that it generates the correct line endings on all platforms. (Also, fwiw, I don't think that it's the case that committing the CL automagically fixes line endings...)
https://codereview.chromium.org/13180002/diff/1/chrome/browser/resources/goog... File chrome/browser/resources/google_now/metrics.js (right): https://codereview.chromium.org/13180002/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/metrics.js:11: // This file was adapted from On 2013/03/28 22:05:05, vadimt wrote: > On 2013/03/28 21:49:56, rgustafson wrote: > > What other metrics will you track with this? If you're just using > > recordUserAction, this seems unnecessarily large. > > I also plan to record errors (may be, using histograms). > The file has a TODO item to remove dead code before release. There's absolutely no benefit to having this file right now, just more complexity to accomplish what could already be done in one line. I'd much prefer that you add this when you need it (or add code now that does make this necessary and lower the number of reviews for owners in the other various directories).
https://codereview.chromium.org/13180002/diff/1/chrome/browser/resources/goog... File chrome/browser/resources/google_now/metrics.js (right): https://codereview.chromium.org/13180002/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/metrics.js:11: // This file was adapted from On 2013/03/29 02:17:55, rgustafson wrote: > On 2013/03/28 22:05:05, vadimt wrote: > > On 2013/03/28 21:49:56, rgustafson wrote: > > > What other metrics will you track with this? If you're just using > > > recordUserAction, this seems unnecessarily large. > > > > I also plan to record errors (may be, using histograms). > > The file has a TODO item to remove dead code before release. > > There's absolutely no benefit to having this file right now, just more > complexity to accomplish what could already be done in one line. I'd much prefer > that you add this when you need it (or add code now that does make this > necessary and lower the number of reviews for owners in the other various > directories). agree - duplicating a sibling file seems error-prone, confusing for people who search for that API later, and might increase the size of the binary unnecessarily. You might not even need a wrapper around the API, similar to how Notifications Galore has one for notifications and you just call the API direct. https://codereview.chromium.org/13180002/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/metrics.js:13: // TODO(vadimt): Remove unused code. On 2013/03/28 21:36:50, vadimt wrote: > On 2013/03/28 21:24:51, Travis Skare wrote: > > possible to unify between the two extensions? > > Ideas? > Copy from file_manager as a custom build step? something like that, but only when you really need it. Or just call the APIs directly. https://codereview.chromium.org/13180002/diff/16001/chrome/tools/chromeaction... File chrome/tools/chromeactions.txt (right): https://codereview.chromium.org/13180002/diff/16001/chrome/tools/chromeaction... chrome/tools/chromeactions.txt:400: 0x436c20892470d838 GoogleNow.MessageClicked these 4 belong. Should all the others still be in the file? Seems like even if they need to be added, that could happen in another CL?
https://codereview.chromium.org/13180002/diff/1/chrome/browser/resources/goog... File chrome/browser/resources/google_now/metrics.js (right): https://codereview.chromium.org/13180002/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/metrics.js:11: // This file was adapted from On 2013/03/29 02:17:55, rgustafson wrote: > On 2013/03/28 22:05:05, vadimt wrote: > > On 2013/03/28 21:49:56, rgustafson wrote: > > > What other metrics will you track with this? If you're just using > > > recordUserAction, this seems unnecessarily large. > > > > I also plan to record errors (may be, using histograms). > > The file has a TODO item to remove dead code before release. > > There's absolutely no benefit to having this file right now, just more > complexity to accomplish what could already be done in one line. I'd much prefer > that you add this when you need it (or add code now that does make this > necessary and lower the number of reviews for owners in the other various > directories). Done. https://codereview.chromium.org/13180002/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/metrics.js:11: // This file was adapted from On 2013/03/29 18:08:35, Travis Skare wrote: > On 2013/03/29 02:17:55, rgustafson wrote: > > On 2013/03/28 22:05:05, vadimt wrote: > > > On 2013/03/28 21:49:56, rgustafson wrote: > > > > What other metrics will you track with this? If you're just using > > > > recordUserAction, this seems unnecessarily large. > > > > > > I also plan to record errors (may be, using histograms). > > > The file has a TODO item to remove dead code before release. > > > > There's absolutely no benefit to having this file right now, just more > > complexity to accomplish what could already be done in one line. I'd much > prefer > > that you add this when you need it (or add code now that does make this > > necessary and lower the number of reviews for owners in the other various > > directories). > > agree - duplicating a sibling file seems error-prone, confusing for people who > search for that API later, and might increase the size of the binary > unnecessarily. You might not even need a wrapper around the API, similar to how > Notifications Galore has one for notifications and you just call the API direct. Done. https://codereview.chromium.org/13180002/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/metrics.js:13: // TODO(vadimt): Remove unused code. On 2013/03/29 18:08:35, Travis Skare wrote: > On 2013/03/28 21:36:50, vadimt wrote: > > On 2013/03/28 21:24:51, Travis Skare wrote: > > > possible to unify between the two extensions? > > > > Ideas? > > Copy from file_manager as a custom build step? > > something like that, but only when you really need it. Or just call the APIs > directly. Done. https://codereview.chromium.org/13180002/diff/16001/chrome/tools/chromeaction... File chrome/tools/chromeactions.txt (right): https://codereview.chromium.org/13180002/diff/16001/chrome/tools/chromeaction... chrome/tools/chromeactions.txt:400: 0x436c20892470d838 GoogleNow.MessageClicked On 2013/03/29 18:08:35, Travis Skare wrote: > these 4 belong. Should all the others still be in the file? Seems like even if > they need to be added, that could happen in another CL? This would require manually editing the generated file, which is against the documented way. I'll get a review from isherman@ for this change anyways.
lgtm https://codereview.chromium.org/13180002/diff/22001/chrome/browser/resources/... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/13180002/diff/22001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:442: unnecessary new line
https://codereview.chromium.org/13180002/diff/22001/chrome/browser/resources/... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/13180002/diff/22001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:442: On 2013/03/29 20:20:00, rgustafson wrote: > unnecessary new line Yeah, I thought about this too. Given that it goes before a large block, the newline seems appropriate. However, let me know if you feel strong about removing it.
LGTM, thanks.
lgtm
arv@, please provide OWNER's approval for chrome/browser/resources/google_now/* miket@, please provide OWNER's approval for chrome/common/extensions/api/_permission_features.json
LGTM https://codereview.chromium.org/13180002/diff/22001/chrome/tools/chromeaction... File chrome/tools/chromeactions.txt (right): https://codereview.chromium.org/13180002/diff/22001/chrome/tools/chromeaction... chrome/tools/chromeactions.txt:61: 0xb3ca99d79240c92f Accel_LockScreen_L Is there are reason for these unrelated changes?
https://codereview.chromium.org/13180002/diff/22001/chrome/tools/chromeaction... File chrome/tools/chromeactions.txt (right): https://codereview.chromium.org/13180002/diff/22001/chrome/tools/chromeaction... chrome/tools/chromeactions.txt:61: 0xb3ca99d79240c92f Accel_LockScreen_L On 2013/03/29 22:58:33, arv wrote: > Is there are reason for these unrelated changes? Someone seems to haven't re-generated this file after adding their events. So, when I run extract_actions.py, it adds all counters since it was run last time.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vadimt@chromium.org/13180002/22001
Presubmit check for 13180002-22001 failed and returned exit status 1.
INFO:root:Found 5 file(s).
Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/resources/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/common/extensions/PRESUBMIT.py
** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
chrome/tools/chromeactions.txt
chrome/tools/extract_actions.py
chrome/common/extensions/api/_permission_features.json
Presubmit checks took 2.2s to calculate.
sky@, please provide OWNER's review for chrome/tools/*
I'm not a good reviewer here, maybe jar?
LGTM for chrome/tools
chrome/common/extensions/api/_permission_features.json LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vadimt@chromium.org/13180002/22001
Presubmit check for 13180002-22001 failed and returned exit status 1.
INFO:root:Found 5 file(s).
Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/resources/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/common/extensions/PRESUBMIT.py
** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
chrome/tools/chromeactions.txt
chrome/tools/extract_actions.py
Presubmit checks took 2.3s to calculate.
darin@, could you please provide OWNER's approval for:
chrome/tools/chromeactions.txt
chrome/tools/extract_actions.py
Thanks!
LGTM (OWNERS rubber stamp)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vadimt@chromium.org/13180002/22001
Message was sent while issue was closed.
Change committed as 191700 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
