722110 - Telemetry for plugin use

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, enhancement)

Comment 2

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

This has been requested in relation to some of the other plugin work I've been assigned (plugin whitelisting on win64, youtube link rewriting, etc), so will see about getting it done.

Comment 4

[Chris Peterson [:cpeterson]]

bsmedberg suggested that we create a dashboard to make tracking plugin use over time easier. After we're collecting plugin activation telemetry, we can find someone to work on the dashboard.

Comment 5

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Turns out we already get plugin installation and version as part of the activePlugins data. So we just need to record each activation of the plugin in this bug.

Comment 6

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v1) - WIP: Add plugin activation telemetry probe

Comment 7

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v2) - WIP: Add plugin activation telemetry probe

Comment 8

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8665595 [details] [diff] [review]
Patch 1 (v2) - WIP: Add plugin activation telemetry probe

Marking cpeterson for code review, vladan for data collection review

Comment 9

[Chris Peterson [:cpeterson]]

Comment on attachment 8665595 [details] [diff] [review]
Patch 1 (v2) - WIP: Add plugin activation telemetry probe

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

LGTM!

::: browser/modules/PluginContent.jsm
@@ +422,3 @@
>          break;
>  
>        case "PluginInstantiated":

I see this handle() function has a "PluginClickToPlay" case to set up the click-to-play UI. When a click-to-play plugin is activated, will this "PluginInstantiated" case run? We'd like to collect telemetry for both automatically activated and click-to-play activated plugins.

Comment 10

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Yup. Whenever click-to-play is clicked, we go through the loading path again, which means a new PluginBindingAttached event is fired, with the new status. I tested this by hand, can try to set up a mochitest for it if you'd like.

Comment 11

[Vladan Djeric (:vladan)]

Comment on attachment 8665595 [details] [diff] [review]
Patch 1 (v2) - WIP: Add plugin activation telemetry probe

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

::: browser/modules/PluginContent.jsm
@@ +422,5 @@
>          break;
>  
>        case "PluginInstantiated":
> +        let info = this._getPluginInfo(plugin);
> +        Services.telemetry.getKeyedHistogramById('PLUGIN_ACTIVATION_COUNT').add(info.pluginName + " " + info.pluginTag.version);

you're going to have a very long tail of keys, but maybe that's necessary. i mention this because our backend gets laggy when the dashboards request information on a keyed histogram with 100s of keys

