1920562 - Remove setEventRecordingEnabled, leaving all Legacy Telemetry events as default-on

Status: RESOLVED FIXED

(Toolkit :: Telemetry, task, P1)

Description

[Chris H-C :chutten]

Legacy Telemetry's setEventRecordingEnabled API has long been more of an annoyance than a help. (In fact, I've used it as justification to use Glean instead). It was intended to supply event instrumentors with confidence to control when and how often their events would be recorded and express our continued preference for default-off instrumentation.

At time of writing, no one uses it that way, preferring to enable event recording in code immediately preceding the recording of the events.

(Okay, so TRRService ties theirs to a pref which could be changed, but it isn't changed by code or experiment. Others could have depended on squelching events that are recorded earlier than the category being enabled, but we haven't found any that do.)

Since we're looking at deprecating and removing other Legacy Telemetry event APIs in favour of Glean (whose events, it must be said, supply a superior operational safety guarantee through Server Knobs which some folks have actually used to ship instrumentation defaulted to off), it's a good time to remove this unused and ill-understood mechanism.

This bug is about:

• Removing the setEventRecordingEnabled API and all of its uses
• Defaulting all events to enabled
• Documenting in the Legacy Telemetry event docs any risks now borne by instrumentors because of this API's removal
  • Instruct users looking for this functionality after this removal to build their own e.g. out of prefs.

Comment 1

[Chris H-C :chutten]

Attachment: Bug 1920562 - Switch all legacy events to default to enabled r?florian!,janerik!

Comment 2

[Chris H-C :chutten]

Attachment: Bug 1920562 - Remove C++ Telemetry::SetEventRecordingEnabled r?florian!

Comment 3

[Chris H-C :chutten]

Attachment: Bug 1920562 - Remove JS uses of Services.telemetry.setEventRecordingEnabled r?florian!

Comment 4

[Chris H-C :chutten]

Attachment: Bug 1920562 - Remove Services.telemetry.setEventRecordingEnabled r?florian!,janerik!

Comment 5

[Chris H-C :chutten]

Attachment: Bug 1920562 - Remove TelemetryEvents.init r?florian!

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/62ab18879eea
Switch all legacy events to default to enabled r=florian,janerik,devtools-reviewers,nchevobbe
https://hg.mozilla.org/integration/autoland/rev/cd3672fc08ed
Remove C++ Telemetry::SetEventRecordingEnabled r=florian,necko-reviewers,valentin
https://hg.mozilla.org/integration/autoland/rev/a68fd13a33ae
Remove JS uses of Services.telemetry.setEventRecordingEnabled r=florian,settings-reviewers,pip-reviewers,credential-management-reviewers,search-reviewers,devtools-reviewers,sync-reviewers,sessionstore-reviewers,omc-reviewers,migration-reviewers,firefox-desktop-core-reviewers ,urlbar-reviewers,sfoster,nchevobbe,valentin,Gijs,dimi,lina,mconley,pdahiya
https://hg.mozilla.org/integration/autoland/rev/4405387ae770
Remove Services.telemetry.setEventRecordingEnabled r=florian,janerik
https://hg.mozilla.org/integration/autoland/rev/8f085ab589a8
Remove TelemetryEvents.init r=florian

Comment 7

[Sandor Molnar[:smolnar]]

Backed out for causing xpc assertion failures

Backout link: https://hg.mozilla.org/integration/autoland/rev/54c74f7f499d14861e9e5eacd33ed21288e30bee

Push with failures

Failure log -> Assertion failure: staticPrefValue == preferenceValue (Preference 'layers.d3d11.enable-blacklist' got modified since StaticPrefs::layers_d3d11_enable_blacklist_AtStartup was initialized

Comment 8

[Chris H-C :chutten]

...what? Okay, uh... I don't know how that could happen. And it doesn't reproduce locally. Maybe a faulty merge? Lemme pop it onto try again real quick.

Comment 9

[Florian Quèze [:florian]]

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

...what? Okay, uh... I don't know how that could happen.

Bug 1822083 seems vaguely related. But no obvious solution there.

Comment 10

[Chris H-C :chutten]

try can reproduce it, but I can't. And the mystery is deepened by how it's a d3d11 pref failing tests on Linux. There's precious little code that even references the pref at all, and the manipulation causing the failure is a clearUserPref call from somewhere in JS.

The stack does do some pref work, removing network.trr.confirmation_telemetry_enabled, security.certerrors.recordEventTelemetry, and security.protectionspopup.recordEventTelemetry. None even close to layers.d3d11.*. There are no changes to gfx/**/* in the patches.

I'm at a loss. I'm going to try binary searching the patch stack to identify which patch triggers the failure and then I'll subdivide the change to try and narrow it down further, roundtripping through try each time.

Comment 11

[Chris H-C :chutten]

Reverting the top three patches (removing TelemetryEvents.init, Services.telemetry.setEventRecordingEnabled, and removing all JS calls to Services.telemetry.setEventRecordingEnabled) still results in the pref test failing, however reverting the fourth patch (removing the C++ uses of SetEventRecordingEnabled and the API) gives a clean run.

Which would suggest that it's something in this patch that's causing the test to fail.

On a hunch, I added back in the definition for the network.tss.confirmation_telemetry_enabled pref to StaticPrefList.yaml and the test now passes. So let's get this landed again.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d40aabb6a4f4
Switch all legacy events to default to enabled r=florian,janerik,devtools-reviewers,nchevobbe
https://hg.mozilla.org/integration/autoland/rev/a39cdf7de3a6
Remove C++ Telemetry::SetEventRecordingEnabled r=florian,necko-reviewers,valentin
https://hg.mozilla.org/integration/autoland/rev/d56bf9374416
Remove JS uses of Services.telemetry.setEventRecordingEnabled r=florian,extension-reviewers,settings-reviewers,pip-reviewers,credential-management-reviewers,search-reviewers,devtools-reviewers,sync-reviewers,sessionstore-reviewers,omc-reviewers,migration-reviewers,firefox-desktop-core-reviewers ,urlbar-reviewers,sfoster,nchevobbe,valentin,Gijs,dimi,lina,mconley,pdahiya,willdurand
https://hg.mozilla.org/integration/autoland/rev/96f22ea62b03
Remove Services.telemetry.setEventRecordingEnabled r=florian,janerik
https://hg.mozilla.org/integration/autoland/rev/595eef978917
Remove TelemetryEvents.init r=florian

Comment 13

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/d40aabb6a4f4
https://hg.mozilla.org/mozilla-central/rev/a39cdf7de3a6
https://hg.mozilla.org/mozilla-central/rev/d56bf9374416
https://hg.mozilla.org/mozilla-central/rev/96f22ea62b03
https://hg.mozilla.org/mozilla-central/rev/595eef978917