1351034 - Remove Expired Telemetry Histograms WEB_NOTIFICATION_PERMISSION_REMOVED, WEB_NOTIFICATION_SENDERS

Status: RESOLVED FIXED

(Core :: DOM: Push Subscriptions, enhancement, P3)

Description

[Chris H-C :chutten]

The Telemetry histograms WEB_NOTIFICATION_PERMISSION_REMOVED, WEB_NOTIFICATION_SENDERS expired in Firefox 50. Since they expired they are no longer collecting or transmitting information, so they can be removed.

This bug is for the removal of the code accumulating to or otherwise using the histograms, any tests that include the histograms, the definition of the histograms in toolkit/components/telemetry/Histograms.json, and their presences in histograms-whitelist.json (if applicable).

Due to how telemetry.mozilla.org currently works, once we remove the definitions from Histograms.json, even historic data on TMO will stop being available. Generally speaking, this isn't an issue with most probe removals, but if you suspect that it will be a problem with this one, it would be a good idea to mention it.

===
This bug was created with the template mzl.la/2odQI44
Please feel free to create your own expired-probe-removal bugs!

Comment 1

[Lina Butler [:lina]]

Bryan, looping you in just in case:

* WEB_NOTIFICATION_PERMISSION_REMOVED counts the number of times a user manually changed permissions for a site in about:preferences#content, or in the control center. (Either removing the grant, or removing the block so the site can ask again). It's opt-in.

* WEB_NOTIFICATION_SENDERS is the number of unique origins that have shown a push or vanilla desktop notification. It's opt-out, so I think we had plans for it at one time?

Since these two probes already expired in 50, is it safe to assume they haven't been useful, and remove them?

Comment 2

[Bryan Clark (DevTools PM) [:clarkbw]]

I agree that I believe we had plans for WEB_NOTIFICATION_SENDERS which I cannot recall now.  We can remove anyway as I don't know what useful question this would answer beyond other metrics.

I don't need WEB_NOTIFICATION_PERMISSION_REMOVED and as an opt-in measure it has little value anyway.

Comment 3

[sanal5hd]

Hi Chris,
I am Santosh,from India.I want to resolve this bug.But as I am new,I don't know how to start working on it.After reading the comments by Kit and Bryan,I understood what WEB_NOTIFICATION_PERMISSION_REMOVED and WEB_NOTIFICATION_SENDERS is.But still I cannot find the repository where this bug is,so that I can contribute to it.It will be very helpful if you guide me a little.I want to know where the repository is,and where to look for this bug in that repository.
Thanks,
Santosh
([e-mail])

Comment 4

[Chris H-C :chutten]

Happy to help!

You may wish to begin by going through some of the Getting Started documentation on MDN: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction

That will give you a solid background of what things are and where you might be able to find them.

From there we have tools like DXR that allow you to search the mozilla-central tree very rapidly: https://dxr.mozilla.org/mozilla-central/search?q=WEB_NOTIFICATION_PERMISSION_REMOVED&=mozilla-central

For this bug I will expect two patches, one for removing each of the probes. You can use mozreview to put these patches up for review: http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview.html

If you have any questions, be sure to ask on #introduction or #developers or here on the bug.

Comment 5

[Chris H-C :chutten]

Have you had a chance to look into this bug? Will you have an opportunity in the near future? If not, no worries, I can just unassign this bug and someone else can take a look.

Comment 6

[Swapnesh Kumar Sahoo [:swapneshks]]

Hi! Can I work on this bug?
I am a beginner and have worked on a few moz bugs but still catching up to things...
I see there is sufficient info available in the description and comments. 
So, once I get the green signal, I can start working on it!

Comment 7

[Chris H-C :chutten]

Please go ahead, and let me know if you have any questions.

Comment 8

[Swapnesh Kumar Sahoo [:swapneshks]]

Attachment: Remove WEB_NOTIFICATION_PERMISSION_REMOVED

Comment 9

[Swapnesh Kumar Sahoo [:swapneshks]]

