Closed Bug 1357460 Opened 3 years ago Closed 3 years ago

send isWebExtension along with Telemetry Environment data

Categories

(Toolkit :: Telemetry, defect, P1)

defect
Points:
1

Tracking

()

VERIFIED FIXED
mozilla55
Tracking Status
firefox54 --- verified
firefox55 --- verified

People

(Reporter: rhelmer, Assigned: rhelmer)

Details

(Whiteboard: [measurement:client])

Attachments

(2 files)

Currently we don't have a great way to determine in the telemetry environment data if an extension is a WebExtension or legacy.

There is a getter for the addon object that indicates this - `isWebExtension` - which we should start sending too.
Attachment #8859432 - Flags: review?(kmaglione+bmo)
Comment on attachment 8859432 [details]
Bug 1357460 - add isWebExtension to Telemetry Environment

https://reviewboard.mozilla.org/r/131458/#review134188

::: toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js:1235
(Diff revision 2)
>    }
>  
> +  // Check webextension add-on data.
> +  Assert.ok(WEBEXTENSION_ADDON_ID in data.addons.activeAddons, "We must have one active webextension addon.");
> +  let targetWebExtensionAddon = data.addons.activeAddons[WEBEXTENSION_ADDON_ID];
> +  for (let f in EXPECTED_WEBEXTENSION_ADDON_DATA) {

Nit: `for (let [key, val] of Object.entries(...))`
Attachment #8859432 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8859432 [details]
Bug 1357460 - add isWebExtension to Telemetry Environment

https://reviewboard.mozilla.org/r/131458/#review134236

This looks good, thanks! Please make sure this doesn't make the test_TelemetryEnvironment.js test intermittent by running the "X" test suite on treeherder a few times.
Attachment #8859432 - Flags: review?(alessio.placitelli) → review+
Points: --- → 13
Priority: -- → P1
Whiteboard: [measurement:client]
Comment on attachment 8859431 [details]
Bug 1357460 - add isWebExtension to Telemetry Environment collection docs

https://reviewboard.mozilla.org/r/131456/#review134310

data-r=me
Attachment #8859431 - Flags: review?(benjamin) → review+
Points: 13 → 1
Comment on attachment 8859432 [details]
Bug 1357460 - add isWebExtension to Telemetry Environment

https://reviewboard.mozilla.org/r/131458/#review134188

> Nit: `for (let [key, val] of Object.entries(...))`

I'd like to leave it to be consistent with the rest of these tests (the same way I used the deferred etc)
Comment on attachment 8859432 [details]
Bug 1357460 - add isWebExtension to Telemetry Environment

https://reviewboard.mozilla.org/r/131458/#review134236

Thanks! Ok will do so before landing, using the try run that's attached to mozreview (https://treeherder.mozilla.org/#/jobs?repo=try&revision=99e848053214&selectedJob=92571995)
Comment on attachment 8859432 [details]
Bug 1357460 - add isWebExtension to Telemetry Environment

https://reviewboard.mozilla.org/r/131458/#review134188

> I'd like to leave it to be consistent with the rest of these tests (the same way I used the deferred etc)

Can you fix the rest of the tests instead, then? :)
Comment on attachment 8859432 [details]
Bug 1357460 - add isWebExtension to Telemetry Environment

https://reviewboard.mozilla.org/r/131458/#review134188

> Can you fix the rest of the tests instead, then? :)

NO
Pushed by rhelmer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/de686e9e9a9a
add isWebExtension to Telemetry Environment collection docs r=bsmedberg
https://hg.mozilla.org/integration/autoland/rev/aa3ee3f6de39
add isWebExtension to Telemetry Environment r=Dexter,kmag
Comment on attachment 8859432 [details]
Bug 1357460 - add isWebExtension to Telemetry Environment

Approval Request Comment
[Feature/Bug causing the regression]: quantum performance metrics want to exclude people with old-style extensions from performance analysis gearing up to Firefox 57
[User impact if declined]: 
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: ensure that "activeAddons" in about:telemetry displays isWebExtension accurately for installed extensions
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]: no
[Why is the change risky/not risky?]: adds a small bit of telemetry to an existing data structure
[String changes made/needed]:
Attachment #8859432 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/de686e9e9a9a
https://hg.mozilla.org/mozilla-central/rev/aa3ee3f6de39
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Hi :rhelmer,
From comment #12, what's the user impact? Can we let this ride the train and won't fix in Beta54
Flags: needinfo?(rhelmer)
(In reply to Gerry Chang [:gchang] from comment #14)
> Hi :rhelmer,
> From comment #12, what's the user impact? Can we let this ride the train and
> won't fix in Beta54

Benjamin asked me to request uplift, I'll defer to him.
Flags: needinfo?(rhelmer) → needinfo?(benjamin)
Gerry, this is a data change only and therefore doesn't have user impact. This data is highly desirable for quantum performance metrics/baselines.
Flags: needinfo?(benjamin)
Comment on attachment 8859432 [details]
Bug 1357460 - add isWebExtension to Telemetry Environment

Per comment #16, this is for quantum performance metrics. Beta54+. Should be 54 beta 2.
Attachment #8859432 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Verified as fixed using scenarios from(on 55, tested only on Windows after talking with Dexter) : https://public.etherpad-mozilla.org/p/_Bug_1357460

Please let me know if any other test are needed before marking it as verified.
Flags: needinfo?(rhelmer)
Flags: needinfo?(kmaglione+bmo)
Test scenarios look good to me, thanks!
Flags: needinfo?(rhelmer)
Flags: needinfo?(kmaglione+bmo)
Georg - we've added this new field to the telemetry environment ("isWebextension") - do we need to do anything on the telemetry server side? Thanks!
Flags: needinfo?(gfritzsche)
We at least need to add this to the pipeline schemas [1].
Mark, is there anything else?

1: https://github.com/mozilla-services/mozilla-pipeline-schemas/
Flags: needinfo?(gfritzsche) → needinfo?(mreid)
Status: RESOLVED → VERIFIED
(In reply to Georg Fritzsche [:gfritzsche] from comment #22)
> We at least need to add this to the pipeline schemas [1].
> Mark, is there anything else?
> 
> 1: https://github.com/mozilla-services/mozilla-pipeline-schemas/

Let's get this field into the relevant derived datasets too in bug 1360174 and bug 1360177
Flags: needinfo?(mreid)
You need to log in before you can comment on or make changes to this bug.