Closed Bug 1562947 Opened 4 months ago Closed 3 months ago

about:telemetry is still using DATASET_RELEASE_CHANNEL_OPTIN

Categories

(Toolkit :: Telemetry, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla69
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- wontfix
firefox68 --- wontfix
firefox69 --- fixed

People

(Reporter: chutten, Assigned: chutten)

References

(Regression)

Details

(Keywords: regression)

Attachments

(1 file)

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.

Gah, sorry for missing this. :-(

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

Pushed by chutten@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/044ef8683e24
Snapshot events from all channels' datasets in about:telemetry r=janerik

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

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

Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69

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:
Attachment #9075425 - Flags: approval-mozilla-beta?

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

Flags: needinfo?(chutten)

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

Flags: needinfo?(chutten)
Attachment #9075425 - Flags: approval-mozilla-beta?
You need to log in before you can comment on or make changes to this bug.