1234526 - Remove services/healthreport

Status: RESOLVED FIXED

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

Description

[Alessio Placitelli [:Dexter]]

This bug is about removing services/metrics & services/healthreport from Firefox codebase.


We should also take care of removing:

- Orphaned manifest entries: https://dxr.mozilla.org/mozilla-central/search?q=%22HealthReport.jsm%22+-path%3Aobj&redirect=false&case=false
- Uses of Metrics.jsm throughout the codebase

Comment 1

[Alessio Placitelli [:Dexter]]

Changed this bug to only remove services/healthreport to ease reviewing.

Comment 2

[Alessio Placitelli [:Dexter]]

This bug should also investigate and eventually remove all the uses of the FHR prefs (listed in [0]), except the following:

- datareporting.healthreport.infoURL - Used to build the preferences panel
- datareporting.healthreport.uploadEnabled - Used to control ping upload in Telemetry
- datareporting.healthreport.about.reportUrl - Used for the about:healthreport content URL / former v2 URL
- datareporting.healthreport.about.reportUrlUnified - Used for the about:healthreport content URL / UT URL

[0] - https://dxr.mozilla.org/mozilla-central/rev/388bdc46ba51ee31da8b8abe977e0ca38d117434/services/healthreport/healthreport-prefs.js

Comment 3

[Georg Fritzsche [:gfritzsche]]

