Closed Bug 722110 Opened 8 years ago Closed 4 years ago

Telemetry for plugin use

Categories

(Core :: Plug-ins, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox42 --- fixed
firefox43 --- fixed
firefox44 --- fixed

People

(Reporter: jruderman, Assigned: qdot)

References

Details

(Keywords: sec-want, Whiteboard: [sg:want?][adv-main42-])

Attachments

(3 files, 2 obsolete files)

No description provided.
Whiteboard: [sg:want?]
Assignee: nobody → kyle
Duplicate of this bug: 940163
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.
Duplicate of this bug: 1199571
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.
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.
Summary: Telemetry for plugin installation, version, and use → Telemetry for plugin use
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
Attachment #8665595 - Flags: review?(vladan.bugzilla)
Attachment #8665595 - Flags: review?(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.
Attachment #8665595 - Flags: review?(cpeterson) → review+
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 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
Attachment #8665595 - Flags: review?(vladan.bugzilla)
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.
(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?
Flags: needinfo?(vladan.bugzilla)
Flags: needinfo?(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.
Flags: needinfo?(cpeterson) → needinfo?(benjamin)
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.
Flags: needinfo?(benjamin)
(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.
Flags: needinfo?(vladan.bugzilla)
Added email/stop after version, changed key to use plugin nice name.
Attachment #8665595 - Attachment is obsolete: true
Attachment #8666941 - Flags: review?(vladan.bugzilla)
Attachment #8666941 - Flags: review?(cpeterson)
Attachment #8666941 - Flags: review?(vladan.bugzilla) → review+
Comment on attachment 8666941 [details] [diff] [review]
Patch 1 (v3) - Add plugin activation telemetry probe

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

LGTM
Attachment #8666941 - Flags: review?(cpeterson) → review+
https://hg.mozilla.org/mozilla-central/rev/91a8ceacc131
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
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
Attachment #8666941 - Flags: approval-mozilla-beta?
Attachment #8666941 - Flags: approval-mozilla-aurora?
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.
Attachment #8666941 - Flags: approval-mozilla-beta?
Attachment #8666941 - Flags: approval-mozilla-beta+
Attachment #8666941 - Flags: approval-mozilla-aurora?
Attachment #8666941 - Flags: approval-mozilla-aurora+
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!
Flags: needinfo?(kyle)
Ok, give this one a try. Applied cleanly to aurora for me.
Flags: needinfo?(kyle) → needinfo?(cbook)
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.
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
Flags: needinfo?(cbook) → needinfo?(kyle)
Ok, made a fixed beta version too.
Flags: needinfo?(kyle) → needinfo?(cbook)
Flags: needinfo?(cbook)
Whiteboard: [sg:want?] → [sg:want?][adv-main42-]
Depends on: 1260065
You need to log in before you can comment on or make changes to this bug.