837292 - Add a TelemetryEnabled flag to FHR

Status: RESOLVED FIXED

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

Description

[Vladan Djeric (:vladan)]

We often have questions about the Telemetry population, such as their build adoption rates, the number of users on a channel, etc. FHR already collects this data and we'd like to be able to answer those questions by identifying the subset of FHR users who are also Telemetry users. To this end, I'd like FHR to report the value of the "toolkit.telemetry.enabled" pref.

Comment 1

[Gregory Szorc [:gps]]

Sheila: Could you please track down the appropriate people to get an answer on adding this data point to FHR?

Comment 2

[Gregory Szorc [:gps]]

Vladan, Taras, et al: Please correct me if I am wrong, but I'm under the impression that there is a lot of uncertainty with the applicability of Telemetry's data compared with the broader user base. i.e. we are uncertain of the skews/biases in Telemetry data. Having the ability to correlate Telemetry users against the broader set of FHR users would greatly help us identify biases in the data and thus would make Telemetry data more useful.

Comment 3

[Vladan Djeric (:vladan)]

That's right

Comment 4

[Gregory Szorc [:gps]]

Privacy feedback requested:

See the first few comments on what's requested.

The preferred proposal is to record a per-day boolean flag saying whether Telemetry is enabled. The alternate proposal is to record a single boolean flag "is Telemetry currently enabled."

If accepted, we would ideally like this in all channels. However, we'd settle for whatever channels we can get for the time being.

Comment 5

[Richard Newman [:rnewman]]

Attachment: Proposed patch. v1

Here's what it'd look like.

I *believe*, but don't conclusively know, that the PP flags are necessary. It seemed to fix things for me, but I think it should have worked anyway, so….

Daily data now looks like:

  {
    "_v":2,
    "isDefaultBrowser":-1,
    "isTelemetryEnabled":0
  }

I have not yet verified this in a live profile. That's up next.

Comment 6

[Gregory Szorc [:gps]]

Comment on attachment 725257 [details] [diff] [review]
Proposed patch. v1

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

Would like your opinion on version numbers before granting r+.

::: services/healthreport/Makefile.in
@@ +33,5 @@
>    $(NULL)
>  
>  MAIN_JS_MODULE := HealthReport.jsm
>  MAIN_JS_MODULE_PATH = $(FINAL_TARGET)/modules
> +MAIN_JS_MODULE_FLAGS = "-DMOZ_TELEMETRY_ON_BY_DEFAULT=$(MOZ_TELEMETRY_ON_BY_DEFAULT)"

You shouldn't need this because $(DEFINES) is always on the command line. From my local build, I see the following flags *without* this patch:

-DMOZ_TELEMETRY_DISPLAY_REV=2 -DMOZ_TELEMETRY_REPORTING=1 -DMOZ_TELEMETRY_ON_BY_DEFAULT=1 -DMOZ_DATA_REPORTING=1

But I also have "export MOZ_TELEMETRY_REPORTING=1" in my mozconfig to force Telemetry to be enabled in local builds. I'm guessing it's only-enabled-in-official-builds threw you for a loop?

Please remove changes from this file.