Attachment: Remove WEB_NOTIFICATION_SENDERS

Comment 10

[Swapnesh Kumar Sahoo [:swapneshks]]

Hi Chris,

I have uploaded 2 separate patches, one for each of the probes. 
Do let me know if I missed anything..

Comment 11

[Chris H-C :chutten]

Comment on attachment 8865497 [details] [diff] [review]
Remove WEB_NOTIFICATION_PERMISSION_REMOVED

Review of attachment 8865497 [details] [diff] [review]:
-----------------------------------------------------------------

Redirecting to domain expert. Looks good from Telemetry's perspective.

Comment 12

[Lina Butler [:lina]]

Comment on attachment 8865497 [details] [diff] [review]
Remove WEB_NOTIFICATION_PERMISSION_REMOVED

Review of attachment 8865497 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great, thanks for doing this, Swapnesh! If you're up for it, you can remove the entire `Observe` method (and the `nsIObserver` implementation), `AddPermissionChangeObserver`, and `RemovePermissionChangeObserver`. I don't think we need it anymore, since we're not recording telemetry for permission changes. If not, no problem; I'll file a follow-up bug.

Comment 13

[Lina Butler [:lina]]

Comment on attachment 8865498 [details] [diff] [review]
Remove WEB_NOTIFICATION_SENDERS

Review of attachment 8865498 [details] [diff] [review]:
-----------------------------------------------------------------

`RecordSender()` and `mOrigins` can be removed, too, if you're up for it.

Comment 14

[Swapnesh Kumar Sahoo [:swapneshks]]

Sure, no issues! I'll do that and upload a new set of patches.

Comment 15

[Swapnesh Kumar Sahoo [:swapneshks]]

Attachment: Remove WEB_NOTIFICATION_PERMISSION_REMOVED and related usages

Comment 16

[Swapnesh Kumar Sahoo [:swapneshks]]

Attachment: Remove WEB_NOTIFICATION_SENDERS and related usages

Comment 17

[Swapnesh Kumar Sahoo [:swapneshks]]

New set of patches have been uploaded. Let me know if I missed any instances.

Comment 18

[Lina Butler [:lina]]

Comment on attachment 8865577 [details] [diff] [review]
Remove WEB_NOTIFICATION_PERMISSION_REMOVED and related usages

Review of attachment 8865577 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, though I think you'll want to change `NotificationTelemetryService` to inherit from `nsISupports`, and change the macro to `NS_IMPL_ISUPPORTS(NotificationTelemetryService, nsISupports)`. Could I see another patch with those changes, please?

Comment 19

[Lina Butler [:lina]]

Comment on attachment 8865580 [details] [diff] [review]
Remove WEB_NOTIFICATION_SENDERS and related usages

Review of attachment 8865580 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good!

Comment 20

[Swapnesh Kumar Sahoo [:swapneshks]]

Attachment: Remove WEB_NOTIFICATION_PERMISSION_REMOVED and related usages

Requested changes have been incorporated in this patch.

Comment 21

[Lina Butler [:lina]]

Comment on attachment 8865908 [details] [diff] [review]
Remove WEB_NOTIFICATION_PERMISSION_REMOVED and related usages

Thanks! I fixed up commit messages and pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d05d6c0124e506016b0e07dacab3b405241436dd

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3eaa4db163b9
Remove WEB_NOTIFICATION_PERMISSION_REMOVED. f=chutten; r=kitcambridge
https://hg.mozilla.org/integration/mozilla-inbound/rev/50453d163a13
Remove usages associated to WEB_NOTIFICATION_SENDERS. f=chutten; r=kitcambridge

Comment 23

[Lina Butler [:lina]]

Thanks again, Swapnesh!

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3eaa4db163b9
https://hg.mozilla.org/mozilla-central/rev/50453d163a13

Comment 25

[Swapnesh Kumar Sahoo [:swapneshks]]

(In reply to Kit Cambridge (he/him) [:kitcambridge] (UTC-8) from comment #23)
> Thanks again, Swapnesh!

My pleasure! :)