1053746 - Add telemetry probes for OpenH264 plugin

Status: VERIFIED FIXED

(Firefox Health Report Graveyard :: Client: Desktop, defect)

Description

[Georg Fritzsche [:gfritzsche]]

We should add telemetry probes for:
* the installed plugin version
* whether the plugin is enabled
* (whether autoupdate is enabled for the plugin?)

Comment 1

[Maire Reavy [:mreavy]]

Should we plan to uplift this to Fx33?

Comment 2

[Benjamin Smedberg]

Yes.

Comment 3

[Georg Fritzsche [:gfritzsche]]

Gavin, can we get this in the next iteration?

Comment 4

[Georg Fritzsche [:gfritzsche]]

The probes for version and enabled state can be added in the OpenH264Provider:
http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/OpenH264Provider.jsm

We could submit the version using TelemetryLog (see e.g. bug 981851).
See here to get started for the other telemetry probe:
https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Adding_a_new_Telemetry_probe

Comment 5

[Marco Mucci [:MarcoM]]

Hi Georg, this bug is currently part of the IT 35.1 priority sheet.

(In reply to Georg Fritzsche [:gfritzsche] from comment #3)
> Gavin, can we get this in the next iteration?

Comment 6

[Benjamin Smedberg]

Attachment: 1053746-openh264-telemetry

I don't think this needs automated tests at this point, and mocking that would be pretty hard.

Comment 7

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8488841 [details] [diff] [review]
1053746-openh264-telemetry

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

I don't think its too hard to set up a test for this, see here for a template:
http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/xpcshell/test_openh264.js
But given that we want the measurements soon, we could do a follow-up.

::: toolkit/mozapps/extensions/internal/OpenH264Provider.jsm
@@ +271,5 @@
> +      userDisabled: OpenH264Wrapper.userDisabled,
> +      version: OpenH264Wrapper.version,
> +      applyBackgroundUpdates: OpenH264Wrapper.applyBackgroundUpdates,
> +    };
> +    AddonManagerPrivate.setTelemetryDetails("GMP", telemetry);

Do we only want to update this once on startup?
If we always want the latest data in the ping, we would need to trigger submissions from onPrefEnabledChanged(), onPrefVersionChanged() and when the auto-update setting changes.

Comment 8

[Benjamin Smedberg]

I don't think we should update this after startup, at least until we've figured out how to deal with telemetry environments.

Comment 9

[Benjamin Smedberg]

Attachment: 1053746-openh264-telemetry

Substantially the same, except I'm now honoring the provider.enabled pref so that this doesn't show up for non-Firefox apps. I think we should punt automated testing to the bug which will deal with telemetry environment changes.

Comment 10

[Georg Fritzsche [:gfritzsche]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #8)
> I don't think we should update this after startup, at least until we've
> figured out how to deal with telemetry environments.

Hm, but given long-running sessions, we will never see them as having OpenH264 installed (nor them updating etc.).
Is that really acceptable?

Comment 11

[Benjamin Smedberg]

Yes, I think it is. For now telemetry seems to record the state of Firefox at the beginning of the session. If I'm reading the XPIProvider code, that's roughly what happens there also (although it doesn't record the enabled state, and upgrades might change some of the metadata).

Comment 12

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8490274 [details] [diff] [review]
1053746-openh264-telemetry

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

Ok then.
I'm not happy about the missing testing though and think we should do a follow-up so we don't accidentally break this.

Comment 13

[Georg Fritzsche [:gfritzsche]]

Looks like this was not landed yet?

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 15

[Benjamin Smedberg]

I'd like to uplift this to 34, but waiting for QA verification first.

Comment 16

[Nils Ohlmeier [:drno]]

Verified with 35.0a1 (2014-10-10) on Mac OSX that I see a new entry in the add-ons section of telemetry for the openH264 add-on, which shows the expected three values and only the values from start-up time of FF.

Not setting status verified as I understand there is an uplift coming.

Comment 17

[Benjamin Smedberg]

Comment on attachment 8490274 [details] [diff] [review]
1053746-openh264-telemetry

Approval Request Comment
[Feature/regressing bug #]: OpenH264 is a new feature, this measures OpenH264 install rates
[User impact if declined]: we won't know whether OpenH264 is installing propertly in the field
[Describe test coverage new/current, TBPL]: manual verification
[Risks and why]: I believe this is very low risk
[String/UUID change made/needed]: none

Comment 18

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8490274 [details] [diff] [review]
1053746-openh264-telemetry

Aurora+

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/39a9012afc3c
https://hg.mozilla.org/releases/mozilla-aurora/rev/5e5b1ec4a370

Comment 20

[Bogdan Maris, Desktop Test Engineering]

Verified with latest Aurora on Mac OS X 10.9.5, Windows 7 64bit and Ubuntu 14.04 32bit. 'gmp-gmpopenh264' appears in Firefox Health Report together with the three expected values: version, userDisabled, applyBackgroundUpdates. Setting the status of the bug to Verified since it has been verified on 34 and 35 as well.