(In reply to Alessio Placitelli [:Dexter] from comment #2)
> This bug should also investigate and eventually remove all the uses of the
> FHR prefs (listed in [0]), except the following:
> 
> - datareporting.healthreport.infoURL - Used to build the preferences panel
> - datareporting.healthreport.uploadEnabled - Used to control ping upload in
> Telemetry
> - datareporting.healthreport.about.reportUrl - Used for the
> about:healthreport content URL / former v2 URL
> - datareporting.healthreport.about.reportUrlUnified - Used for the
> about:healthreport content URL / UT URL

This was planned to be used, but we decided to go a different route (see bug 1227579).
We should remove that and any uses in code (potentially in a separate bug).

Comment 4

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Remove services/healthreport

Comment 5

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Remove unused prefs

Comment 6

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8704160 [details] [diff] [review]
part 1 - Remove services/healthreport

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

This is not removing test_healthreporter.js.

::: mobile/android/installer/package-manifest.in
@@ -440,5 @@
>  
> -#ifdef MOZ_SERVICES_HEALTHREPORT
> -@BINPATH@/components/HealthReportComponents.manifest
> -@BINPATH@/components/HealthReportService.js
> -#endif

Already removed: https://hg.mozilla.org/mozilla-central/rev/de71199f4e00

::: modules/libpref/greprefs.js
@@ +6,5 @@
>  #ifdef MOZ_SERVICES_HEALTHREPORT
>  #if MOZ_WIDGET_TOOLKIT == android
>  #include ../../mobile/android/chrome/content/healthreport-prefs.js
>  #else
> +#include ../../toolkit/components/telemetry/healthreport-prefs.js

Lets file something about consolidating those prefs into one file, telemetry-prefs.js

::: services/healthreport/docs/dataformat.rst
@@ -1,1 @@
> -.. _healthreport_dataformat:

People might still have to look at this for a bit.
Lets move this to the Telemetry doc dir and index (so we can kill the FHR dirs) and mark it with a clear disclaimer on FHR being obsolete.

Comment 7

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8704162 [details] [diff] [review]
part 2 - Remove unused prefs

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

::: toolkit/components/telemetry/healthreport-prefs.js
@@ +11,2 @@
>  pref("datareporting.healthreport.about.reportUrl", "https://fhr.cdn.mozilla.net/%LOCALE%/v4/");
>  pref("datareporting.healthreport.about.reportUrlUnified", "https://fhr.cdn.mozilla.net/%LOCALE%/v4/");

Didn't we want to kill .reportUrlUnified somewhere?
I'd be happy to have it in a separate bug as long as it happens.

Comment 8

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Remove services/healthreport

Comment 9

[Alessio Placitelli [:Dexter]]

(In reply to Georg Fritzsche [:gfritzsche] [away until Jan 3] from comment #6)
> Comment on attachment 8704160 [details] [diff] [review]
> part 1 - Remove services/healthreport
> 
> Review of attachment 8704160 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is not removing test_healthreporter.js.

This is being removed by bug 1234522.

> ::: mobile/android/installer/package-manifest.in
> @@ -440,5 @@
> >  
> > -#ifdef MOZ_SERVICES_HEALTHREPORT
> > -@BINPATH@/components/HealthReportComponents.manifest
> > -@BINPATH@/components/HealthReportService.js
> > -#endif
> 
> Already removed: https://hg.mozilla.org/mozilla-central/rev/de71199f4e00

Ah, good!

> ::: modules/libpref/greprefs.js
> @@ +6,5 @@
> >  #ifdef MOZ_SERVICES_HEALTHREPORT
> >  #if MOZ_WIDGET_TOOLKIT == android
> >  #include ../../mobile/android/chrome/content/healthreport-prefs.js
> >  #else
> > +#include ../../toolkit/components/telemetry/healthreport-prefs.js
> 
> Lets file something about consolidating those prefs into one file,
> telemetry-prefs.js

Filed bug 1237276.

> ::: services/healthreport/docs/dataformat.rst
> @@ -1,1 @@
> > -.. _healthreport_dataformat:
> 
> People might still have to look at this for a bit.
> Lets move this to the Telemetry doc dir and index (so we can kill the FHR
> dirs) and mark it with a clear disclaimer on FHR being obsolete.

Good point.

Comment 10

[Alessio Placitelli [:Dexter]]

(In reply to Georg Fritzsche [:gfritzsche] [away until Jan 3] from comment #7)
> Comment on attachment 8704162 [details] [diff] [review]
> part 2 - Remove unused prefs
> 
> Review of attachment 8704162 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/telemetry/healthreport-prefs.js
> @@ +11,2 @@
> >  pref("datareporting.healthreport.about.reportUrl", "https://fhr.cdn.mozilla.net/%LOCALE%/v4/");
> >  pref("datareporting.healthreport.about.reportUrlUnified", "https://fhr.cdn.mozilla.net/%LOCALE%/v4/");
> 
> Didn't we want to kill .reportUrlUnified somewhere?
> I'd be happy to have it in a separate bug as long as it happens.

That's being done as part of bug 1236580.

Comment 11

[Alessio Placitelli [:Dexter]]

Attachment: part 3 - Don't track SQL statements for healthreport.sqlite in Telemetry

Comment 12

[Alessio Placitelli [:Dexter]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e0b62d85cb8

Comment 13

[Alessio Placitelli [:Dexter]]

https://hg.mozilla.org/integration/fx-team/rev/096d46bdaf86112bc1bf350fec18d5d4063ab0c2
Bug 1234526 - Remove services/healthreport. r=gfritzsche

https://hg.mozilla.org/integration/fx-team/rev/3124025d114712f6717e6cf6a4ba930f0ab02ac3
Bug 1234526 - Remove unused healthreporter prefs. r=gfritzsche

https://hg.mozilla.org/integration/fx-team/rev/c7689b72d3fa71043aa8601134b7bdc4581deb86
Bug 1234526 - Don't track healthreport.sqlite statements from Telemetry. r=gfritzsche

Comment 14

[Phil Ringnalda (:philor)]

Backed out the whole push in https://hg.mozilla.org/integration/fx-team/rev/19a2342819e4 - see bug 1234522 comment 23

Comment 15

[Alessio Placitelli [:Dexter]]

https://hg.mozilla.org/integration/fx-team/rev/839ed1dfce6477dc20a77b7f1465188af1068c04
Bug 1234526 - Remove services/healthreport. r=gfritzsche

https://hg.mozilla.org/integration/fx-team/rev/a90decf2cd6a344110d74c5473f02255cb0d06ac
Bug 1234526 - Remove unused healthreporter prefs. r=gfritzsche

https://hg.mozilla.org/integration/fx-team/rev/86e069b34fb4690ac6f5e959ef7d259014307641
Bug 1234526 - Don't track healthreport.sqlite statements from Telemetry. r=gfritzsche

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/839ed1dfce64
https://hg.mozilla.org/mozilla-central/rev/a90decf2cd6a
https://hg.mozilla.org/mozilla-central/rev/86e069b34fb4