::: services/healthreport/providers.jsm
@@ +74,5 @@
>  AppInfoMeasurement.prototype = Object.freeze({
>    __proto__: Metrics.Measurement.prototype,
>  
>    name: "appinfo",
> +  version: 2,

Please note that because we don't have migration code in place, this will effectively lose the version 1 data stored in the DB. Because only isDefaultBrowser has longitudinal data stored, that's effectively the only data that will suddenly become missing.

I'm tempted to keep calling this version 1.

What are your thoughts?

Comment 7

[Richard Newman [:rnewman]]

(In reply to Gregory Szorc [:gps] from comment #6)

> But I also have "export MOZ_TELEMETRY_REPORTING=1" in my mozconfig to force
> Telemetry to be enabled in local builds. I'm guessing it's
> only-enabled-in-official-builds threw you for a loop?

That's probably it.

> Please remove changes from this file.

Happy to!



> What are your thoughts?

Let's just solve Bug 851544 before landing this. We're going to bump into this eventually, so we might as well have a pattern for solving it.

Comment 8

[Gregory Szorc [:gps]]

Attachment: Proposed patch, v1

I copied the old appinfo measurement and eliminated the last fields. It should still report the app versions and only the app versions in the payload. I haven't verified this but will shortly once my build has finished.

I also removed the Makefile.in changes and used a more suitable API in the tests.

Consider this r+ from me. I just want your sign-off on the old measurement behavior.

Comment 9

[Gregory Szorc [:gps]]

Hmm. I tested this out on my local profile and it is full of fail.

The good news is I see longitudinal data for both measurements:

        "org.mozilla.appInfo.appinfo": {
          "_v": 2,
          "isDefaultBrowser": 1,
          "isTelemetryEnabled": 0
        },

        "org.mozilla.appInfo.appinfo": {
          "_v": 1,
          "isDefaultBrowser": 1
        },

The bad news is:

1) isTelemetryEnabled is 0 despite the checkbox being checked in the preferences pane. Hmmm.

2) The "last" fields aren't showing up for appInfo v2. Instead, I see:

      "org.mozilla.appInfo.appinfo": {
        "_v": 1
      },

Hmmm.

Comment 10

[Gregory Szorc [:gps]]

Ugh. Remember when we moved the version outside of the measurement name (bug 830922)? Yeah, that's coming back to bite us. Since the measurement name in the mapping is now shared between two versions, last write wins. In this case, version 1 of org.mozilla.appInfo.appinfo seems to be running after version 2 and, well, suckitude.

This also exposes another bug: we include empty measurements in the payload. We should fix that. That will coincidentally fix the overwrite issue in this case. But, the general issue will continue to bite us down the road.

Do we want to move the version back to the measurement's name or do we want to do something like:

  "org.mozilla.appInfo.appinfo": {
    1: {
      "foo": "bar"
    },
    2: {
      "isDefaultBrowser": 1,
      "isTelemetryEnabled": 0
    }
  }

I prefer the measurement name. But Metrics fought so vigorously against that in bug 830922.

Or, we could change the containers for "days" and "last" into arrays instead of maps. Ugh.

Comment 11

[Gregory Szorc [:gps]]

We need to change the payload format again to properly deal with multiple versions of the same measurement. Do you have a preference?

Comment 12

[Gregory Szorc [:gps]]

Attachment: Part 1: Don't report empty measurements, v1

This is a temporary workaround for the overwriting issue. If the return value of the serializer is falsy, that thing is not added to the payload.

Comment 13

[Gregory Szorc [:gps]]

Attachment: Part 2: Don't report empty measurements, v2

Forgot to qref.

Comment 14

[Gregory Szorc [:gps]]

Attachment: Part 1: Don't give each provider its own Preferences, v1

While we set this._prefs in the ctor, it was being overwritten by initPreferences() when the provider was initialized. Therefore, this._prefs was pointing to a local branch instead of the global branch in a few providers. This is why isTelemetryEnabled wasn't reporting properly.

I looked at the providers and we weren't using the preferences objects created during initPreferences(). So I axed the feature.

As a bonus, Yoric identified these Preferences as excessive the performance reviews. Double win!

Comment 15

[Richard Newman [:rnewman]]

(In reply to Gregory Szorc [:gps] from comment #11)
> We need to change the payload format again to properly deal with multiple
> versions of the same measurement. Do you have a preference?

Let's take that to Bug 851544.

Comment 16

[Richard Newman [:rnewman]]

Comment on attachment 726311 [details] [diff] [review]
Part 2: Don't report empty measurements, v2

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

I actually think we *should* report measurements without data: that's either a bug or a feature (presence is meaningful), and for the former it would be nice to know which version has the bug!

Instead, let's solve the actual problem here: make sure the highest version number wins.

We're going to have to handle multiple version packets for Bug 851544, so let's insert the start of that logic here, and just have the conflict resolution be "take the highest version" rather than "encode some multi-version structure".

Comment 17

[Gregory Szorc [:gps]]

Landed part 2 as part 1:

https://hg.mozilla.org/integration/mozilla-inbound/rev/4cd8bd47cd1d

mreid: let's take payload discussion to bug 851544.

Comment 18

[Mark Reid [:mreid]]

(In reply to Richard Newman [:rnewman] from comment #16)
> We're going to have to handle multiple version packets for Bug 851544, so
> let's insert the start of that logic here, and just have the conflict
> resolution be "take the highest version" rather than "encode some
> multi-version structure".

Sounds good for now, moving discussion to 851544.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4cd8bd47cd1d

Comment 20

[Gregory Szorc [:gps]]

Attachment: Part 2: Use newest measurement only, v1

Changed up the patch to only record the newest version of a measurement if there are multiple measurement versions.

Comment 21

[Gregory Szorc [:gps]]

https://hg.mozilla.org/services/services-central/rev/98a625f3bf43
https://hg.mozilla.org/services/services-central/rev/35970a2baa8d

We should land bug 850483 before we merge to central so we don't get any builds with version=2 that are missing isBlocklistEnabled.

Comment 22

[Gregory Szorc [:gps]]

Attachment: Part 4: Bustage fix for part 2, v1

Derp.

Comment 23

[Richard Newman [:rnewman]]

Comment on attachment 726844 [details] [diff] [review]
Part 4: Bustage fix for part 2, v1

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

Herp. 

Please follow up with a test that this unbreaks!

Comment 24

[Gregory Szorc [:gps]]

https://hg.mozilla.org/services/services-central/rev/7c80db9ec6ec

Comment 25

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/98a625f3bf43
https://hg.mozilla.org/mozilla-central/rev/35970a2baa8d
https://hg.mozilla.org/mozilla-central/rev/7c80db9ec6ec

Comment 26

[Gregory Szorc [:gps]]

Some may have noticed that this bug has privacy-review-needed. Yet, it still landed. I'm pretty sure I had at least verbal OK from Alex to proceed with this and with its cousin, bug 850483. I am following up with Alex and Jishnu to confirm. Let's keep privacy-review-needed until we have something more formal.

Comment 27

[Gregory Szorc [:gps]]

Issues were hashed out in bug 855710. We'll move forward on Aurora uplift.

Comment 28

[Gregory Szorc [:gps]]

Comment on attachment 726408 [details] [diff] [review]
Part 1: Don't give each provider its own Preferences, v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): FHR
User impact if declined: Won't be able to correlate FHR with Telemetry until 22.
Testing completed (on m-c, etc.): This has baked for many days without issue.
Risk to taking this patch (and alternatives if risky): Low
String or IDL/UUID changes made by this patch: None

Comment 29

[Gregory Szorc [:gps]]

Comment on attachment 726770 [details] [diff] [review]
Part 2: Use newest measurement only, v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): FHR
User impact if declined: See previous
Testing completed (on m-c, etc.): See previous
Risk to taking this patch (and alternatives if risky): See previous
String or IDL/UUID changes made by this patch: None

Comment 30

[Gregory Szorc [:gps]]

Comment on attachment 725714 [details] [diff] [review]
Proposed patch, v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): FHR
User impact if declined: See previous
Testing completed (on m-c, etc.): See previous
Risk to taking this patch (and alternatives if risky): See previous
String or IDL/UUID changes made by this patch: None

Comment 31

[Gregory Szorc [:gps]]

Comment on attachment 726844 [details] [diff] [review]
Part 4: Bustage fix for part 2, v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): FHR
User impact if declined: See previous
Testing completed (on m-c, etc.): See previous
Risk to taking this patch (and alternatives if risky): See previous
String or IDL/UUID changes made by this patch: None

General Aurora uplift notes: All these patches are related and are required for uplift. All code has been sitting on Nightly for a number of days and we have observed no obvious regressions with it.

These patches are also a prerequisite to a pending uplift request in bug 850483. Both bugs should be uplifted at the same time.

Comment 32

[bhavana bajaj [:bajaj]]

Comment on attachment 725714 [details] [diff] [review]
Proposed patch, v1

low risk ,well baked patches & needed in support of new FHR feature support - approving for uplift

Comment 33

[Gregory Szorc [:gps]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f3a6fc9019f9
https://hg.mozilla.org/releases/mozilla-aurora/rev/1b45e5594489
https://hg.mozilla.org/releases/mozilla-aurora/rev/72bdfa761b1f