1219030 - Add telemetry for notification management

Status: RESOLVED FIXED

(Core :: DOM: Push Subscriptions, defect)

Description

[Lina Butler [:lina]]

* Flag if "do not disturb" is supported, captured at startup.
* Boolean for toggling "do not disturb" in content preferences.
* Count removed permissions (enum: "remove deny" and "remove allow").
* Count remove all permissions.
* Count times the notification permissions subdialog is opened.
* Count number of unique domains with notification permissions.

Comment 1

[Lina Butler [:lina]]

Attachment: MozReview Request: Bug 1219030 - Collect notification management telemetry. p=ally, r?wchen,MattN

Bug 1219030, Part 1 - Collect notification display telemetry.

Comment 2

[Lina Butler [:lina]]

Comment on attachment 8683426 [details]
MozReview Request: Bug 1219030 - Collect notification management telemetry. p=ally, r?wchen,MattN

Here's a first cut at collecting telemetry for:

* Whether XUL do not disturb is supported.
* When a notification permission is revoked, distinguishing between "revoke allow" and "revoke deny."
* The number of origins with a notification permission, collected at startup.

Comment 3

[Lina Butler [:lina]]

https://reviewboard.mozilla.org/r/24319/#review22623

::: dom/notification/Notification.cpp:1402
(Diff revision 1)
> +        telemetryService->NoteShow(notification)));

This needs to account for private browsing.

Comment 4

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/24319/#review22705

LGTM from the measurement side

::: dom/notification/Notification.h:64
(Diff revision 1)
> +  nsresult NoteShow(Notification* aNotification);
> +
> +private:
> +  NotificationTelemetryService();
> +  virtual ~NotificationTelemetryService();
> +
> +  void Startup();
> +  nsresult ObservePermissionChange();
> +  nsresult NoteDoNotDisturbSupported();
> +  nsresult NotePermissions();

Nit: s/Note/Accumulate/ or s/Note/Record/ for clarity

"Note" can get confused with "Notification"

::: dom/notification/Notification.cpp:700
(Diff revision 1)
> +NotificationTelemetryService::ObservePermissionChange()

Nit: This method name makes me think it's the listener itself but it's just doing the setup. I think the name should be more clear.

::: dom/notification/Notification.cpp:801
(Diff revision 1)
> +  nsAutoString origin;
> +  nsresult rv = Notification::GetOrigin(principal, origin);
> +  if (NS_FAILED(rv)) {
> +    return rv;
> +  }

FYI: this will include things like the system principal origin but maybe that's fine.

::: toolkit/components/telemetry/Histograms.json:9986
(Diff revision 1)
> +    "alert_emails": ["[e-mail]"],

I think this list has over a hundred people on it and has high SNR currently so probably not a good list. I think [e-mail] would be better (and that is already being used by other probes).

::: toolkit/components/telemetry/Histograms.json:9989
(Diff revision 1)
> +    "releaseChannelCollection": "opt-out",

Bill was supposed to talk to the privacy team and figure out which should be opt-out…

Comment 5

[Lina Butler [:lina]]

https://reviewboard.mozilla.org/r/24319/#review22705

> Nit: s/Note/Accumulate/ or s/Note/Record/ for clarity
> 
> "Note" can get confused with "Notification"

"Record" is nice.

> Nit: This method name makes me think it's the listener itself but it's just doing the setup. I think the name should be more clear.

Good point. Renamed to `AddPermissionChangeObserver`, since that's what it does. :-)

> FYI: this will include things like the system principal origin but maybe that's fine.

I think we should ignore system and expanded principals.

Comment 6

[Lina Butler [:lina]]

Comment on attachment 8683426 [details]
MozReview Request: Bug 1219030 - Collect notification management telemetry. p=ally, r?wchen,MattN

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24319/diff/1-2/

Comment 7

[Lina Butler [:lina]]

Attachment: MozReview Request: Bug 1219030, Part 2 - Collect notification management UI telemetry.

