Closed Bug 1053746 Opened 5 years ago Closed 5 years ago

Add telemetry probes for OpenH264 plugin

Categories

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

defect
Not set

Tracking

(firefox32 unaffected, firefox33 wontfix, firefox34+ verified, firefox35 verified)

VERIFIED FIXED
Firefox 35
Tracking Status
firefox32 --- unaffected
firefox33 --- wontfix
firefox34 + verified
firefox35 --- verified

People

(Reporter: gfritzsche, Assigned: benjamin, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [openh264-uplift] [lang=js])

Attachments

(1 file, 1 obsolete file)

We should add telemetry probes for:
* the installed plugin version
* whether the plugin is enabled
* (whether autoupdate is enabled for the plugin?)
Flags: firefox-backlog+
Should we plan to uplift this to Fx33?
Blocks: OpenH264
Yes.
Whiteboard: [openh264-uplift]
QA Whiteboard: [qa?]
QA Whiteboard: [qa?] → [qa+]
QA Whiteboard: [qa+]
Flags: qe-verify+
Gavin, can we get this in the next iteration?
Flags: needinfo?(gavin.sharp)
Mentor: georg.fritzsche
Whiteboard: [openh264-uplift] → [openh264-uplift] [lang=js]
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
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?
Flags: needinfo?(gavin.sharp)
Assignee: nobody → benjamin
Status: NEW → ASSIGNED
Iteration: --- → 35.1
QA Contact: drno
Attached patch 1053746-openh264-telemetry (obsolete) — Splinter Review
I don't think this needs automated tests at this point, and mocking that would be pretty hard.
Attachment #8488841 - Flags: review?(georg.fritzsche)
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.
Attachment #8488841 - Flags: review?(georg.fritzsche)
Iteration: 35.1 → 35.2
I don't think we should update this after startup, at least until we've figured out how to deal with telemetry environments.
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.
Attachment #8490274 - Flags: review?(georg.fritzsche)
Attachment #8488841 - Attachment is obsolete: true
(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?
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 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.
Attachment #8490274 - Flags: review?(georg.fritzsche) → review+
Looks like this was not landed yet?
Flags: needinfo?(benjamin)
Iteration: 35.2 → ---
https://hg.mozilla.org/mozilla-central/rev/bbbf658ef840
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
Iteration: --- → 35.3
I'd like to uplift this to 34, but waiting for QA verification first.
Flags: needinfo?(benjamin) → needinfo?(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.
Flags: needinfo?(drno)
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
Attachment #8490274 - Flags: approval-mozilla-aurora?
Comment on attachment 8490274 [details] [diff] [review]
1053746-openh264-telemetry

Aurora+
Attachment #8490274 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Status: RESOLVED → VERIFIED
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in before you can comment on or make changes to this bug.