Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1155483002:
Adding UMA for persistent notifications. (Closed)
|
Created:
5 years, 7 months ago by Deepak Modified:
5 years, 6 months ago CC:
chromium-reviews, peter+watch_chromium.org, mlamouri+watch-notifications_chromium.org, asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdding UMA for persistent notifications callbacks so that when these functions get called we will have record.
BUG=490583
Committed: https://crrev.com/67992a64a582f725c38f9317479beaaa9bc962d9
Cr-Commit-Position: refs/heads/master@{#331742}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Changes as per review comments. #Patch Set 3 : Changes as per review comments. #
Total comments: 7
Patch Set 4 : Changes as per review comments. #
Total comments: 10
Patch Set 5 : Changes in histogram as per review comments. #
Total comments: 9
Patch Set 6 : Changes as per review comments. #
Total comments: 15
Patch Set 7 : Changes as per review comments. #Patch Set 8 : Changing histgram enum label. #
Messages
Total messages: 32 (7 generated)
deepak.m1@samsung.com changed reviewers: + peter@chromium.org
PTAL
Cheers! Please see my comments. https://codereview.chromium.org/1155483002/diff/1/chrome/browser/notification... File chrome/browser/notifications/platform_notification_service_impl.cc (right): https://codereview.chromium.org/1155483002/diff/1/chrome/browser/notification... chrome/browser/notifications/platform_notification_service_impl.cc:54: content::RecordAction( I was thinking of using a UMA_HISTOGRAM_BOOLEAN() here, since capturing the |success| state is important. We don't care about *how often* these events happen, but we do care about their success rate. Dito on lines 61-62. https://codereview.chromium.org/1155483002/diff/1/chrome/browser/notification... chrome/browser/notifications/platform_notification_service_impl.cc:318: std::set<std::string> persistent_notifications; Please let this TODO be for now, I'm working on a better way to identify those, which comes with large improvements in the way how we route this. https://codereview.chromium.org/1155483002/diff/1/tools/metrics/actions/actio... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/1155483002/diff/1/tools/metrics/actions/actio... tools/metrics/actions/actions.xml:2680: <action name="DocumentActivity_OptOutShownOnHome"> Wrong rebase? :)
@Peter Thanks for review. PTAL https://codereview.chromium.org/1155483002/diff/1/chrome/browser/notification... File chrome/browser/notifications/platform_notification_service_impl.cc (right): https://codereview.chromium.org/1155483002/diff/1/chrome/browser/notification... chrome/browser/notifications/platform_notification_service_impl.cc:54: content::RecordAction( On 2015/05/21 11:33:10, Peter Beverloo wrote: > I was thinking of using a UMA_HISTOGRAM_BOOLEAN() here, since capturing the > |success| state is important. We don't care about *how often* these events > happen, but we do care about their success rate. > > Dito on lines 61-62. Done. https://codereview.chromium.org/1155483002/diff/1/tools/metrics/actions/actio... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/1155483002/diff/1/tools/metrics/actions/actio... tools/metrics/actions/actions.xml:2680: <action name="DocumentActivity_OptOutShownOnHome"> On 2015/05/21 11:33:10, Peter Beverloo wrote: > Wrong rebase? :) I have done rebase again, and then making the changes in this file asked me to run tools/metrics/actions/extract_actions.py And after running this the strings that are not added in this file are generated and getting added. I checked with "Stars_SignInPromoHeader_Displayed" This is getting used in EnhancedBookmarkPromoHeader.java file and is not added in actions.xml Like this other are getting used and not added in action.xml file.
deepak.m1@samsung.com changed reviewers: + dewittj@chromium.org
@Peter I have removed dependency from action.xml file.. may be it was due to rebase issue.:) @dewittj Please review.
https://codereview.chromium.org/1155483002/diff/40001/chrome/browser/notifica... File chrome/browser/notifications/platform_notification_service_impl.cc (right): https://codereview.chromium.org/1155483002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/platform_notification_service_impl.cc:57: "PlatformNotificationService.PersistentNotificationDataDeleted", success); These need to be added to tools/metrics/histograms/histograms.xml, otherwise they won't actually show up in the dashboards. https://codereview.chromium.org/1155483002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/platform_notification_service_impl.cc:64: "PlatformNotificationService.PersistentWebNotificationComplete", Why wouldn't we record all states of |status| here using UMA_HISTOGRAM_ENUMERATION? Sorry if that wasn't clear. https://codereview.chromium.org/1155483002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/platform_notification_service_impl.cc:64: "PlatformNotificationService.PersistentWebNotificationComplete", naming nit: PNS.PersistentWebNotificationClickResult? https://codereview.chromium.org/1155483002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/platform_notification_service_impl.cc:66: PERSISTENT_NOTIFICATION_STATUS_SUCCESS == status); nit: indentation.
Thanks for review. PTAL https://codereview.chromium.org/1155483002/diff/40001/chrome/browser/notifica... File chrome/browser/notifications/platform_notification_service_impl.cc (right): https://codereview.chromium.org/1155483002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/platform_notification_service_impl.cc:57: "PlatformNotificationService.PersistentNotificationDataDeleted", success); On 2015/05/22 13:40:34, Peter Beverloo wrote: > These need to be added to tools/metrics/histograms/histograms.xml, otherwise > they won't actually show up in the dashboards. Done. https://codereview.chromium.org/1155483002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/platform_notification_service_impl.cc:64: "PlatformNotificationService.PersistentWebNotificationComplete", On 2015/05/22 13:40:34, Peter Beverloo wrote: > Why wouldn't we record all states of |status| here using > UMA_HISTOGRAM_ENUMERATION? Sorry if that wasn't clear. Done. https://codereview.chromium.org/1155483002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/platform_notification_service_impl.cc:66: PERSISTENT_NOTIFICATION_STATUS_SUCCESS == status); On 2015/05/22 13:40:35, Peter Beverloo wrote: > nit: indentation. ok, I will run 'git cl format' for checking indentation. Thanks
https://codereview.chromium.org/1155483002/diff/60001/chrome/browser/notifica... File chrome/browser/notifications/platform_notification_service_impl.cc (right): https://codereview.chromium.org/1155483002/diff/60001/chrome/browser/notifica... chrome/browser/notifications/platform_notification_service_impl.cc:56: UMA_HISTOGRAM_BOOLEAN("PNS.PersistentNotificationDataDeleted", success); Oh dear. Sorry, I typed PNS as an acronym for PlatformNotificationService to avoid typing it all out. The names should start with "PlatformNotificationService" instead of PNS. https://codereview.chromium.org/1155483002/diff/60001/chrome/browser/notifica... chrome/browser/notifications/platform_notification_service_impl.cc:65: PERSISTENT_NOTIFICATION_STATUS_DATABASE_ERROR + 1); Could we add a PERSISTENT_NOTIFICATION_STATUS_MAX to the PersistentNotificationStatus enumeration? Also, the indentation is still off, it should be four spaces. We'll also need a new <enum> in histograms.xml to make sure its values are properly represented. https://codereview.chromium.org/1155483002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1155483002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:29697: + <owner>deepak.m1@samsung.com</owner> Please add me as an owner for these histograms as well. https://codereview.chromium.org/1155483002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:29698: + <summary>Logged when notification data get deleted.</summary> --> Recorded when the data associated with a persistent Web Notification gets deleted. https://codereview.chromium.org/1155483002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:29703: + <summary>Logged when Notification get clicked.</summary> --> Recorded when handling a click on a persistent Web Notification has finished.
@Peter Thanks for review. PTAL https://codereview.chromium.org/1155483002/diff/60001/chrome/browser/notifica... File chrome/browser/notifications/platform_notification_service_impl.cc (right): https://codereview.chromium.org/1155483002/diff/60001/chrome/browser/notifica... chrome/browser/notifications/platform_notification_service_impl.cc:56: UMA_HISTOGRAM_BOOLEAN("PNS.PersistentNotificationDataDeleted", success); On 2015/05/22 15:35:42, Peter Beverloo wrote: > Oh dear. Sorry, I typed PNS as an acronym for PlatformNotificationService to > avoid typing it all out. > > The names should start with "PlatformNotificationService" instead of PNS. Done. https://codereview.chromium.org/1155483002/diff/60001/chrome/browser/notifica... chrome/browser/notifications/platform_notification_service_impl.cc:65: PERSISTENT_NOTIFICATION_STATUS_DATABASE_ERROR + 1); On 2015/05/22 15:35:42, Peter Beverloo wrote: > Could we add a PERSISTENT_NOTIFICATION_STATUS_MAX to the > PersistentNotificationStatus enumeration? > > Also, the indentation is still off, it should be four spaces. > > We'll also need a new <enum> in histograms.xml to make sure its values are > properly represented. Done. https://codereview.chromium.org/1155483002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1155483002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:29697: + <owner>deepak.m1@samsung.com</owner> On 2015/05/22 15:35:42, Peter Beverloo wrote: > Please add me as an owner for these histograms as well. Done. https://codereview.chromium.org/1155483002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:29698: + <summary>Logged when notification data get deleted.</summary> On 2015/05/22 15:35:42, Peter Beverloo wrote: > --> Recorded when the data associated with a persistent Web Notification gets > deleted. Done. https://codereview.chromium.org/1155483002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:29703: + <summary>Logged when Notification get clicked.</summary> On 2015/05/22 15:35:42, Peter Beverloo wrote: > --> Recorded when handling a click on a persistent Web Notification has > finished. Done.
deepak.m1@samsung.com changed reviewers: + asvitkine@chromium.org, nasko@chromium.org
https://codereview.chromium.org/1155483002/diff/80001/content/public/common/p... File content/public/common/persistent_notification_status.h (right): https://codereview.chromium.org/1155483002/diff/80001/content/public/common/p... content/public/common/persistent_notification_status.h:10: // Delivery status for persistent notification clicks to a Service Worker. Now that this is used in UMA, mention that entries shouldn't be re-ordered or removed. https://codereview.chromium.org/1155483002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1155483002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:28496: +<histogram name="PlatformNotificationService.PersistentNotificationDataDeleted" Can these be under the ServiceWorker.* prefix? Or is it not always related to service workers? Generally, best practice is to not introduce a new top-level prefix. https://codereview.chromium.org/1155483002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:28497: + enum="Boolean"> Please use a more specific enum than just Boolean - like BooleanSuccess or BooleanDeleted. Add one if the most appropriate one doesn't exist yet. https://codereview.chromium.org/1155483002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:28502: + deleted. Mention what the value means.
@Alexei Thanks for review, Changes done as suggested. PTAL https://codereview.chromium.org/1155483002/diff/80001/content/public/common/p... File content/public/common/persistent_notification_status.h (right): https://codereview.chromium.org/1155483002/diff/80001/content/public/common/p... content/public/common/persistent_notification_status.h:10: // Delivery status for persistent notification clicks to a Service Worker. On 2015/05/26 19:21:23, Alexei Svitkine wrote: > Now that this is used in UMA, mention that entries shouldn't be re-ordered or > removed. Done. https://codereview.chromium.org/1155483002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1155483002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:28496: +<histogram name="PlatformNotificationService.PersistentNotificationDataDeleted" On 2015/05/26 19:21:23, Alexei Svitkine wrote: > Can these be under the ServiceWorker.* prefix? Or is it not always related to > service workers? Generally, best practice is to not introduce a new top-level > prefix. Done. https://codereview.chromium.org/1155483002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:28497: + enum="Boolean"> On 2015/05/26 19:21:23, Alexei Svitkine wrote: > Please use a more specific enum than just Boolean - like BooleanSuccess or > BooleanDeleted. Add one if the most appropriate one doesn't exist yet. Done. https://codereview.chromium.org/1155483002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:28502: + deleted. On 2015/05/26 19:21:23, Alexei Svitkine wrote: > Mention what the value means. Done.
Cool, thanks! A few naming nits remaining, lgtm otherwise. https://codereview.chromium.org/1155483002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1155483002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:28496: +<histogram name="PlatformNotificationService.PersistentNotificationDataDeleted" On 2015/05/27 04:41:18, Deepak wrote: > On 2015/05/26 19:21:23, Alexei Svitkine wrote: > > Can these be under the ServiceWorker.* prefix? Or is it not always related to > > service workers? Generally, best practice is to not introduce a new top-level > > prefix. > > Done. Sorry to disagree with Alexei here, but I would prefer to see them under the "Notifications" top-level prefix in that case. My proposal: Notifications.PersistentNotificationDataDeleted Notifications.PersistentWebNotificationClickResult PlatformNotificationStatus (the enum) https://codereview.chromium.org/1155483002/diff/100001/content/public/common/... File content/public/common/persistent_notification_status.h (right): https://codereview.chromium.org/1155483002/diff/100001/content/public/common/... content/public/common/persistent_notification_status.h:11: // NotificationStatus entries should not be reordered or removed. nit: Either "PersistentNotificationStatus entries should..." or just "Entries should..." https://codereview.chromium.org/1155483002/diff/100001/content/public/common/... content/public/common/persistent_notification_status.h:30: PERSISTENT_NOTIFICATION_STATUS_MAX nit: Please add a comment above this line saying: // Only add new entries above this line. https://codereview.chromium.org/1155483002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1155483002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:63443: + <int value="0" label="PERSISTENT_NOTIFICATION_STATUS_SUCCESS"/> label = OK https://codereview.chromium.org/1155483002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:63444: + <int value="1" label="PERSISTENT_NOTIFICATION_STATUS_NO_SERVICE_WORKER"/> Service Worker not found https://codereview.chromium.org/1155483002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:63445: + <int value="2" label="PERSISTENT_NOTIFICATION_STATUS_SERVICE_WORKER_ERROR"/> Service Worker error https://codereview.chromium.org/1155483002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:63447: + label="PERSISTENT_NOTIFICATION_STATUS_EVENT_WAITUNTIL_REJECTED"/> event.waitUntil promise rejected https://codereview.chromium.org/1155483002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:63448: + <int value="4" label="PERSISTENT_NOTIFICATION_STATUS_DATABASE_ERROR"/> Database error
@Peter Thanks for review. nit addressed. @nasko for persistent_notification_status.h @ASvitkine for histograms.xml https://codereview.chromium.org/1155483002/diff/100001/content/public/common/... File content/public/common/persistent_notification_status.h (right): https://codereview.chromium.org/1155483002/diff/100001/content/public/common/... content/public/common/persistent_notification_status.h:11: // NotificationStatus entries should not be reordered or removed. On 2015/05/27 12:28:48, Peter Beverloo wrote: > nit: Either "PersistentNotificationStatus entries should..." or just "Entries > should..." Done. https://codereview.chromium.org/1155483002/diff/100001/content/public/common/... content/public/common/persistent_notification_status.h:30: PERSISTENT_NOTIFICATION_STATUS_MAX On 2015/05/27 12:28:48, Peter Beverloo wrote: > nit: Please add a comment above this line saying: > > // Only add new entries above this line. Done. https://codereview.chromium.org/1155483002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1155483002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:63443: + <int value="0" label="PERSISTENT_NOTIFICATION_STATUS_SUCCESS"/> On 2015/05/27 12:28:48, Peter Beverloo wrote: > label = OK Done. https://codereview.chromium.org/1155483002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:63444: + <int value="1" label="PERSISTENT_NOTIFICATION_STATUS_NO_SERVICE_WORKER"/> On 2015/05/27 12:28:48, Peter Beverloo wrote: > Service Worker not found Done. https://codereview.chromium.org/1155483002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:63445: + <int value="2" label="PERSISTENT_NOTIFICATION_STATUS_SERVICE_WORKER_ERROR"/> On 2015/05/27 12:28:48, Peter Beverloo wrote: > Service Worker error Done. https://codereview.chromium.org/1155483002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:63447: + label="PERSISTENT_NOTIFICATION_STATUS_EVENT_WAITUNTIL_REJECTED"/> On 2015/05/27 12:28:48, Peter Beverloo wrote: > event.waitUntil promise rejected I have made this like: "Event wait Until promise rejected" As I found that First character of first Word is always capital letter :) https://codereview.chromium.org/1155483002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:63448: + <int value="4" label="PERSISTENT_NOTIFICATION_STATUS_DATABASE_ERROR"/> On 2015/05/27 12:28:48, Peter Beverloo wrote: > Database error Done.
https://codereview.chromium.org/1155483002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1155483002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:63447: + label="PERSISTENT_NOTIFICATION_STATUS_EVENT_WAITUNTIL_REJECTED"/> On 2015/05/27 12:42:39, Deepak wrote: > On 2015/05/27 12:28:48, Peter Beverloo wrote: > > event.waitUntil promise rejected > > I have made this like: > "Event wait Until promise rejected" > > As I found that First character of first Word is always capital letter :) event.waitUntil() actually is the name of the JavaScript method which will be used to trigger this! :-) Could you keep my suggestion? You'll find that we do the same for a Push enumeration elsewhere in this file (I actually copied the text!).
On 2015/05/27 12:45:45, Peter Beverloo wrote: > https://codereview.chromium.org/1155483002/diff/100001/tools/metrics/histogra... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/1155483002/diff/100001/tools/metrics/histogra... > tools/metrics/histograms/histograms.xml:63447: + > label="PERSISTENT_NOTIFICATION_STATUS_EVENT_WAITUNTIL_REJECTED"/> > On 2015/05/27 12:42:39, Deepak wrote: > > On 2015/05/27 12:28:48, Peter Beverloo wrote: > > > event.waitUntil promise rejected > > > > I have made this like: > > "Event wait Until promise rejected" > > > > As I found that First character of first Word is always capital letter :) > > event.waitUntil() actually is the name of the JavaScript method which will be > used to trigger this! :-) > > Could you keep my suggestion? You'll find that we do the same for a Push > enumeration elsewhere in this file (I actually copied the text!). Sure. Changes done.
lgtm
The CQ bit was checked by deepak.m1@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org Link to the patchset: https://codereview.chromium.org/1155483002/#ps140001 (title: "Changing histgram enum label.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1155483002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
@nasko Please provide approval for content/public/common/persistent_notification_status.h Thanks
content/public Rubberstamp LGTM
On 2015/05/28 05:04:09, nasko wrote: > content/public Rubberstamp LGTM Thankyou all for review.
The CQ bit was checked by deepak.m1@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1155483002/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/67992a64a582f725c38f9317479beaaa9bc962d9 Cr-Commit-Position: refs/heads/master@{#331742}
Message was sent while issue was closed.
Uh, I just see that you submitted this without replying to my naming concerns here: https://codereview.chromium.org/1155483002/diff/80001/tools/metrics/histogram... Could you please still do so? I don't think that these are the UMA histogram names we should be using...
Message was sent while issue was closed.
On 2015/05/28 13:42:00, Peter Beverloo wrote: > Uh, I just see that you submitted this without replying to my naming concerns > here: > > https://codereview.chromium.org/1155483002/diff/80001/tools/metrics/histogram... > > Could you please still do so? I don't think that these are the UMA histogram > names we should be using... https://codereview.chromium.org/1161983002/ has been raised as follow up to address above concerns. Thanks |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
