1225336 - Add telemetry about notification display/messages

Status: RESOLVED FIXED

(Toolkit Graveyard :: Notifications and Alerts, enhancement)

Description

[Matthew N. [:MattN]]

Implemented the "Messages" section of https://public.etherpad-mozilla.org/p/notification-telemetry which gathers telemetry about the Notifications that appear on screen.

Comment 1

[Edwin Wong [:edwong]]

NI: Jeff - please provide direction on what telemetry is opt-in/out. thx

Comment 2

[Matthew N. [:MattN]]

Attachment: MozReview Request: Bug 1225336 - Add telemetry about notification display/messages. r=wchen,kitcambridge

Bug 1225336 - Add telemetry about notification display/messages. r=wchen,kitcambridge

Comment 3

[Lina Butler [:lina]]

Comment on attachment 8693851 [details]
MozReview Request: Bug 1225336 - Add telemetry about notification display/messages. r=wchen,kitcambridge

https://reviewboard.mozilla.org/r/26583/#review24089

Looks good!

::: toolkit/components/telemetry/Histograms.json:10095
(Diff revision 1)
> +    "description": "Count of times a Notification was rendered (accouting for XUL DND). A system backend may put the notification directly into the tray if it's own DND is on."

Nit: "accounting," s/it's/its.

Comment 4

[William Chen [:wchen]]

Comment on attachment 8693851 [details]
MozReview Request: Bug 1225336 - Add telemetry about notification display/messages. r=wchen,kitcambridge

https://reviewboard.mozilla.org/r/26583/#review24157

::: dom/notification/Notification.cpp:1191
(Diff revision 1)
> +  if (!strcmp("alertshow", aTopic) ||

I think it's better to keep the else if statement as before and put the check for "alertshow" in the body so we only do an additional string comparison for the "alertshow" and "alertfinsihed" case instead of every case.

Comment 5

[Vladan Djeric (:vladan)]

Comment on attachment 8693851 [details]
MozReview Request: Bug 1225336 - Add telemetry about notification display/messages. r=wchen,kitcambridge

https://reviewboard.mozilla.org/r/26583/#review24197

Looks good to me.
You could put all these histograms into a single enum if you plan to compare them against each other in the dash

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/06ff4eab29c6

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/06ff4eab29c6

Comment 8

[Matthew N. [:MattN]]

Comment on attachment 8693851 [details]
MozReview Request: Bug 1225336 - Add telemetry about notification display/messages. r=wchen,kitcambridge

Approval Request Comment
[Feature/regressing bug #]: Push notifications
[User impact if declined]: No understanding their users
[Describe test coverage new/current, TreeHerder]: None, we will watch dashboards
[Risks and why]: Low risk telemetry addition.
[String/UUID change made/needed]: None

Comment 9

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8693851 [details]
MozReview Request: Bug 1225336 - Add telemetry about notification display/messages. r=wchen,kitcambridge

Seems like a good idea, Aurora44+

Comment 10

[Carsten Book [:Tomcat]]

has problems to apply to aurora:

Tomcats-MacBook-Pro-2:mozilla-central Tomcat$ hg graft --edit -r 06ff4eab29c6
grafting 317795:06ff4eab29c6 "Bug 1225336 - Add telemetry about web notification display/messages. r=wchen,kitcambridge p=vladan"
merging dom/notification/Notification.cpp
merging toolkit/components/alerts/resources/content/alert.js
merging toolkit/components/telemetry/Histograms.json
warning: conflicts while merging dom/notification/Notification.cpp! (edit, then use 'hg resolve --mark')
warning: conflicts while merging toolkit/components/telemetry/Histograms.json! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue

Comment 11

[Lina Butler [:lina]]

Attachment: 1.patch

Looks like this missed Aurora 44. Re-requesting uplift for Beta. Same rationale as comment 8.

Comment 12

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8702775 [details] [diff] [review]
1.patch

Makes sense, Bata44+

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/0f0e157a12b2

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/0f0e157a12b2