Closed Bug 1420908 Opened 6 years ago Closed 6 years ago

Remove Telemetry Experiments from browser/experiments, and toolkit/telemetry

Categories

(Toolkit :: Telemetry, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: chutten, Assigned: kmag)

References

Details

Attachments

(1 file)

Splitting the work from bug 1415284, this covers the removing the non-addons-manager code handing Telemetry Experiments.
Priority: -- → P3
Blocks: 1450834
Comment on attachment 8964714 [details]
Bug 1420908: Remove telemetry experiments.

https://reviewboard.mozilla.org/r/233430/#review239018


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/components/telemetry/docs/fhr/dataformat.rst:1900
(Diff revision 1)
>  ::
>  
>      "org.mozilla.uitour.treatment": {
>        "_v": 1,
>        "treatment": [
>          "optin",

Warning: Optin  ==> option [codespell]
Blocks: 1450801
Comment on attachment 8964714 [details]
Bug 1420908: Remove telemetry experiments.

https://reviewboard.mozilla.org/r/233430/#review239074


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/components/telemetry/docs/fhr/dataformat.rst:1900
(Diff revision 2)
>  ::
>  
>      "org.mozilla.uitour.treatment": {
>        "_v": 1,
>        "treatment": [
>          "optin",

Warning: Optin  ==> option [codespell]
Assignee: nobody → kmaglione+bmo
Comment on attachment 8964714 [details]
Bug 1420908: Remove telemetry experiments.

Taking this review.
Attachment #8964714 - Flags: review?(chutten) → review?(gfritzsche)
Comment on attachment 8964714 [details]
Bug 1420908: Remove telemetry experiments.

https://reviewboard.mozilla.org/r/233430/#review239160

Cheers for doing this!
I only have small notes here.

I'd like to double-check pipeline impact, so i called out to not remove the data point from the TelemetryEnvironment yet. We can look at this in a follow-up bug and still remove the bulk of the code here.

I also still see Experiments.jsm being referenced in:
- toolkit/mozapps/extensions/content/extensions.js
- toolkit/mozapps/extensions/test/browser/browser_experiments.js

::: toolkit/components/telemetry/TelemetryEnvironment.jsm
(Diff revision 2)
>      let addons = {
>        activeAddons: await this._getActiveAddons(),
>        theme: await this._getActiveTheme(),
>        activePlugins: this._getActivePlugins(),
>        activeGMPlugins: await this._getActiveGMPlugins(),
> -      activeExperiment: this._getActiveExperiment(),

Let's remove this property in a followup and always set it to `{}` here.

We'll need to double-check if we can just leave this out or if this will have pipeline impact.
It's optional in the schema, but we should double-check if our data jobs handle this properly.
https://github.com/mozilla-services/mozilla-pipeline-schemas/blob/dev/templates/include/telemetry/environment.1.schema.json

::: toolkit/components/telemetry/docs/data/environment.rst
(Diff revision 2)
> -        activeExperiment: { // obsolete in firefox 61, section is empty if there's no active experiment
> -            id: <string>, // id
> -            branch: <string>, // branch name
> -        },

Let's leave this for the follow-up bug.

::: toolkit/components/telemetry/docs/fhr/dataformat.rst
(Diff revision 2)
>                  "bartext": {
>                    "google": 1
>                  },
>                  "_v": "4"
> -              },
> -              "org.mozilla.experiment": {

This is deprecated documentation, kept around for archeological reasons.
Let's leave this file as is.

::: toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js:1671
(Diff revision 2)
>  
>    // ... but that if a cohort identifier is set, we send it.
>    deferred = PromiseUtils.defer();
>    TelemetryEnvironment.registerChangeListener("testSearchEngine_pref", deferred.resolve);
>    Services.prefs.setCharPref("browser.search.cohort", "testcohort");
> -  Services.obs.notifyObservers(null, "browser-search-service", "init-complete");
> +  Services.obs.notifyObservers(null, "browser-search-engine-modified", "engine-current");

This change looks unrelated.
This is probably for a different patch/bug?

::: toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js:1682
(Diff revision 2)
>  
>    // Check that when changing the cohort identifier...
>    deferred = PromiseUtils.defer();
>    TelemetryEnvironment.registerChangeListener("testSearchEngine_pref", deferred.resolve);
>    Services.prefs.setCharPref("browser.search.cohort", "testcohort2");
> -  Services.obs.notifyObservers(null, "browser-search-service", "init-complete");
> +  Services.obs.notifyObservers(null, "browser-search-engine-modified", "engine-current");

This also looks unrelated.

::: toolkit/content/aboutTelemetry.js
(Diff revision 2)
>      addonSection.setAttribute("class", "subsection-data subdata");
>      let addons = ping.environment.addons;
>      this.renderAddonsObject(addons.activeAddons, addonSection, "activeAddons");
>      this.renderActivePlugins(addons.activePlugins, addonSection, "activePlugins");
>      this.renderKeyValueObject(addons.theme, addonSection, "theme");
> -    this.renderKeyValueObject(addons.activeExperiment, addonSection, "activeExperiment");

Let's leave that for the follow-up.
Attachment #8964714 - Flags: review?(gfritzsche)
Priority: P3 → P1
:kmag, can you address the comments from Georg's review? 
Once this is done I can move on with the final steps to remove legacy Telemetry APIs.
Flags: needinfo?(kmaglione+bmo)
Comment on attachment 8964714 [details]
Bug 1420908: Remove telemetry experiments.

https://reviewboard.mozilla.org/r/233430/#review239160

Andrew removed the toolkit/mozapps/extensions references in bug 1450801, but landing that is blocked on this bug, now, since the other experiments tests wind up registering an add-on provider and breaking some other add-on manager tests.

> This change looks unrelated.
> This is probably for a different patch/bug?

It was part of the first version of the patch. I meant to remove it when I updated.
I applied the patch locally to ensure that it works. I run the test file :gfritzsche mentioned above, it passed.
`Experiments.jsm` is still referenced, but in both places usage is if-guarded to check for its availability before use (`"@mozilla.org/browser/experiments-service;1" in Cc`). This is ok, these files will then be removed in bug 1450801 anyway.

However, when applying the patch locally, I had to fix it in certain places. It seems to be based on changes from bug 1450801 (see below). This patch should be rebased on latest central to apply cleanly, and then bug 1450801 can be applied on top of that.


* Patch not applying cleanly
 
This patch: removing 3 preferences and empty line
  https://reviewboard.mozilla.org/r/233430/diff/3#chunk1.2

Patch from bug 1450801: removing remaining preferences, leaving 3 "old" preferences in place
  https://reviewboard.mozilla.org/r/233142/diff/3#chunk1.2
The patch on mozreview is on top of bug 1450801. The two will need to land at the same time, so there's no point in rebasing on top of m-c.
Flags: needinfo?(kmaglione+bmo)
Comment on attachment 8964714 [details]
Bug 1420908: Remove telemetry experiments.

https://reviewboard.mozilla.org/r/233430/#review242202

Thanks, this looks good to me! Happy to see this cleaned up for us all!
I assume you and Andrew have the dependencies between this and bug 1450801 covered.
Attachment #8964714 - Flags: review?(gfritzsche) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a77da7f1488ef1c8011f049c5e536ca3716a619
Bug 1420908: Follow-up: Remove missing directory from codespell.yml. r=bustage DONTBUILD CLOSED TREE
Depends on: 1454416
You need to log in before you can comment on or make changes to this bug.