Closed Bug 1219030 Opened 4 years ago Closed 4 years ago

Add telemetry for notification management

Categories

(Core :: DOM: Push Notifications, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox44 --- affected
firefox45 --- fixed
firefox46 --- fixed

People

(Reporter: Lina, Assigned: Lina)

References

Details

Attachments

(1 file, 1 obsolete file)

* 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.
Blocks: 1204608
No longer depends on: 1204608
Bug 1219030, Part 1 - Collect notification display telemetry.
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.
Attachment #8683426 - Flags: feedback?(wchen)
Attachment #8683426 - Flags: feedback?(MattN+bmo)
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.
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": ["fx-team@mozilla.com"],

I think this list has over a hundred people on it and has high SNR currently so probably not a good list. I think firefox-dev@mozilla.org 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…
Attachment #8683426 - Flags: feedback?(MattN+bmo) → feedback+
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.
Attachment #8683426 - Flags: feedback?(wchen)
Attachment #8683426 - Flags: feedback+
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/
Bug 1219030, Part 2 - Collect notification management UI telemetry.
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?
Attachment #8683426 - Flags: feedback?(wchen)
Attachment #8683426 - Flags: feedback?(ally)
Attachment #8687514 - Flags: feedback?(ally)
https://reviewboard.mozilla.org/r/24319/#review22815

::: toolkit/components/telemetry/Histograms.json:10039
(Diff revision 2)
> +  },
> +  "NOTIFICATION_SHOWN": {
> +    "alert_emails": ["firefox-dev@mozilla.org"],

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": ["firefox-dev@mozilla.org"],
> +    "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.
Status: NEW → ASSIGNED
https://reviewboard.mozilla.org/r/24319/#review22885

::: toolkit/components/telemetry/Histograms.json:10041
(Diff revision 2)
> +    "alert_emails": ["firefox-dev@mozilla.org"],

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.
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.
Attachment #8683426 - Flags: feedback?(ally)
Attachment #8687514 - Flags: feedback?(ally)
NI: Jeff - can you provide color on what should be opt-in/out for each telemetry item.
Flags: needinfo?(jgriffiths)
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.
Attachment #8683426 - Flags: feedback?(wchen)
Flags: needinfo?(jgriffiths)
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".
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.
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 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/
Attachment #8683426 - Attachment description: MozReview Request: Bug 1219030, Part 1 - Collect notification display telemetry. → MozReview Request: Bug 1219030 - Collect notification management telemetry. p=ally, r?MattN,wchen
Attachment #8683426 - Flags: review?(wchen)
Attachment #8683426 - Flags: review?(MattN+bmo)
Attachment #8687514 - Attachment is obsolete: true
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.
Attachment #8683426 - Flags: review?(wchen)
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 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/
Attachment #8683426 - Flags: review?(wchen)
(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 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.
Attachment #8683426 - Flags: review?(wchen) → review+
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 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/
Attachment #8683426 - Attachment description: MozReview Request: Bug 1219030 - Collect notification management telemetry. p=ally, r?MattN,wchen → MozReview Request: Bug 1219030 - Collect notification management telemetry. p=ally, r=wchen, r?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": ["firefox-dev@mozilla.org"],
> +    "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.
Attachment #8683426 - Flags: review?(MattN+bmo)
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/
Attachment #8683426 - Flags: review?(MattN+bmo)
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 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": ["firefox-dev@mozilla.org"],

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": ["firefox-dev@mozilla.org"],
> +    "bug_numbers": [1219030],
> +    "expires_in_version": "48",
> +    "kind": "flag",

"Flag-type histograms should have the suffix "\_FLAG" in their name."
Attachment #8683426 - Flags: review?(MattN+bmo) → review+
Attachment #8683426 - Attachment description: MozReview Request: Bug 1219030 - Collect notification management telemetry. p=ally, r=wchen, r?MattN → MozReview Request: Bug 1219030 - Collect notification management telemetry. p=ally, r?wchen,MattN
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 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.
Attachment #8683426 - Flags: review?(wchen)
Attachment #8683426 - Flags: review?(MattN+bmo)
Attachment #8683426 - Flags: review+
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 on attachment 8683426 [details]
MozReview Request: Bug 1219030 - Collect notification management telemetry. p=ally, r?wchen,MattN

https://reviewboard.mozilla.org/r/24319/#review24537
Attachment #8683426 - Flags: review?(wchen) → review+
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
Attachment #8683426 - Flags: review?(MattN+bmo) → review+
https://hg.mozilla.org/integration/fx-team/rev/ba5dfdb479515bbf0db38094cc8d4906457a63da
Bug 1219030 - Collect notification management telemetry. r=wchen,MattN; p=ally
https://hg.mozilla.org/mozilla-central/rev/ba5dfdb47951
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
from speaking with kit, we should try to uplift to beta chan
Flags: needinfo?(kcambridge)
(In reply to Edwin Wong [:edwong] from comment #40)
> from speaking with kit, we should try to uplift to beta chan

+1
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.
Flags: needinfo?(kcambridge)
Attachment #8683426 - Flags: approval-mozilla-beta?
Attachment #8683426 - Flags: approval-mozilla-aurora?
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?
Flags: needinfo?(kcambridge)
Flags: needinfo?(jgriffiths)
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.
Attachment #8683426 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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?
Flags: needinfo?(kcambridge)
(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.
Flags: needinfo?(jgriffiths)
Attachment #8683426 - Flags: approval-mozilla-beta?
You need to log in before you can comment on or make changes to this bug.