::: toolkit/components/telemetry/Histograms.json
@@ +9655,5 @@
>      "expires_in_version": "55",
>      "kind": "count",
>      "description": "Attempt to notify ServiceWorker of push notification resubscription."
> +  },
> +  "PLUGIN_ACTIVATION_COUNT": {

add an alert_emails field so we know who owns this histogram and so you get notifications when the data distribution changes suddenly (once we add regression monitoring to *keyed* histograms as well)

@@ +9656,5 @@
>      "kind": "count",
>      "description": "Attempt to notify ServiceWorker of push notification resubscription."
> +  },
> +  "PLUGIN_ACTIVATION_COUNT": {
> +    "expires_in_version": "never",

should this really never expire? Can we set this to e.g. 5 versions in the future and then extend it if we need to (you'll get an email reminder before it expires)

@@ +9659,5 @@
> +  "PLUGIN_ACTIVATION_COUNT": {
> +    "expires_in_version": "never",
> +    "kind": "count",
> +    "keyed": true,
> +    "description": "Counts number of times a certain plugin has been activated."

You're going to use the telemetry dashboard to look at the aggregated data, right?
There are some misconceptions about how count histograms are aggregated, so I'll just summarize here: 0 counts aren't reported or shown in the dash. You can infer them from the total # of sessions reported if you need to. The dash shows the distribution of counts per session for sessions with counts > 0, e.g. for plugin with key=X, 70% of sessions had 1 plugin activation, 20% of sessions had 2 plugin activations, 10% of sessions had 3 activations, etc

Comment 12

[Benjamin Smedberg]

We shouldn't use pluginName/version for this. We should use the standard "nice name" from nsIPluginTag.niceName.

Also let's not include the version. If we do need to know the version, we can get it from the environment, but in general I think it will be noise.

Comment 13

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #11)
> Comment on attachment 8665595 [details] [diff] [review]
> Patch 1 (v2) - WIP: Add plugin activation telemetry probe
> 
> Review of attachment 8665595 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/modules/PluginContent.jsm
> @@ +422,5 @@
> >          break;
> >  
> >        case "PluginInstantiated":
> > +        let info = this._getPluginInfo(plugin);
> > +        Services.telemetry.getKeyedHistogramById('PLUGIN_ACTIVATION_COUNT').add(info.pluginName + " " + info.pluginTag.version);
> 
> you're going to have a very long tail of keys, but maybe that's necessary. i
> mention this because our backend gets laggy when the dashboards request
> information on a keyed histogram with 100s of keys

Actually, I believe I can remove the version, cpeterson said it wasn't really needed. ni?'ing him to make sure that's ok. We get plugin version elsewhere in the ping (as part of activePlugins), if we need to we can aggregate that way.

> 
> ::: toolkit/components/telemetry/Histograms.json
> @@ +9655,5 @@
> >      "expires_in_version": "55",
> >      "kind": "count",
> >      "description": "Attempt to notify ServiceWorker of push notification resubscription."
> > +  },
> > +  "PLUGIN_ACTIVATION_COUNT": {
> 
> add an alert_emails field so we know who owns this histogram and so you get
> notifications when the data distribution changes suddenly (once we add
> regression monitoring to *keyed* histograms as well)

Another question for cpeterson: You want your email as the alert email, or should I just put mine and forward you spikes/changes as needed?

> 
> @@ +9656,5 @@
> >      "kind": "count",
> >      "description": "Attempt to notify ServiceWorker of push notification resubscription."
> > +  },
> > +  "PLUGIN_ACTIVATION_COUNT": {
> > +    "expires_in_version": "never",
> 
> should this really never expire? Can we set this to e.g. 5 versions in the
> future and then extend it if we need to (you'll get an email reminder before
> it expires)

That's fine, I'll just set it to 48 right now.

> 
> @@ +9659,5 @@
> > +  "PLUGIN_ACTIVATION_COUNT": {
> > +    "expires_in_version": "never",
> > +    "kind": "count",
> > +    "keyed": true,
> > +    "description": "Counts number of times a certain plugin has been activated."
> 
> You're going to use the telemetry dashboard to look at the aggregated data,
> right?
> There are some misconceptions about how count histograms are aggregated, so
> I'll just summarize here: 0 counts aren't reported or shown in the dash. You
> can infer them from the total # of sessions reported if you need to. The
> dash shows the distribution of counts per session for sessions with counts >
> 0, e.g. for plugin with key=X, 70% of sessions had 1 plugin activation, 20%
> of sessions had 2 plugin activations, 10% of sessions had 3 activations, etc

Ah, ok. What we're looking for is aggregated number of activations, yes. We just want to know how many times each plugin was activated per session. Which would be the best histogram type to use for that?

Comment 14

[Chris Peterson [:cpeterson]]

(In reply to Kyle Machulis [:kmachulis] [:qdot] (USE NEEDINFO?) from comment #13)
> Actually, I believe I can remove the version, cpeterson said it wasn't
> really needed. ni?'ing him to make sure that's ok. We get plugin version
> elsewhere in the ping (as part of activePlugins), if we need to we can
> aggregate that way.

Agreed. The plugin version is not important. Some plugins have version numbers in their names (e.g. "Adobe Acrobat NPAPI Plug-in, Version 11.0.02" and "QuickTime Plug-in 7.7.3"). If we want to minimize the number of different names sent from the client, we could strip whitespace, numbers, and dots from the end of the plugin names like "Adobe Acrobat NPAPI Plug-in, Version 11.0.02" -> "Adobe Acrobat NPAPI Plug-in, Version".

@ Benjamin, is the niceName supposed to replace "Shockwave Flash" with "Adobe Flash Player"? I thought our plugin code had a hard-coded map of well-known "ugly names" to nice names, but I still see the ugly name "Shockwave Flash" in about:addons or about:plugins.

> Another question for cpeterson: You want your email as the alert email, or
> should I just put mine and forward you spikes/changes as needed?

Sure. You can use my email.

Comment 15

[Benjamin Smedberg]

The "nice" name is the one that shows up in prefs and the permission manager. it's "flash" and "java" and other plugins its based on the filename, excluding trailing version numbers, so e.g. plugin.state.npgoogleupdate

It's not really intended to be human-readable, but it's programmatically pretty precise and I think what we want here.

Comment 16

[Vladan Djeric (:vladan)]

(In reply to Kyle Machulis [:kmachulis] [:qdot] (USE NEEDINFO?) from comment #13)
> Ah, ok. What we're looking for is aggregated number of activations, yes. We
> just want to know how many times each plugin was activated per session.
> Which would be the best histogram type to use for that?

Ok, I think a keyed count histogram is indeed the right choice to answer this question.

Comment 17

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v3) - Add plugin activation telemetry probe

Added email/stop after version, changed key to use plugin nice name.

Comment 18

[Chris Peterson [:cpeterson]]

Comment on attachment 8666941 [details] [diff] [review]
Patch 1 (v3) - Add plugin activation telemetry probe

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

LGTM

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/91a8ceacc131

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/91a8ceacc131

Comment 21

[Chris Peterson [:cpeterson]]

Comment on attachment 8666941 [details] [diff] [review]
Patch 1 (v3) - Add plugin activation telemetry probe

Approval Request Comment
[Feature/regressing bug #]: None. This is a new telemetry probe.
[User impact if declined]: We will have less data on the use of plugins. Since Chrome is making Flash ads click-to-play, we want to gather lots of data soon so we can watch how quickly the use of Flash falls off. Ideally, we should have started tracking plugin use before Chrome announced their intention to click-to-play Flash ads.
[Describe test coverage new/current, TreeHerder]: This telemetry probe has been live in the Nightly channel for a few days. Here is an example of some of the plugin data we have seen on the Nightly channel so far: http://mzl.la/1O97x9c
[Risks and why]: Low risk. It is a simple telemetry probe.
[String/UUID change made/needed]: None

Comment 22

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8666941 [details] [diff] [review]
Patch 1 (v3) - Add plugin activation telemetry probe

Sure, anything to help killing flash is good :)
Should be in 42 beta 4.

Comment 23

[Carsten Book [:Tomcat]]

Hi, this cause problems when uplifting:

grafting 304174:91a8ceacc131 "Bug 722110 - Plugin Activation Telemetry Probe; r=cpeterson r=vladan"
merging toolkit/components/telemetry/Histograms.json
warning: conflicts during merge.
merging toolkit/components/telemetry/Histogram
could you take a look, thanks!

Comment 24

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (AURORA VERSION) - Add plugin activation telemetry probe; r=cpeterson r=vladan a=sylvestre

Ok, give this one a try. Applied cleanly to aurora for me.

Comment 25

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

BTW, I /think/ the original patch should apply to beta, but if it doesn't, let me know and I'll see what I can do.

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/719d1e176171

Comment 27

[Carsten Book [:Tomcat]]

Hi Kyle,

this fails on uplifting to beta with:

Hunk #1 FAILED at 9757
1 out of 1 hunks FAILED -- saving rejects to file toolkit/components/telemetry/Histograms.json.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and refresh 0001-Bug-722110-Plugin-Activation-Telemetry-Probe.patch

Comment 28

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (BETA VERSION) - Add plugin activation telemetry probe; r=cpeterson r=vladan a=sylvestre

Ok, made a fixed beta version too.

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c3aecf966271

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c3aecf966271