Closed Bug 1352497 Opened 3 years ago Closed 2 years ago

Remove about:healthreport

Categories

(Toolkit :: Telemetry, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
relnote-firefox --- 59+
firefox59 --- fixed

People

(Reporter: mheubusch, Assigned: dao)

References

(Depends on 1 open bug, Blocks 3 open bugs)

Details

(Whiteboard: [measurement:client:tracking])

Attachments

(2 files, 2 obsolete files)

Please remove the access points to the about:healthreport page.  On all platforms from the HamburgerMenu>? button>Help menu and on Mac also from the top Help menu.
In the Preferences/Options,
new preferences (Nightly; after bug 1335907 landed): Updates > Enable Health Report
old preferences: Advanced > Data Choices > Enable Health Report
has as description: "Helps you understand your browser performance and shares data with Mozilla about your browser health"

Should this be shortened to "Shares data with Mozilla about your browser health"?

Furthermore, Telemetry depends on FHR on desktop in the preferences, but not on Android.
Flags: needinfo?(mheubusch)
(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #1)
> In the Preferences/Options,
> new preferences (Nightly; after bug 1335907 landed): Updates > Enable Health
> Report
> old preferences: Advanced > Data Choices > Enable Health Report
> has as description: "Helps you understand your browser performance and
> shares data with Mozilla about your browser health"
> 
> Should this be shortened to "Shares data with Mozilla about your browser
> health"?

See bug 1348074 on FHR/Telemetry description concerns.
This bug is separate and only about not exposing about:healthreport directly anymore.
Assignee: nobody → aryx.bugmail
Status: NEW → ASSIGNED
Flags: needinfo?(mheubusch)
Has this gone through a legal review with Marshall? I fully support this but there have been concerns before.
Flags: needinfo?(mheubusch)
Attachment #8853948 - Flags: review?(rnewman) → review?(s.kaspari)
Comment on attachment 8853948 [details]
Bug 1352497 - Remove link to about:healthreport from menus: android.

https://reviewboard.mozilla.org/r/125966/#review129358

LGTM
Attachment #8853948 - Flags: review?(s.kaspari) → review+
So these patches remove the references to about:healthreport but the page still exists, basically as dead code at that point. What's the longer-term plan for about:healthreport?
Please DO NOT land this without confirmation from Michelle that this has gone through risk analysis.

If we can remove this, we can probably remove the about:healthreport mechanism altogether.
(In reply to Benjamin Smedberg [:bsmedberg] from comment #5)
> Has this gone through a legal review with Marshall? I fully support this but
> there have been concerns before.

Hello Benjamin - I filed this bug per a discussion with Mika. Since we are only removing the access points from the UI but not the page itself, she simply asked that I notify Marshall so that we had a response ready in case anyone thought the page itself was gone.  He is cc'ed on the bug. I will file a separate bug and x-ref it here just to be thorough.
Flags: needinfo?(mheubusch)
There is absolutely no point in keeping the about:healthreport page if it's not in the UI.
Comment on attachment 8853947 [details]
Bug 1352497 - Remove link to about:healthreport from menus: browser/.

I haven't heard back from Mika, but I think general consensus was that we should completely remove this page.
Attachment #8853947 - Flags: review?(dao+bmo) → review-
Whiteboard: [measurement:client:tracking]
Summary: Remove link to about:healthreport from Menu → Remove about:healthreport
Hi all. I'm visiting from bug 1346273 where we're trying, and failing, to apply modern web security principles to about:healthreport. Can we get this removal shipped in Firefox soon?
See Also: → 1348074
Looking at what about:healthreport does, it almost seems like this would be better served as a wrapped webextension, if we want it to continue to exist.
Who can make a final decision here?
Flags: needinfo?(benjamin)
pdol owns this decision.
Flags: needinfo?(benjamin) → needinfo?(pdolanjski)
I spoke with Elvin about this (since Mika is now out on leave).
We will likely want to move forward with removal in v56, but Elvin is trying to coordinate some governance communication about a number of changes for telemetry (preferences) and healthreport. (that we may want to put out before the removal)
He asked that we hold off until he comes back with confirmation to proceed. I'm NI'ing him here so he can reply directly.
Flags: needinfo?(pdolanjski) → needinfo?(ellee)
Hi, confirming that we can move forward with the work to remove for 56+. How are we planning on informing users in-product what happened to FHR?
Flags: needinfo?(ellee) → needinfo?(pdolanjski)
(In reply to Elvin Lee [:ellee] (use NEEDINFO please) from comment #17)
> Hi, confirming that we can move forward with the work to remove for 56+. How
> are we planning on informing users in-product what happened to FHR?

What happened to FHR in terms of it just becoming telemetry or do you mean about:healthreport, specifically?
Flags: needinfo?(pdolanjski)
Flags: needinfo?(ellee)
What happened to about:healthreport, specifically. The concern being that few users who do go to about:healthreport will suddenly get "address not valid"
Flags: needinfo?(ellee) → needinfo?(pdolanjski)
(In reply to Elvin Lee [:ellee] (use NEEDINFO please) from comment #19)
> What happened to about:healthreport, specifically. The concern being that
> few users who do go to about:healthreport will suddenly get "address not
> valid"

I think the standard way to do this is to put in the release notes.

We could also have a stub about:healthreport page explaining that the feature doesn't exist anymore. However, I suspect for most people who ever saw or used this feature, the Help menu has been the entry point, so once that menu item is gone, leaving about:healthreport behind, even as a stub, seems pointless.
Yes, release notes.  If someone goes there and it's not there, they can check the release notes.  That's probably the best we can do and maybe add something in SUMO.

Elvin, does that work for you?

Joni, can you help with the SUMO piece?  If we have any information on about:health report, it should be updated to say that the page has been removed.
Flags: needinfo?(pdolanjski)
Flags: needinfo?(jsavage)
Flags: needinfo?(ellee)
That's fine - probably should indicate the version it is being removed for in the SUMO article too.
Flags: needinfo?(ellee)
Sebastian, are you available to pick this up again?
Flags: needinfo?(aryx.bugmail)
Blocks: 1390677
Blocks: 1390678
(In reply to Dão Gottwald [::dao] from comment #23)
> Sebastian, are you available to pick this up again?
Yes, will do.
Flags: needinfo?(aryx.bugmail)
Just curious, are the underlying about:config settings being stripped out as well?
Flags: needinfo?(aryx.bugmail)
(In reply to [:jgruen] from comment #25)
> Just curious, are the underlying about:config settings being stripped out as
> well?

What prefs in particular?

Any pref that's needed only for the about:healthreport page should ideally go away here. Other prefs would be off-topic as far as this bug is concerned.
Flags: needinfo?(jgruen)
Screenshots uses `datareporting.healthreport.uploadEnabled` to determine whether or not we collect data. I've already filed an issue to move us off this pref. https://github.com/mozilla-services/screenshots/issues/3390
Flags: needinfo?(jgruen)
(In reply to [:jgruen] from comment #27)
> `datareporting.healthreport.uploadEnabled`

I don't think this has anything to do with this bug.
Fair enough, we're going to make the switch anyway since FHR seems to be going into the dustbin.
(In reply to [:jgruen] from comment #27)
> Screenshots uses `datareporting.healthreport.uploadEnabled` to determine
> whether or not we collect data. I've already filed an issue to move us off
> this pref. https://github.com/mozilla-services/screenshots/issues/3390

This pref still controls all data upload, even with Telemetry. Let's check what exactly can get cleanup.
To simplify things we could limit this bug to just remove what is clearly only used for the about:healthreport page and move anything that requires investigation or discussion to a follow-up.

I'll follow up on the GH issue.
See Also: → 1400272
please help us what to do with the existing sumo article at https://support.mozilla.org/kb/firefox-health-report-understand-your-browser-perf documenting fhr (bug 1400272) :-)
we can add version specific content there - in which version of firefox are you planning to totally remove the page?
At this point this will miss 57 unless we still uplift it. Sebastian, are you working on this or should somebody else take over?
Assignee: aryx.bugmail → dao+bmo
Flags: needinfo?(jsavage)
Flags: needinfo?(aryx.bugmail)
Component: Menus → Telemetry
Product: Firefox → Toolkit
Attachment #8853947 - Attachment is obsolete: true
Attachment #8853948 - Attachment is obsolete: true
Blocks: 1418565
No longer blocks: 1418565
Comment on attachment 8928932 [details]
Bug 1352497 - Remove about:healthreport.

https://reviewboard.mozilla.org/r/200278/#review206456

Cheers for this, i'm happy to see this go!

One thing: far as i can tell, the selfsupport service is only used for about:healthreport and can go too:
https://searchfox.org/mozilla-central/search?q=MozSelfSupport&path=
https://searchfox.org/mozilla-central/search?q=SelfSupport&case=false&regexp=false&path=

Also, i think we can remove more preferences (see below).

::: testing/geckodriver/src/prefs.rs:121
(Diff revision 1)
>          ("browser.warnOnQuit", Pref::new(false)),
>  
>          // Do not show datareporting policy notifications which can
>          // interfere with tests
> -        ("datareporting.healthreport.about.reportUrl", Pref::new("http://%(server)s/dummy/abouthealthreport/")),
>          ("datareporting.healthreport.documentServerURI", Pref::new("http://%(server)s/dummy/healthreport/")),

I think we can remove all `datareporting.*` prefs, except:
- `datareporting.policy.dataSubmission*`
- `datareporting.healthreport.uploadEnabled`
- `datareporting.healthreport.infoURL`

We should double-check though for what Fennec uses.

::: toolkit/components/telemetry/docs/fhr/architecture.rst
(Diff revision 1)
>  service.loadDelayMsec
>     How long (in milliseconds) after initial application start should FHR
>     wait before initializing.
>  
>     FHR may initialize sooner than this if the FHR service is requested.
> -   This will happen if e.g. the user goes to ``about:healthreport``.

FWIW, these docs are marked as obsolete already:
https://firefox-source-docs.mozilla.org/toolkit/components/telemetry/telemetry/fhr/index.html

We already removed the FHR service before and only keep these docs for data archeology purposes.

::: toolkit/components/telemetry/docs/internals/preferences.rst:31
(Diff revision 1)
>  
>  ``MOZ_SERVICES_HEALTHREPORT``
>  
>    When Defined (which it is on most platforms):
>  
> -  * Builds ``about:healthreport`` and associated underpinnings (see `healthreport <../fhr/index>`)
> +  * Builds healthreport underpinnings (see `healthreport <../fhr/index>`)

While we're here, could you also remove this bullet?
We already removed FHR a while ago.
Attachment #8928932 - Flags: review?(gfritzsche)
Sebastian, do you know who can give high-level feedback on the Android removals here?
Flags: needinfo?(s.kaspari)
Priority: -- → P1
(In reply to Georg Fritzsche [:gfritzsche] from comment #35)
> One thing: far as i can tell, the selfsupport service is only used for
> about:healthreport and can go too:
> https://searchfox.org/mozilla-central/search?q=MozSelfSupport&path=
> https://searchfox.org/mozilla-central/
> search?q=SelfSupport&case=false&regexp=false&path=

It's a web API, right? I can't tell from those searches where it's used. New bug please.

> ::: testing/geckodriver/src/prefs.rs:121
> (Diff revision 1)
> >          ("browser.warnOnQuit", Pref::new(false)),
> >  
> >          // Do not show datareporting policy notifications which can
> >          // interfere with tests
> > -        ("datareporting.healthreport.about.reportUrl", Pref::new("http://%(server)s/dummy/abouthealthreport/")),
> >          ("datareporting.healthreport.documentServerURI", Pref::new("http://%(server)s/dummy/healthreport/")),
> 
> I think we can remove all `datareporting.*` prefs, except:
> - `datareporting.policy.dataSubmission*`
> - `datareporting.healthreport.uploadEnabled`
> - `datareporting.healthreport.infoURL`

Ditto, this should be a separate bug. Looks like some of these prefs are already unused today.
(In reply to Dão Gottwald [::dao] from comment #37)
> (In reply to Georg Fritzsche [:gfritzsche] from comment #35)
> > One thing: far as i can tell, the selfsupport service is only used for
> > about:healthreport and can go too:
> > https://searchfox.org/mozilla-central/search?q=MozSelfSupport&path=
> > https://searchfox.org/mozilla-central/
> > search?q=SelfSupport&case=false&regexp=false&path=
> 
> It's a web API, right? I can't tell from those searches where it's used. New
> bug please.

Sounds good.

> > ::: testing/geckodriver/src/prefs.rs:121
> > (Diff revision 1)
> > >          ("browser.warnOnQuit", Pref::new(false)),
> > >  
> > >          // Do not show datareporting policy notifications which can
> > >          // interfere with tests
> > > -        ("datareporting.healthreport.about.reportUrl", Pref::new("http://%(server)s/dummy/abouthealthreport/")),
> > >          ("datareporting.healthreport.documentServerURI", Pref::new("http://%(server)s/dummy/healthreport/")),
> > 
> > I think we can remove all `datareporting.*` prefs, except:
> > - `datareporting.policy.dataSubmission*`
> > - `datareporting.healthreport.uploadEnabled`
> > - `datareporting.healthreport.infoURL`
> 
> Ditto, this should be a separate bug. Looks like some of these prefs are
> already unused today.

Right. So, if i understand this correctly, you are just removing the one specific pref related to the about:healthreport content?
(In reply to Georg Fritzsche [:gfritzsche] from comment #39)
> Right. So, if i understand this correctly, you are just removing the one
> specific pref related to the about:healthreport content?

Yeah.
Comment on attachment 8928932 [details]
Bug 1352497 - Remove about:healthreport.

https://reviewboard.mozilla.org/r/200278/#review206970

This looks good to me overall.
Two things though:

1) I'd really want a mobile peer to look over it if the changes make sense to them.

2) Removing `mobile/android/chrome/content/healthreport-prefs.js` might have side-effects as it includes `datareporting.healthreport.uploadEnabled`.
I'm not off-hand 100% sure that this won't effect Fennec Telemetry in some way.
I'd prefer to leave the pref `uploadEnabled` in for Fennec and let us clean this up when we get to sorting the mobile Telemetry situation.
Alternatively we can trace the pref usage through the tree and verify, but that will take a bit.
Attachment #8928932 - Flags: review?(gfritzsche)
Attachment #8928932 - Flags: review?(s.kaspari)
Attachment #8928932 - Flags: review?(s.kaspari) → review?(cnevinchen)
Comment on attachment 8928932 [details]
Bug 1352497 - Remove about:healthreport.

https://reviewboard.mozilla.org/r/200278/#review208022

The code looks goot to me. I've pulled it and tested in my local. In Fennec side it works fine. But I still don't know the reason to remove this. It'll be great if you can give us more background about this.
Attachment #8928932 - Flags: review?(cnevinchen) → review+
(In reply to Nevin Chen [:nechen] from comment #45)
> Comment on attachment 8928932 [details]
> Bug 1352497 - Remove about:healthreport.
> 
> https://reviewboard.mozilla.org/r/200278/#review208022
> 
> The code looks goot to me. I've pulled it and tested in my local. In Fennec
> side it works fine. But I still don't know the reason to remove this. It'll
> be great if you can give us more background about this.

The feature is unneeded and nobody wants to maintain it.
Flags: needinfo?(s.kaspari)
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/be86ccde4f4a
Remove about:healthreport. r=gfritzsche,nechen
https://hg.mozilla.org/mozilla-central/rev/be86ccde4f4a
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Depends on: 1420637
Reopened due to back-out
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Depends on: 1410759
Blocks: 1176373
Attachment #8928932 - Flags: review?(s.kaspari)
Flags: needinfo?(dao+bmo)
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7a1a46675739
Remove about:healthreport. r=gfritzsche,nechen
https://hg.mozilla.org/mozilla-central/rev/7a1a46675739
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
(In reply to Dão Gottwald [::dao] from comment #20)
> (In reply to Elvin Lee [:ellee] (use NEEDINFO please) from comment #19)
> > What happened to about:healthreport, specifically. The concern being that
> > few users who do go to about:healthreport will suddenly get "address not
> > valid"
> 
> I think the standard way to do this is to put in the release notes.
> 

Do you still intend to have it mentionned in the release notes? If so, please nominate this bug for release note addition. Thanks
Flags: needinfo?(dao+bmo)
Release Note Request (optional, but appreciated)
[Why is this notable]: users may have used this page to understand what data Firefox sends to Mozilla
[Affects Firefox for Android]: yes
[Suggested wording]: about:healthreport (Firefox Health Report) has been removed in favor of about:telemetry
[Links (documentation, blog post, etc)]:
relnote-firefox: --- → ?
Flags: needinfo?(dao+bmo)
Blocks: 1420691
Blocks: 1337660
Noted for 59.0b1 as: Changed the location of Firefox Health Report from about:healthreport to about:telemetry
Note that this patch removed the
datareporting.healthreport.about.reportUrl pref from various automation
tools in the tree that targets release channels out-of-tree.
This change should not have been made without approval from a
peer of the Testing component.  I’m remedying this as part of
https://bugzilla.mozilla.org/show_bug.cgi?id=1401129.
You need to log in before you can comment on or make changes to this bug.