1222070 - Make TelemetrySession respect the extended telemetry settings w\o having to restart

Status: RESOLVED INVALID

(Toolkit :: Telemetry, defect, P4)

Description

[Alessio Placitelli [:Dexter]]

As of now, disabling the extended telemetry isn't immediately reflected in TelemetrySession, a browser restart is required.

We could improve TelemetrySession by checking both |canRecordExtended| and "toolkit.telemetry.enabled" when building the payload.

Comment 1

[Alessio Placitelli [:Dexter]]

Attachment: bug1222070.patch

Comment 2

[Alessio Placitelli [:Dexter]]

There's an interesting implication with the changes in this patch.

If we immediately stop recording data as soon as toolkit.telemetry.enabled is turned off without waiting for FF to restart (to update Telemetry.canRecordExtended), then we end up with the next subsessions having no extended fields and environment.settings.telemetryEnabled = true (at least ideally, see bug 1225187). This could probably affect server-side analyses, as there could be pings with telemetryEnabled = true with no extended data.

On the other hand, if we don't honour the preference as soon as it's flipped off, we could end up with problems with session spanning through multiple days without a browser restart (common on Mac?).

Vladan, do you think it makes sense to stop recording extended data promptly even though it may cause discrepancies server-side?

Comment 3

[Vladan Djeric (:vladan)]

Will opting out of extended telemetry start a new Telemetry subsession? We definitely don't want to have a single subsession both collecting & not collecting extended Telemetry.
At the very least, we should somehow flag that the extended Telemetry from the ping should be ignored because extendedTelemetry opt-out status changed during the subusession.

Overall, it doesn't seem very valuable to support the disabling of extended Telemetry at runtime if we don't add the same support for base set Telemetry. Is that in the plans?

Comment 4

[Georg Fritzsche [:gfritzsche]]

