Closed Bug 1225102 Opened 5 years ago Closed 5 years ago

Stop recording addon details for GMP plugins


(Toolkit :: Telemetry, defect, P4)




Tracking Status
firefox45 --- affected
firefox47 --- fixed


(Reporter: adalucinet, Assigned: nm.mozilla, Mentored)


(Whiteboard: [measurement:client] [lang=js] [good first bug])


(2 files, 1 obsolete file)

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.
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.
Priority: -- → P4
Whiteboard: [measurement:client]
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)
Flags: needinfo?(mreavy)
We (WebRTC) are not; forwarding NI to cpearce to see if he's using it.
See comment 2
Flags: needinfo?(mreavy) → needinfo?(cpearce)
I'm not using it. How do I look at this telemetry on What's it keyed under?
Flags: needinfo?(cpearce)
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.

This is about removing the use of the AddonManager Telemetry details from the GMPProvider:

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
Mentor: gfritzsche
Whiteboard: [measurement:client] → [measurement:client] [lang=js] [good first bug]
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.
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.
Mentor: alessio.placitelli
Summary: about:telemetry/Add-ons Details data is not accurate → Stop recording addon details for GMP plugins
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.
Thanks Nils, taking a look.
Assignee: nobody → nm.mozilla
Attachment #8709182 - Flags: review?(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 " +
>                  + " - 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.
Attachment #8709182 - Flags: review?(gfritzsche) → feedback+
Included remarks from Georg
Attachment #8710157 - Flags: review?(gfritzsche)
Thanks for your feedback, Georg. I added a new patch - ready for review :).
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.
Attachment #8710157 - Flags: review?(gfritzsche) → review+
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?
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.
Attachment #8709182 - Attachment is obsolete: true
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.