1242591 - Rename MOZ_SERVICES_HEALTHREPORT to MOZ_ENABLE_HEALTHREPORT

Status: RESOLVED WONTFIX

(Toolkit :: Telemetry, defect, P4)

Description

[Alessio Placitelli [:Dexter]]

Since bug 1234526 landed, there's no Health Report service anymore. The MOZ_SERVICES_HEALTHREPORT is still being used to enable/disable about:healthreport and SelfSupport.

We should rename MOZ_SERVICES_HEALTHREPORT to something more fitting - MOZ_ENABLE_HEALTHREPORT for example.

Comment 1

[Georg Fritzsche [:gfritzsche]]

Lets try to do this soon - its not a lot of work and its confusingly named now

Comment 2

[Claas Augner [:claas]]

Is there anything else to do here beyond renaming instances of MOZ_SERVICES_HEALTHREPORT in the source code to MOZ_ENABLE_HEALTHREPORT (which would be consistent with other MOZ_ENABLE_* constants)?

Comment 3

[Claas Augner [:claas]]

Maybe I can answer the question myself: As MOZ_SERVICES_HEALTHREPORT=0 is possibly already used, it could either be kept as an alias (e.g. by adding an #elif branch at 0 and similar locations) or a warning could be emitted when configured. Can you affirm?

0: https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/AppConstants.jsm#64

Comment 4

[Georg Fritzsche [:gfritzsche]]

Sorry for the long response time.

I think we can just replace all the occurences we see in our code, that is fine.
However, we do need an alias to support historic values in existing mozconfig files that don't live in our tree (e.g. for Linux distributions).
For that we'd need to define MOZ_ENABLE_HEALTHREPORT in the configure script when either of MOZ_ENABLE_HEALTHREPORT and MOZ_SERVICES_HEALTHREPORT are set:
https://dxr.mozilla.org/mozilla-central/search?q=path%3Aold-configure.in+MOZ_SERVICES_HEALTHREPORT&redirect=false&case=true

Comment 5

[Georg Fritzsche [:gfritzsche]]

Margaret, just checking to be sure:
Given that the FHR code was removed on Android too, are we fine with renaming MOZ_SERVICES_HEALTHREPORT to MOZ_ENABLE_HEALTHREPORT on mobile too?

https://dxr.mozilla.org/mozilla-central/search?q=MOZ_SERVICES_HEALTHREPORT+path%3Amobile&redirect=false&case=true

Comment 6

[:Margaret Leibovic]

(In reply to Georg Fritzsche [:gfritzsche] from comment #5)
> Margaret, just checking to be sure:
> Given that the FHR code was removed on Android too, are we fine with
> renaming MOZ_SERVICES_HEALTHREPORT to MOZ_ENABLE_HEALTHREPORT on mobile too?
> 
> https://dxr.mozilla.org/mozilla-central/
> search?q=MOZ_SERVICES_HEALTHREPORT+path%3Amobile&redirect=false&case=true

This sounds fine to me, but let's double check with Mike.

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

We use an #ifdef in AppConstants to set this value. Provided we update the usages in mobile/android/ [1] too, I'm sure it'll be fine.

[1]: https://mxr.mozilla.org/mozilla-central/search?string=MOZ_SERVICES_HEALTHREPORT&find=mobile%2Fandroid&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

Comment 8

[Georg Fritzsche [:gfritzsche]]

Ok, thanks.

Claas, would you be interested in taking this bug?

Comment 9

[Claas Augner [:claas]]

Thanks for the feedback, Georg, I'm happily taking this bug.

Comment 10

[Claas Augner [:claas]]

Attachment: Rename MOZ_SERVICES_HEALTHREPORT to MOZ_ENABLE_HEALTHREPORT

Comment 11

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8756974 [details] [diff] [review]
Rename MOZ_SERVICES_HEALTHREPORT to MOZ_ENABLE_HEALTHREPORT

Sorry, i'm out sick. Alessio, can you take look?

Comment 12

[Alessio Placitelli [:Dexter]]

Comment on attachment 8756974 [details] [diff] [review]
Rename MOZ_SERVICES_HEALTHREPORT to MOZ_ENABLE_HEALTHREPORT

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

The changes look good to me.

IMHO, most of the changes in old-cofnigure.in can be dropped due to bug 1257326.

[2] - https://dxr.mozilla.org/mozilla-central/rev/4d63dde701b47b8661ab7990f197b6b60e543839/build/moz.configure/init.configure#715

::: browser/moz.configure
@@ +4,5 @@
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  imply_option('MOZ_PLACES', True)
> +imply_option('MOZ_ENABLE_HEALTHREPORT', True)

I think that, due to bug 1257326 [1] and this change, we can drop the additions in old-configure.in, as MOZ_ENABLE_HEALTHREPORT is automatically implied here.

[1] - https://hg.mozilla.org/mozilla-central/rev/5b74c0699042

::: old-configure.in
@@ +6760,5 @@
>        AC_DEFINE(MOZ_TELEMETRY_ON_BY_DEFAULT)
>      fi
>  fi
>  
> +dnl Support historic FHR switch.

I think we can drop this part up to line 6768, see the comment from moz.configure.

Comment 13

[Claas Augner [:claas]]

Attachment: Rename MOZ_SERVICES_HEALTHREPORT to MOZ_ENABLE_HEALTHREPORT.

Thanks for the review, Alessio.

The attached patch contains all changes from the previous patch except for the additions in |old-configure.in| (I kept the replacement for the sake of consistency).

Comment 14

[Georg Fritzsche [:gfritzsche]]

Hi Claas. Due to the London workweek, we'll be slow to respond until next week.

Comment 15

[Alessio Placitelli [:Dexter]]

Comment on attachment 8762569 [details] [diff] [review]
Rename MOZ_SERVICES_HEALTHREPORT to MOZ_ENABLE_HEALTHREPORT.

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

This looks good to me.

Georg has to review this though :-)

Comment 16

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8762569 [details] [diff] [review]
Rename MOZ_SERVICES_HEALTHREPORT to MOZ_ENABLE_HEALTHREPORT.

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

Mike, can you give feedback on this from the build perspective?

We don't have a FHR services anymore, but we still have user-facing "FHR"-branded features.
So we are trying to rename the, now confusing, "MOZ_SERVICES_HEALTHREPORT" to "MOZ_ENABLE_HEALTHREPORT".
What i'm concerned about is users or distributions having "MOZ_SERVICES_HEALTHREPORT" in their mozconfigs.

I think we need to treat "MOZ_SERVICES_HEALTHREPORT" as an alias for "MOZ_ENABLE_HEALTHREPORT".
Is defining the latter in old-configure.in when the first is present in the mozconfig the right approach for that?
I also see this with |imply_option()| in |browser/moz.configure| - can we still set it to a "false" equivalent? How?

::: old-configure.in
@@ +6738,5 @@
>  
>  dnl If we have any service that uploads data (and requires data submission
>  dnl policy alert), set MOZ_DATA_REPORTING.
>  dnl We need SUBST for build system and DEFINE for xul preprocessor.
> +if test -n "$MOZ_TELEMETRY_REPORTING" || test -n "$MOZ_ENABLE_HEALTHREPORT" || test -n "$MOZ_CRASHREPORTER"; then

Do we need to support "MOZ_SERVICES_HEALTHREPORT" here as an alias here?

::: toolkit/moz.configure
@@ +407,1 @@
>               help='Build Firefox Health Reporter Service',

help='Enable Firefox Health Report features'

Comment 17

[Georg Fritzsche [:gfritzsche]]

Review ping.

Comment 18

[Mike Hommey [:glandium]]

(In reply to Georg Fritzsche [:gfritzsche] [away Jun 24 - Jul 3] from comment #16)
> Comment on attachment 8762569 [details] [diff] [review]
> Rename MOZ_SERVICES_HEALTHREPORT to MOZ_ENABLE_HEALTHREPORT.
> 
> Review of attachment 8762569 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Mike, can you give feedback on this from the build perspective?
> 
> We don't have a FHR services anymore, but we still have user-facing
> "FHR"-branded features.
> So we are trying to rename the, now confusing, "MOZ_SERVICES_HEALTHREPORT"
> to "MOZ_ENABLE_HEALTHREPORT".
> What i'm concerned about is users or distributions having
> "MOZ_SERVICES_HEALTHREPORT" in their mozconfigs.
> 
> I think we need to treat "MOZ_SERVICES_HEALTHREPORT" as an alias for
> "MOZ_ENABLE_HEALTHREPORT".

If MOZ_SERVICES_HEALTHREPORT is kept for that reason, then why rename at all? Plus, MOZ_ENABLE_* variables are not very appealing either. The ENABLE is kind of redundant with the value of the variable. Yes, we have some, but they are a minority.

Comment 19

[Georg Fritzsche [:gfritzsche]]

(In reply to Mike Hommey [:glandium] from comment #18)
> > I think we need to treat "MOZ_SERVICES_HEALTHREPORT" as an alias for
> > "MOZ_ENABLE_HEALTHREPORT".
> 
> If MOZ_SERVICES_HEALTHREPORT is kept for that reason, then why rename at
> all? Plus, MOZ_ENABLE_* variables are not very appealing either. The ENABLE
> is kind of redundant with the value of the variable. Yes, we have some, but
> they are a minority.

MOZ_SERVICES_HEALTHREPORT is confusing - there is no healthreport service anymore.
This now just controls some front-end features.

Can you maybe suggest a better naming? Just MOZ_HEALTHREPORT?

Comment 20

[Mike Hommey [:glandium]]

> Just MOZ_HEALTHREPORT?

why not?

Comment 21

[Claas Augner [:claas]]

Attachment: Rename MOZ_SERVICES_HEALTHREPORT to MOZ_HEALTHREPORT

Comment 22

[Claas Augner [:claas]]

Comment on attachment 8777933 [details] [diff] [review]
Rename MOZ_SERVICES_HEALTHREPORT to MOZ_HEALTHREPORT

Review cancelled due to merge conflicts.

Comment 23

[Claas Augner [:claas]]

Attachment: Rename MOZ_SERVICES_HEALTHREPORT to MOZ_HEALTHREPORT

Comment 24

[Claas Augner [:claas]]

Comment on attachment 8818390 [details] [diff] [review]
Rename MOZ_SERVICES_HEALTHREPORT to MOZ_HEALTHREPORT

I have updated my patch to replace all current occurrences of MOZ_SERVICES_HEALTHREPORT by MOZ_HEALTHREPORT, correcting the merge conflicts.

Regarding MOZ_SERVICES_HEALTHREPORT: As it used to default to True (in moz.configure), it only allowed disabling FHR by setting it to "" (empty), which would now become its new default (if omitted in moz.configure). That said, the only way to maintain it as an alias would be to have MOZ_SERVICES_HEALTHREPORT and MOZ_HEALTHREPORT both default to True (in moz.configure), but to set MOZ_HEALTHREPORT to "" (empty) if MOZ_SERVICES_HEALTHREPORT is "" (empty) (in old-configure.in). So I wonder: Is it worth it? Do we have build telemetry data to support that MOZ_SERVICES_HEALTHREPORT is actually used in any mozconfig?

PS: I noticed that the crash reporter can be disabled with |ac_add_options --disable-crashreporter|, so I was wondering whether |ac_add_options --disable-healthreport| wouldn't be a cleaner way to disable healthreport?!

Comment 25

[Alessio Placitelli [:Dexter]]

Closing this as WONTFIX, since this is a bit of a risky change to do right now. Moreover, we might have a different data collection mechanism in the future that save us the trouble of doing this now.