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.

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?

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?

(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.

(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.

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

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.

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?

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.

Docs update will follow up in a separate patch.

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.

Also, please add test-coverage.

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

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

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

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"

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.

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

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

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.