Bug 1219030, Part 2 - Collect notification management UI telemetry.

Comment 8

[Lina Butler [:lina]]

Comment on attachment 8683426 [details]
MozReview Request: Bug 1219030 - Collect notification management telemetry. p=ally, r?wchen,MattN

Ally, could you have a look at this patch, please? We'd like some guidance around opt-in vs. opt-out, too. I think the Push histograms are all opt-in, so maybe it makes sense to do the same here?

Comment 9

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/24319/#review22815

::: toolkit/components/telemetry/Histograms.json:10039
(Diff revision 2)
> +  },
> +  "NOTIFICATION_SHOWN": {
> +    "alert_emails": ["[e-mail]"],

This is display telemetry, not management related so should be in bug 1225336 and the two patches here which are both about management can maybe be folded together

::: toolkit/components/telemetry/Histograms.json:10040
(Diff revision 2)
> +  "NOTIFICATION_SHOWN": {
> +    "alert_emails": ["[e-mail]"],
> +    "expires_in_version": "48",
> +    "kind": "count",
> +    "releaseChannelCollection": "opt-out",
> +    "description": "Number of origins that have shown a notification."

Please rename the probe name to match the description. There is another probe requested to keep track of the number of notifications shown which would be better for this name.

Comment 10

[Allison Naaktgeboren :ally]

https://reviewboard.mozilla.org/r/24319/#review22885

::: toolkit/components/telemetry/Histograms.json:10041
(Diff revision 2)
> +    "alert_emails": ["[e-mail]"],

I understand gfritzche has recently added a bug_number field for histograms. Please add it and the bug number

::: toolkit/components/telemetry/Histograms.json:10044
(Diff revision 2)
> +    "releaseChannelCollection": "opt-out",

The precedent for opt-in vs opt-out is opt-in, regular telemetry, unless you have a really compelling reason for doing so.

I know your product person, Bill Maggs, has left recently, but are you aware of such a compelling reason?

::: toolkit/components/telemetry/Histograms.json:10049
(Diff revision 2)
> +    "expires_in_version": "48",

Thank you for remembering the expires_in_version flag.

::: toolkit/components/telemetry/Histograms.json:10065
(Diff revision 2)
> +    "n_values": 5,

We generally suggest a slightly higer number of n_values to leave room for future values. n_values can't be changed after the probe lands and starts reporting to the server and this only leaves room for 3 more. 10 perhaps? Up to you.

Thanks for not collecting with sites 
with changes addressed and probes changed to opt-in (ie regular telemetry probes), you can have a p=ally.

However, if you have your heart(s) set on opt-out, the bar's a bit higher. What the compelling reason for opt-out is, what feature/changes you have in mind/actions to take based on the data needed to be documented clearly. 

I understand Bill's departure might put y'all in a tight spot, so I'd advise you to take the opt-in and opt-out can be made later if data and product need warrant it.

Comment 11

[Allison Naaktgeboren :ally]

https://reviewboard.mozilla.org/r/25221/#review22889

Identical feedback to the first patch really.
p=ally with additon of bug_number, change to opt-in/regular telemetry probe, etc.

Comment 12

[Edwin Wong [:edwong]]

NI: Jeff - can you provide color on what should be opt-in/out for each telemetry item.

Comment 13

[Edwin Wong [:edwong]]

we're using this to mark each one: https://public.etherpad-mozilla.org/p/notification-telemetry

Comment 14

[William Chen [:wchen]]

https://reviewboard.mozilla.org/r/24319/#review23149

::: dom/notification/Notification.cpp:663
(Diff revision 2)
> +NotificationTelemetryService* sTelemetryService = nullptr;

What is keeping the service alive?

::: dom/notification/Notification.cpp:680
(Diff revision 2)
> +NotificationTelemetryService::GetInstance()

I don't think you need to do the work of managing your own singleton, you can just let XPConnect do this for you since you've already done all the work of setting up the contract.

It looks like you can just use NS_GENERIC_FACTORY_CONSTRUCTOR_INIT for your factory contructor and use do_GetService when you want to get the service.

::: dom/notification/Notification.cpp:706
(Diff revision 2)
> +  return obs->AddObserver(this, "perm-changed", true);

Remove the observer in the destructor.

::: layout/build/nsLayoutModule.cpp:1358
(Diff revision 2)
> +  { "profile-before-change", "Notification Telemetry Service", NOTIFICATIONTELEMETRYSERVICE_CONTRACTID },

It looks like you mean to start your service early at start-up. If that's the case, I'm not sure profile-before-change is what you want because I think that topic gets used at shutdown. I think you want to use profile-after-change for startup. Also, if you do switch to using a XPConnect singleton, then you should probably also add "service" to the category entry.

Comment 15

[Lina Butler [:lina]]

https://reviewboard.mozilla.org/r/24319/#review22815

> Please rename the probe name to match the description. There is another probe requested to keep track of the number of notifications shown which would be better for this name.

Renamed to "NOTIFICATION_SENDERS".

Comment 16

[Lina Butler [:lina]]

https://reviewboard.mozilla.org/r/24319/#review22885

> The precedent for opt-in vs opt-out is opt-in, regular telemetry, unless you have a really compelling reason for doing so.
> 
> I know your product person, Bill Maggs, has left recently, but are you aware of such a compelling reason?

I think we'll want some opt-out metrics to see if the notification controls are discoverable. At the very least, we want to make sure we aren't annoying users. Identifying the relevant probes is fodder for a breakdown session, though. Changing these to opt-in for now.

> We generally suggest a slightly higer number of n_values to leave room for future values. n_values can't be changed after the probe lands and starts reporting to the server and this only leaves room for 3 more. 10 perhaps? Up to you.

Increased to 10 values. I don't think we'll add too many new states (maybe "always ask"), but famous last words and all.

Comment 17

[Lina Butler [:lina]]

https://reviewboard.mozilla.org/r/24319/#review23149

> I don't think you need to do the work of managing your own singleton, you can just let XPConnect do this for you since you've already done all the work of setting up the contract.
> 
> It looks like you can just use NS_GENERIC_FACTORY_CONSTRUCTOR_INIT for your factory contructor and use do_GetService when you want to get the service.

*headdesk* Of course! I forgot I can use `static_cast` to cast the `nsCOMPtr` back to a `NotificationTelemetryService*`.

> It looks like you mean to start your service early at start-up. If that's the case, I'm not sure profile-before-change is what you want because I think that topic gets used at shutdown. I think you want to use profile-after-change for startup. Also, if you do switch to using a XPConnect singleton, then you should probably also add "service" to the category entry.

Oops, you're right; I want `profile-after-change`. Thanks!

If I prepend "service," to the contract ID, it'll call the init function on the first `do_GetService` call, instead of at startup.

Comment 18

[Lina Butler [:lina]]

Comment on attachment 8683426 [details]
MozReview Request: Bug 1219030 - Collect notification management telemetry. p=ally, r?wchen,MattN

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24319/diff/2-3/

Comment 19

[William Chen [:wchen]]

Comment on attachment 8683426 [details]
MozReview Request: Bug 1219030 - Collect notification management telemetry. p=ally, r?wchen,MattN

https://reviewboard.mozilla.org/r/24319/#review23215

::: dom/notification/Notification.cpp:680
(Diff revisions 2 - 3)
> -  if (!instance) {
> +  if (NS_WARN_IF(NS_FAILED(rv))) {

NS_ENSURE_SUCCESS(rv, rv) works here and it also warns on failure.

::: dom/notification/Notification.cpp:775
(Diff revisions 2 - 3)
>  NotificationTelemetryService::RecordDoNotDisturbSupported()

It looks like this always records DND supported = true or fails (which will fail service initialization as well and probably skip recording the other stats). Why was this changed from the previous version?

::: dom/notification/Notification.cpp:1638
(Diff revisions 2 - 3)
> +    static_cast<NotificationTelemetryService*>(telemetrySupports.get());

You don't need to use a RefPtr here, you can just use a raw ptr and the nsCOMPtr above will keep it alive.

Comment 20

[Lina Butler [:lina]]

https://reviewboard.mozilla.org/r/24319/#review23215

> It looks like this always records DND supported = true or fails (which will fail service initialization as well and probably skip recording the other stats). Why was this changed from the previous version?

Oops. I changed it because recording `false` for a flag histogram triggers an assertion. This shouldn't return errors, though.

Comment 21

[Lina Butler [:lina]]

Comment on attachment 8683426 [details]
MozReview Request: Bug 1219030 - Collect notification management telemetry. p=ally, r?wchen,MattN

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24319/diff/3-4/

Comment 22

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Edwin Wong [:edwong] from comment #12)
> NI: Jeff - can you provide color on what should be opt-in/out for each
> telemetry item.

Do you mean opted in by default ( aka fhr ) vs opted out by default ( aka old telemetry )? 

I think we should be conservative with measures turned on for all users, I think of the two following categories:

* top line feature usage, as mentioned in your doc
* metrics that tell us something actionable about the service, eg abuse indicators

I have a theory that we can use numbers from top line feature usage metrics to allow use to reason about all the other numbers collected, because we'll have something with which to adjust the numbers coming from users who manually opted in to data collection. I am going to talk to rweiss next week to ensure I'm not making stupid assumptions, and to get more advice generally on telemetry.

Comment 23

[William Chen [:wchen]]

Comment on attachment 8683426 [details]
MozReview Request: Bug 1219030 - Collect notification management telemetry. p=ally, r?wchen,MattN

https://reviewboard.mozilla.org/r/24319/#review23379

::: dom/notification/Notification.cpp:787
(Diff revision 4)
> +  Telemetry::Accumulate(Telemetry::NOTIFICATION_DO_NOT_DISTURB_SUPPORTED,

Did you intent to only record when DND is enabled and ignore when it's not?

::: dom/notification/Notification.cpp:1631
(Diff revision 4)
> +    return NS_ERROR_OUT_OF_MEMORY;

This line is unreachable. You can get rid of the if statement.

Comment 24

[Lina Butler [:lina]]

https://reviewboard.mozilla.org/r/24319/#review23379

> Did you intent to only record when DND is enabled and ignore when it's not?

I did. Recording `false` for a flag histogram hits this assertion: https://dxr.mozilla.org/mozilla-central/rev/abbd213422a560f1180c4ec6e3bf4792c2ea81ba/ipc/chromium/src/base/histogram.cc#1020 There's a separate boolean histogram for tracking whether the user actually enabled DND; this one is the denominator.

Comment 25

[Lina Butler [:lina]]

Comment on attachment 8683426 [details]
MozReview Request: Bug 1219030 - Collect notification management telemetry. p=ally, r?wchen,MattN

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24319/diff/4-5/

Comment 26

[Lina Butler [:lina]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9b769ff38ad

Comment 27

[Matthew N. [:MattN]]

Comment on attachment 8683426 [details]
MozReview Request: Bug 1219030 - Collect notification management telemetry. p=ally, r?wchen,MattN

https://reviewboard.mozilla.org/r/24319/#review23533

::: browser/components/preferences/in-content/content.js:294
(Diff revision 5)
> +    Services.telemetry.getHistogramById(
> +      "ALERT_NOTIFICATION_DO_NOT_DISTURB_ENABLED").add(isEnabled);

Nit: we normally prefer to wrap at the `.` instead.

::: toolkit/components/telemetry/Histograms.json:10062
(Diff revision 5)
> +  "ALERT_NOTIFICATION_SENDERS": {
> +    "alert_emails": ["[e-mail]"],
> +    "bug_numbers": [1219030],
> +    "expires_in_version": "48",
> +    "kind": "count",
> +    "description": "Number of origins that have shown a web notification. Excludes system alerts like update reminders and add-ons."
> +  },
> +  "ALERT_NOTIFICATION_DO_NOT_DISTURB_SUPPORTED": {

Looking at the names, I think the ALERT_NOTIFICATION prefix is too confusing and we should probably just have 2 different prefixes: WEB_NOTIFICATION for most and ALERT_SERVICE for stuff like DND.

Comment 28

[Lina Butler [:lina]]

Comment on attachment 8683426 [details]
MozReview Request: Bug 1219030 - Collect notification management telemetry. p=ally, r?wchen,MattN

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24319/diff/5-6/

Comment 29

[Lina Butler [:lina]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a28b602ef40

Comment 30

[Lina Butler [:lina]]

Comment on attachment 8683426 [details]
MozReview Request: Bug 1219030 - Collect notification management telemetry. p=ally, r?wchen,MattN

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24319/diff/6-7/

Comment 31

[Lina Butler [:lina]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8f9ff40b37cc

Comment 32

[Matthew N. [:MattN]]

Comment on attachment 8683426 [details]
MozReview Request: Bug 1219030 - Collect notification management telemetry. p=ally, r?wchen,MattN

https://reviewboard.mozilla.org/r/24319/#review24087

::: browser/components/preferences/in-content/content.js:294
(Diff revision 6)
> +    Services.telemetry
> +            .getHistogramById("ALERTS_SERVICE_DO_NOT_DISTURB_ENABLED")
> +            .add(isEnabled);

I think this should be done from within the setter so we don't miss future UI

::: dom/notification/Notification.cpp:677
(Diff revision 6)
> +NotificationTelemetryService::Init()
> +{
> +  nsresult rv = AddPermissionChangeObserver();
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  RecordDoNotDisturbSupported();

I think this isn't going to catch DND being supported due to a fallback during the first showing. If we move this to record after the first showing e.g. alertshow we would get more accurate numbers.

::: dom/notification/Notification.cpp:791
(Diff revision 6)
> +nsresult
> +NotificationTelemetryService::RecordSender(Notification* aNotification)
> +{
> +  nsCOMPtr<nsIPrincipal> principal = aNotification->GetPrincipal();
> +  if (!principal) {

I think this method should bail if the equivalent of Services.telemetry.canRecord(Base|Extended) is false. (AFAICT Base = opt-out, Extended = opt-in)

::: dom/notification/Notification.cpp:1428
(Diff revision 6)
>    } else if (!strcmp("alertshow", aTopic)) {
> +    Unused << NS_WARN_IF(NS_FAILED(notification->RecordSender()));
>      notification->DispatchTrustedEvent(NS_LITERAL_STRING("show"));

Note that this won't record if DND is on with XUL. Not sure if that's what we want for this probe since I thought it was more about the popularity of the web feature, not the user experience (though it currently matches the description).

::: toolkit/components/telemetry/Histograms.json:10069
(Diff revision 6)
> +  "WEB_NOTIFICATIONS_SENDERS": {
> +    "alert_emails": ["[e-mail]"],

Nit: remove the "s" on Notifications and sort the additions alphabetically

::: toolkit/components/telemetry/Histograms.json:10072
(Diff revision 6)
> +    "expires_in_version": "48",

Is 48 the standard amount for a probe landing now?

::: toolkit/components/telemetry/Histograms.json:10076
(Diff revision 6)
> +  "ALERTS_SERVICE_DO_NOT_DISTURB_SUPPORTED": {
> +    "alert_emails": ["[e-mail]"],
> +    "bug_numbers": [1219030],
> +    "expires_in_version": "48",
> +    "kind": "flag",

"Flag-type histograms should have the suffix "\_FLAG" in their name."

Comment 33

[Lina Butler [:lina]]

Comment on attachment 8683426 [details]
MozReview Request: Bug 1219030 - Collect notification management telemetry. p=ally, r?wchen,MattN

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24319/diff/7-8/

Comment 34

[Lina Butler [:lina]]

Comment on attachment 8683426 [details]
MozReview Request: Bug 1219030 - Collect notification management telemetry. p=ally, r?wchen,MattN

Just realized I wasn't recording senders for worker or service worker notifications. Fixed that, moved "DND enabled" telemetry to the setter, and added a call to record the sender if XUL DND is enabled.

Re-requesting review to make sure I didn't miss anything.

Comment 35

[Lina Butler [:lina]]

Comment on attachment 8683426 [details]
MozReview Request: Bug 1219030 - Collect notification management telemetry. p=ally, r?wchen,MattN

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24319/diff/8-9/

Comment 36

[William Chen [:wchen]]

Comment on attachment 8683426 [details]
MozReview Request: Bug 1219030 - Collect notification management telemetry. p=ally, r?wchen,MattN

https://reviewboard.mozilla.org/r/24319/#review24537

Comment 37

[Matthew N. [:MattN]]

Comment on attachment 8683426 [details]
MozReview Request: Bug 1219030 - Collect notification management telemetry. p=ally, r?wchen,MattN

https://reviewboard.mozilla.org/r/24319/#review25491

Apologies for the delay

Comment 38

[Lina Butler [:lina]]

https://hg.mozilla.org/integration/fx-team/rev/ba5dfdb479515bbf0db38094cc8d4906457a63da
Bug 1219030 - Collect notification management telemetry. r=wchen,MattN; p=ally

Comment 39

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ba5dfdb47951

Comment 40

[Edwin Wong [:edwong]]

from speaking with kit, we should try to uplift to beta chan

Comment 41

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Edwin Wong [:edwong] from comment #40)
> from speaking with kit, we should try to uplift to beta chan

+1

Comment 42

[Lina Butler [:lina]]

Comment on attachment 8683426 [details]
MozReview Request: Bug 1219030 - Collect notification management telemetry. p=ally, r?wchen,MattN

Approval Request Comment
[Feature/regressing bug #]: This bug.
[User impact if declined]: No insight into how users react to the new notifications.
[Describe test coverage new/current, TreeHerder]: None. We'll create dashboards once we have some initial data.
[Risks and why]: Low risk, telemetry addition.
[String/UUID change made/needed]: None.

Comment 43

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

Kit, Jeff: While I understand that getting telemetry data on push notifications is a good idea, I am feeling a bit concerned about the timing of this uplift. We are almost half way through the Beta44 cycle and this patch seems quite extensive. Can we let this one ride the trains from Aurora45 -> Beta45? This would mean getting the data from Beta users in ~3 weeks. Also if the dashboard is not ready for reading/using this data, perhaps it makes sense to wait it out. Thoughts?

Comment 44

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8683426 [details]
MozReview Request: Bug 1219030 - Collect notification management telemetry. p=ally, r?wchen,MattN

Same as Ritu for beta! For aurora, taking it to give you more data quickly.

Comment 45

[Lina Butler [:lina]]

Sounds reasonable. The probes are opt-in, too, so ISTM we'd get more interesting numbers from Aurora. Out of curiosity, do you know how many users actually opt in on Beta/Release?

Comment 46

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Ritu Kothari (:ritu) from comment #43)
> Kit, Jeff: While I understand that getting telemetry data on push
> notifications is a good idea, I am feeling a bit concerned about the timing
> of this uplift. We are almost half way through the Beta44 cycle and this
> patch seems quite extensive. Can we let this one ride the trains from
> Aurora45 -> Beta45? This would mean getting the data from Beta users in ~3
> weeks. Also if the dashboard is not ready for reading/using this data,
> perhaps it makes sense to wait it out. Thoughts?

Sure. We don't expect a lot of usage in 44 anyway.

Comment 47

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/95c408391907