1225102 - Stop recording addon details for GMP plugins

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P4)

Description

[Ada [:adalucinet]]

Attachment: Addon Details Telemetry Data.png

Reproducible with 43.0b3 build 2 (Build ID: 20151112144305), latest 44.0a2 and 45.0a1 (from 2015-11-15) e10s on/off
Affected platforms: Windows 10 64-bit, Ubuntu 12.04 32-bit and Mac OS X 10.10

Steps to reproduce:
1. Launch Firefox and wait for the gmp plugins to install via Add-ons Manager.
2. Refresh the page.
3. In a New Tab, go to about:telemetry and focus Add-on Details section.

Expected results: Details are updated upon refresh.

Actual results: Details are not updated unless the browser is restarted.

Additional details:
1. For example, about:telemetry/Environment Data is updated after refreshing the page - the activeGMPlugins entry for both openh264 and eme plugins.  
2. gmp-eme-adobe is displayed on Ubuntu and OS X about:telemetry/Add-on Details, although the plugin is disabled and not present in Add-ons Manager.

Comment 1

[Georg Fritzsche [:gfritzsche]]

This bug here is about the issue with the Addon Details section.
We might actually drop "Addon Details" when we have time to investigate how to power its use-case from the environment data.

Comment 2

[Georg Fritzsche [:gfritzsche]]

Maire, do you know if anyone uses this data for GMP at all?
For active GMPs we have the same data in the environment, so i'm inclined to just remove this data.

In about:telemetry, compare:
* "Environment Data" - "addons" - "activeGMPlugins" (want to keep)
* "Add-on Details" - "GMP Provider" (inclined to remove)

Comment 3

[Maire Reavy [:mreavy]]

We (WebRTC) are not; forwarding NI to cpearce to see if he's using it.
See comment 2

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

I'm not using it. How do I look at this telemetry on telemetry.mozilla.org? What's it keyed under?

Comment 5

[Georg Fritzsche [:gfritzsche]]

It is not displayed there. If you are interested in viewing the data, you can currently e.g. go for it self-serve.
Starter documentation can be found at e.g. [0] & [1], but we can talk over the best option via #telemetry, mail or Vidyo if you want.

I'll make this bug about removing the GMP info from addonDetails as it duplicates the data found in the environment.

0: http://robertovitillo.com/2015/01/16/next-gen-data-analysis-framework-for-telemetry/
1: https://github.com/vitillo/telemetry-onboarding

Comment 6

[Georg Fritzsche [:gfritzsche]]

This is about removing the use of the AddonManager Telemetry details from the GMPProvider:
https://dxr.mozilla.org/mozilla-central/rev/4e2224c009dfedfcd95035e2fc67779567c2cdea/toolkit/mozapps/extensions/internal/GMPProvider.jsm#540

We need to remove that call and the code building the - now unused - |telemetry| object.
To verify the changes didn't break anything, run the following test:
mach test toolkit/mozapps/extensions/test/xpcshell/test_gmpProvider.js

Comment 7

[Nils M.]

I would volunteer for this bug. But I have to admit that this would be my first bug and I may have one or the other question.

Comment 8

[Georg Fritzsche [:gfritzsche]]

That is fine - do you have a working build already?
From there i would make sure that the test runs through fine without any changes (and maybe confirm that the GMP plugins show up in the addon details in about:telemetry).
Then try to make the change, build and see whether the test and about:telemetry behave as expected.

Comment 9

[Nils M.]

Attachment: Removed GMP Provider details from Add-on Details section on 'about:telemetry'

Comment 10

[Nils M.]

Ok, build environment is set up. I built Firefox completely and made sure that the "GMP Provider" is listed beneath the "Add-on Details" section. Tests ran also successfully. After the changes the "GMP Provider" is not longer displayed and tests are still successful. I created a pacth with mercurial and attached it to this bug.

Comment 11

[Georg Fritzsche [:gfritzsche]]

Thanks Nils, taking a look.

Comment 12

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8709182 [details] [diff] [review]
Removed GMP Provider details from Add-on Details section on 'about:telemetry'

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

This looks good, only two style comments below.

A minor issue: The patch message doesn't seem quite right: The details showing up in the "Add-on Details" is a side-effect of the change, i would suggest something like "Stop recording addon telemetry details for GMP plugins" instead.

Once you made the changes, you can upload a new patch and flag me for review on it (that way i get mail about a pending review etc.).

::: toolkit/mozapps/extensions/internal/GMPProvider.jsm
@@ +576,5 @@
>            this._log.warn("startup - adding gmp directory failed with " +
>                           e.name + " - sandboxing not available?", e);
>          }
>        }
>  

Nit: We don't need an empty line here, lets remove it.

@@ +594,5 @@
>        } catch (e) {
>          this._log.warn("startup - adding clearkey CDM failed", e);
>        }
>      }
> +    

Nit: Redundant line with trailing whitespace - lets remove this line.

Comment 13

[Nils M.]

Attachment: This patch stops recording addon telemetry details for GMP plugins

Included remarks from Georg

Comment 14

[Nils M.]

Thanks for your feedback, Georg. I added a new patch - ready for review :).

Comment 15

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8710157 [details] [diff] [review]
This patch stops recording addon telemetry details for GMP plugins

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

This looks good, thanks :)
One more request: For the patch message, "This patch" seems redundant.
Let's change it to "Stop recording ...", then we can get it landed.

Comment 16

[Nils M.]

The commit message in the patch is "Bug 1225102 - Stop recording addon telemetry details for GMP plugins". It's the description of the attachment which is 'This patch stops recording addon telemetry details for GMP plugins'. Should I nonetheless create a new attachment? On more question: is it you, Georg, who commits the patch?

Comment 17

[Georg Fritzsche [:gfritzsche]]

Sorry Nils, i didn't see the question here. If you request needinfo below ("Need more information from ...") then those questions don't get lost.

Yes, i mixed up the messages apparently. The message looks good.
I'll set "checkin-needed" for the keywords. If you do that that signals to other people with the rights to land it (me or others) that it should be checked in.

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/e93d49248f69

Comment 19

[Carsten Book [:Tomcat]]

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