1562947 - about:telemetry is still using DATASET_RELEASE_CHANNEL_OPTIN

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P1)

Description

[Chris H-C :chutten]

Oh, JavaScript.

aboutTelemetry.js was accidentally switched back to use DATASET_RELEASE_CHANNEL_OPTIN when getting event snapshots. This is undefined, which is coerced to 0, which is taken to mean DATASET_ALL_CHANNELS.

We need to put this back to DATASET_PRERELEASE_CHANNELS to get all of the events.

Comment 1

[:Gijs (he/him)]

Gah, sorry for missing this. :-(

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

We should make the failure (coercion) case default to either nothing or opt-in.

Comment 3

[Chris H-C :chutten]

Attachment: Bug 1562947 - Snapshot events from all channels' datasets in about:telemetry r?janerik

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/044ef8683e24
Snapshot events from all channels' datasets in about:telemetry r=janerik

Comment 5

[Chris H-C :chutten]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #2)

We should make the failure (coercion) case default to either nothing or opt-in.

I'm not sure if this was the designed intent, but the coercing case defaults to "report less" which is a good default for data reporting. I'd like to keep it this way, and instead consider unifying the snapshot APIs: events are the only ones that suffer this problem because the other two were rewritten as getSnapshotsForX calls which choose the appropriate dataset at the TelemetryImpl layer.

(( This would likely be undertaken as part of a "support multi-store for events" work which may be required under the Ecosystem Telemetry banner later this year, which is why it hasn't yet happened ))

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Chris H-C :chutten from comment #5)

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #2)

We should make the failure (coercion) case default to either nothing or opt-in.

I'm not sure if this was the designed intent, but the coercing case defaults to "report less" which is a good default for data reporting. I'd like to keep it this way, and instead consider unifying the snapshot APIs: events are the only ones that suffer this problem because the other two were rewritten as getSnapshotsForX calls which choose the appropriate dataset at the TelemetryImpl layer.

(( This would likely be undertaken as part of a "support multi-store for events" work which may be required under the Ecosystem Telemetry banner later this year, which is why it hasn't yet happened ))

Okay thanks, I misinterpreted what DATASET_ALL_CHANNELS meant.

Comment 7

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/044ef8683e24

Comment 8

[Chris H-C :chutten]

Comment on attachment 9075425 [details]
Bug 1562947 - Snapshot events from all channels' datasets in about:telemetry r?janerik

Beta/Release Uplift Approval Request

• User impact if declined: Users won't be able to see all unreported collected Events until after they are sent. about:telemetry isn't a widely-used feature but it is user-visible.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): It's small, it's in an infrequently-used feature, and if the value isn't present it defaults to precisely the bug case.
• String changes made/needed:

Comment 9

[:Gijs (he/him)]

Does this affect 68? Then you probably want to set the status flag to affected and request release + esr68 approval, but given that we're spinning rc builds I expect the bar might be quite high...

Comment 10

[Chris H-C :chutten]

Ack, whoops, yeah. Misread the calendar. Not needed for uplift or ESR.