(In reply to Vladan Djeric (:vladan) -- please needinfo! PTO Nov 16/19/20 from comment #3)
> Will opting out of extended telemetry start a new Telemetry subsession? We
> definitely don't want to have a single subsession both collecting & not
> collecting extended Telemetry.

Alessio, can you check?

> Overall, it doesn't seem very valuable to support the disabling of extended
> Telemetry at runtime if we don't add the same support for base set
> Telemetry. Is that in the plans?

That is not an issue:
This bug is about not submitting the extended Telemetry data in pings when the user opted out of that.
If the users pref choices mean "don't send base data" then we will not send pings at all.

Comment 5

[Alessio Placitelli [:Dexter]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #4)
> (In reply to Vladan Djeric (:vladan) -- please needinfo! PTO Nov 16/19/20
> from comment #3)
> > Will opting out of extended telemetry start a new Telemetry subsession? We
> > definitely don't want to have a single subsession both collecting & not
> > collecting extended Telemetry.
> 
> Alessio, can you check?

Right now we don't break a subsession when disabling extended Telemetry, no. It makes sense to do it, as Vladan pointed out.

Comment 6

[Alessio Placitelli [:Dexter]]

Vladan, the previous comments should settle the open questions. Are we good to go or is there any other issue you can think of?

Comment 7

[Vladan Djeric (:vladan)]

That's all I could think of. Make sure you support the case where the user opts-in and opts-out in a single session.

Comment 8

[Alessio Placitelli [:Dexter]]

Attachment: bug1222070.patch

Comment 9

[Alessio Placitelli [:Dexter]]

With the attached patch, we break the subsession when telemetry is flipped on/off.

We have an edge case though: since on environment changes we're reporting the "old environment" (the environment the change happened in), when the user turns off Telemetry, we will generate a ping with |settings.telemetryEnabled = true| and no extended telemetry data. 

On the other hand, when turning telemetry back on, we will have the opposite situation: telemetryEnabled to false and extended data in the ping.

Could this create any problem?

Comment 10

[Roberto Agostino Vitillo (:rvitillo)]

As flipping that pref should be a rare event I don't expect this to cause any issues, as long we document this behaviour somewhere. That said, it would be nice to don't have an edge case at all.

Comment 11

[Alessio Placitelli [:Dexter]]

Docs update will follow up in a separate patch.

Comment 12

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8693532 [details] [diff] [review]
bug1222070.patch

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

::: toolkit/components/telemetry/TelemetryEnvironment.jsm
@@ +824,5 @@
>      }
> +
> +    // We provide the value for this pref through "settings.telemetryEnabled", but we
> +    // want to know when it's turned off to trigger an environment change.
> +    Preferences.observe(PREF_TELEMETRY_ENABLED, this._onPrefChanged, this);

Let's not re-use the pref path here, that's confusing - we are using that specifically for our general _watchedPrefs implementation.
Let's keep that separate for initialization and listener.

::: toolkit/components/telemetry/TelemetrySession.jsm
@@ +214,5 @@
>  /**
> + * Check if extended Telemetry is enabled.
> + * @return {Boolean} True if extended Telemetry is enabled, false otherwise.
> + */
> +function getExtendedTelemetryEnabled() {

We wanted to base this on "toolkit.telemetry.enabled" as we don't update .canRecordExtended.

@@ +711,5 @@
>        appTimestamps = o.TelemetryTimestamps.get();
>      } catch (ex) {}
>  
>      // Only submit this if the extended set is enabled.
> +    if (!Utils.isContentProcess && getExtendedTelemetryEnabled()) {

Didn't we want to pass the "extended" flag down from one central "generate payload" function?
Otherwise this becomes hard to reason about.

Comment 13

[Georg Fritzsche [:gfritzsche]]

Also, please add test-coverage.

Comment 14

[Alessio Placitelli [:Dexter]]

Unassigning, as I'm moving on to a more important bug.

Comment 16

[Georg Fritzsche [:gfritzsche]]

Manoj, would you be interested in picking this up?
I can add more info, especially about adding a test, here.

Comment 17

[simplyblue24]

Hi, let me know how to add the test coverage, please do add more info.Thanks.

Comment 18

[Georg Fritzsche [:gfritzsche]]

The problem here comes from switching the extended data collection on & off here:
about:preferences -> advanced -> data choices -> "Share additional data"

The naming in the UI can be confusing, since we reworked the two data collection systems last year to become one.
Both the "core"/"base"/"opt-out" measurements as well as the "extended"/"opt-in" measurements are now collected with Telemetry and the UI above toggles collecting "extended" measurements by flipping the preference "toolkit.telemetry.enabled".

Now, our code in TelemetrySession.jsm doesn't respect flipping that preference during a session as well as it should, that is what this bug is about.

Alessios patch already takes it towards what we should do, but has some issues as pointed out above.
We also need to add testing, high-level we want to check that:

(1) Sending a ping with extended Telemetry turned off - it should contains the right data (no "extended" data, right value for environment.settings.telemetryEnabled)
(2) Turning extended Telemetry on and sending a ping - it should contain "extended" data
(3) Turning extended Telemetry off again and sending a ping - it should not contain "extended" data

We should add this to test_TelemetrySession.js.
Some notes for this:
* you'll want to mimic the cleanup of other test functions, e.g. test_userIdleAndSchedlerTick()
* use Preferences.set() to change a pref
* changing the telemetry pref should trigger a new ping being sent
* we throttle ping generation though, you have to move the clock forward using e.g.:
  now = fakeNow(futureDate(now, 10 * MILLISECONDS_PER_MINUTE));
* to get the next ping that was sent out, use: ping = yield PingServer.promiseNextPing()
* then you can check ping contents that the patch affects, e.g.
  * ping.environment.settings.telemetryEnabled
  * ping.payload.log
  * ping.payload.histograms containing "TELEMETRY_TEST_COUNT"
  * ping.payload.keyedHistograms containing "TELEMETRY_TEST_KEYED_COUNT"

Comment 19

[simplyblue24]

Hi, can you please review the "pseudo code", and tell me, where am i going in the right direction - https://pastebin.mozilla.org/8860616 .Is this the right way to run the test file : "./mach mochitest toolkit/components/telemetry/tests/unit/test_TelemetrySession.js" Thanks.

Comment 20

[Deepjyoti Mondal]

@Georg Is this bug still valid? I would like to work on this, how should I proceed?

Comment 21

[Georg Fritzsche [:gfritzsche]]

I don't think this is a good mentored bug at this point.

Comment 22

[Chris H-C :chutten]

Firefox for Desktop no longer has runtime-manipulable settings for extended/base collection (it's controlled by the release channel). Fennec does but our focus is on its Fenix evolution at this point, so we're not likely to move things here.