From 84aa2d26d51f9329e97cf07708dc5023aaabe30c Mon Sep 17 00:00:00 2001 From: Chris H-C Date: Tue, 8 Oct 2024 20:14:41 +0000 Subject: [PATCH] Bug 1920562 - Switch all legacy events to default to enabled r=florian,janerik,devtools-reviewers,nchevobbe Differential Revision: https://phabricator.services.mozilla.com/D223831 --- .../docs/contributor/frontend/telemetry.md | 3 --- image/decoders/nsAVIFDecoder.cpp | 3 --- .../extensions/schemas/telemetry.json | 2 +- toolkit/components/glean/docs/user/gifft.md | 5 ++-- .../docs/user/glean_for_legacy_events.md | 6 ----- toolkit/components/telemetry/core/Telemetry.h | 2 +- .../telemetry/core/TelemetryEvent.cpp | 25 ++++++------------- .../telemetry/core/nsITelemetry.idl | 2 +- .../telemetry/docs/collection/events.rst | 19 +++++++------- .../docs/collection/webextension-api.rst | 2 +- .../telemetry/tests/gtest/TestEvents.cpp | 4 ++- .../tests/integration/tests/conftest.py | 14 ----------- .../integration/tests/test_event_ping.py | 1 - .../tests/unit/test_TelemetryEvents.js | 5 +++- .../unit/test_TelemetryEvents_buildFaster.js | 3 ++- 15 files changed, 33 insertions(+), 63 deletions(-) diff --git a/devtools/docs/contributor/frontend/telemetry.md b/devtools/docs/contributor/frontend/telemetry.md index 5636031f7ac3..97fb0644d832 100644 --- a/devtools/docs/contributor/frontend/telemetry.md +++ b/devtools/docs/contributor/frontend/telemetry.md @@ -239,9 +239,6 @@ this._telemetry = new Telemetry(); And use the instance to report e.g. tool opening... ```js -// Event telemetry is disabled by default so enable it for your category. -this._telemetry.setEventRecordingEnabled(true); - // If you already have all the properties for the event you can send the // telemetry event using: // this._telemetry.recordEvent(method, object, value, extra) e.g. diff --git a/image/decoders/nsAVIFDecoder.cpp b/image/decoders/nsAVIFDecoder.cpp index 4bef19903421..b63f010a4bfd 100644 --- a/image/decoders/nsAVIFDecoder.cpp +++ b/image/decoders/nsAVIFDecoder.cpp @@ -673,9 +673,6 @@ class Dav1dDecoder final : AVIFDecoderInterface { // the easiest way to see if we're getting unexpected behavior to // investigate. if (aShouldSendTelemetry && r != 0) { - // Uncomment once bug 1691156 is fixed - // mozilla::Telemetry::SetEventRecordingEnabled("avif"_ns, true); - mozilla::glean::avif::Dav1dGetPictureReturnValueExtra extra = { .value = Some(nsPrintfCString("%d", r)), }; diff --git a/toolkit/components/extensions/schemas/telemetry.json b/toolkit/components/extensions/schemas/telemetry.json index 5e3c8186e44c..5cdf9c92c66a 100644 --- a/toolkit/components/extensions/schemas/telemetry.json +++ b/toolkit/components/extensions/schemas/telemetry.json @@ -364,7 +364,7 @@ { "name": "setEventRecordingEnabled", "type": "function", - "description": "Enable recording of events in a category. Events default to recording disabled. This allows to toggle recording for all events in the specified category.", + "description": "Enable recording of events in a category. Events default to recording enabled. This allows to toggle recording for all events in the specified category.", "async": true, "parameters": [ { diff --git a/toolkit/components/glean/docs/user/gifft.md b/toolkit/components/glean/docs/user/gifft.md index 5c364c53148b..78d4fded9454 100644 --- a/toolkit/components/glean/docs/user/gifft.md +++ b/toolkit/components/glean/docs/user/gifft.md @@ -156,9 +156,8 @@ Assert.equal(true, snapshot["telemetry.test.mirror_for_labeled_bool"]["1".repeat ### Telemetry Events A Glean event can be mirrored to a Telemetry Event. -Telemetry Events must be enabled before they can be recorded to via the API -`Telemetry.setEventRecordingEnabled(category, enable);`. -If the Telemetry Event isn't enabled, +If the Telemetry Event is disabled +(by calling `Services.telemetry.setEventRecordingEnabled("event.category", false);`), recording to the Glean event will still work, and the event will be Summarized in Telemetry as all disabled events are. diff --git a/toolkit/components/glean/docs/user/glean_for_legacy_events.md b/toolkit/components/glean/docs/user/glean_for_legacy_events.md index 3efec45a9acb..2854d268a877 100644 --- a/toolkit/components/glean/docs/user/glean_for_legacy_events.md +++ b/toolkit/components/glean/docs/user/glean_for_legacy_events.md @@ -24,12 +24,6 @@ To record your new event, use [the Glean `record(...)` API][glean-event-api]. To test your new event, use [the Glean `testGetValue()` API][glean-test-api]. -```{admonition} Don't Forget! -Though you're using Glean, there's still a Legacy Telemetry event underneath. -You must call `Services.telemetry.setEventRecordingEnabled("myCategory", true);` -in order for the Legacy Telemetry event to be recorded. -``` - Your Legacy Telemetry event will appear in `about:telemetry` when your code is triggered as confirmation this is all working as you expect. diff --git a/toolkit/components/telemetry/core/Telemetry.h b/toolkit/components/telemetry/core/Telemetry.h index 3e087bed814c..81c185400c21 100644 --- a/toolkit/components/telemetry/core/Telemetry.h +++ b/toolkit/components/telemetry/core/Telemetry.h @@ -551,7 +551,7 @@ class MOZ_RAII AutoScalarTimer { /** * Enables recording of events in a category. - * Events default to recording disabled. + * Events default to recording enabled. * This toggles recording for all events in the specified category. * * @param aCategory The category name. diff --git a/toolkit/components/telemetry/core/TelemetryEvent.cpp b/toolkit/components/telemetry/core/TelemetryEvent.cpp index b032fff5212f..72bb958d28d5 100644 --- a/toolkit/components/telemetry/core/TelemetryEvent.cpp +++ b/toolkit/components/telemetry/core/TelemetryEvent.cpp @@ -334,8 +334,8 @@ nsTHashMap gEventNameIDMap(kEventCount); // The CategoryName set. nsTHashSet gCategoryNames; -// This tracks the IDs of the categories for which recording is enabled. -nsTHashSet gEnabledCategories; +// This tracks the IDs of the categories for which recording is disabled. +nsTHashSet gDisabledCategories; // The main event storage. Events are inserted here, keyed by process id and // in recording order. @@ -503,7 +503,7 @@ RecordEventResult RecordEvent(const StaticMutexAutoLock& lock, processType, dynamicNonBuiltin); // Check whether this event's category has recording enabled - if (!gEnabledCategories.Contains(GetCategory(lock, eventKey))) { + if (gDisabledCategories.Contains(GetCategory(lock, eventKey))) { return RecordEventResult::Ok; } @@ -582,12 +582,6 @@ void RegisterEvents(const StaticMutexAutoLock& lock, const nsACString& category, if (aBuiltin) { gCategoryNames.Insert(category); } - - if (!aBuiltin) { - // Now after successful registration enable recording for this category - // (if not a dynamic builtin). - gEnabledCategories.Insert(category); - } } } // anonymous namespace @@ -750,9 +744,6 @@ void TelemetryEvent::InitializeGlobalState(bool aCanRecordBase, gCategoryNames.Insert(info.common_info.category()); } - // A hack until bug 1691156 is fixed - gEnabledCategories.Insert("avif"_ns); - gInitDone = true; } @@ -765,7 +756,7 @@ void TelemetryEvent::DeInitializeGlobalState() { gEventNameIDMap.Clear(); gCategoryNames.Clear(); - gEnabledCategories.Clear(); + gDisabledCategories.Clear(); gEventRecords.Clear(); gDynamicEventInfo = nullptr; @@ -1394,10 +1385,10 @@ void TelemetryEvent::SetEventRecordingEnabled(const nsACString& category, return; } - if (enabled) { - gEnabledCategories.Insert(category); + if (!enabled) { + gDisabledCategories.Insert(category); } else { - gEnabledCategories.Remove(category); + gDisabledCategories.Remove(category); } } @@ -1427,7 +1418,7 @@ size_t TelemetryEvent::SizeOfIncludingThis( } n += gCategoryNames.ShallowSizeOfExcludingThis(aMallocSizeOf); - n += gEnabledCategories.ShallowSizeOfExcludingThis(aMallocSizeOf); + n += gDisabledCategories.ShallowSizeOfExcludingThis(aMallocSizeOf); if (gDynamicEventInfo) { n += gDynamicEventInfo->ShallowSizeOfIncludingThis(aMallocSizeOf); diff --git a/toolkit/components/telemetry/core/nsITelemetry.idl b/toolkit/components/telemetry/core/nsITelemetry.idl index ddad0ea7d451..41de553eb614 100644 --- a/toolkit/components/telemetry/core/nsITelemetry.idl +++ b/toolkit/components/telemetry/core/nsITelemetry.idl @@ -531,7 +531,7 @@ interface nsITelemetry : nsISupports /** * Enable recording of events in a category. - * Events default to recording disabled. This allows to toggle recording for all events + * Events default to recording enabled. This allows to toggle recording for all events * in the specified category. * * @param aCategory The category name. diff --git a/toolkit/components/telemetry/docs/collection/events.rst b/toolkit/components/telemetry/docs/collection/events.rst index 773a63462bca..8477f916157c 100644 --- a/toolkit/components/telemetry/docs/collection/events.rst +++ b/toolkit/components/telemetry/docs/collection/events.rst @@ -164,16 +164,14 @@ events in Firefox Desktop are Services.telemetry.setEventRecordingEnabled(category, enabled); -Event recording is currently disabled by default for events registered in Events.yaml. -Dynamically-registered events (those registered using ``registerEvents()``) are enabled by default, and cannot be disabled. +Event recording is currently enabled by default for events registered in Events.yaml. +Dynamically-registered events (those registered using ``registerEvents()``) cannot be disabled. Privileged add-ons and Firefox code can enable & disable recording events for specific categories using this function. Example: .. code-block:: js - Services.telemetry.setEventRecordingEnabled("ui", true); - // ... now events in the "ui" category will be recorded. Services.telemetry.setEventRecordingEnabled("ui", false); // ... now "ui" events will not be recorded anymore. @@ -182,6 +180,11 @@ Example: Even if your event category isn't enabled, counts of events that attempted to be recorded will be :ref:`summarized `. +.. note:: + Events can be expensive to store, submit, and query. + You are responsible for ensuring that you don't submit too many events. + When your new events land in Nightly, consult with the Data Org about whether they are too "chatty". + Internal API ------------ @@ -240,14 +243,12 @@ the dynamic-process scalar ``telemetry.dynamic_event_counts`` would have a key Testing ======= -Tests involving Event Telemetry often follow this four-step form: +Tests involving Event Telemetry often follow this three-step form: 1. ``Services.telemetry.clearEvents();`` To minimize the effects of prior code and tests. -2. ``Services.telemetry.setEventRecordingEnabled(myCategory, true);`` To enable the collection of - your events. (May or may not be relevant in your case) -3. ``runTheCode();`` This is part of the test where you call the code that's supposed to collect +2. ``runTheCode();`` This is part of the test where you call the code that's supposed to collect Event Telemetry. -4. ``TelemetryTestUtils.assertEvents(expected, filter, options);`` This will check the +3. ``TelemetryTestUtils.assertEvents(expected, filter, options);`` This will check the events recorded by Event Telemetry against your provided list of expected events. If you only need to check the number of events recorded, you can use ``TelemetryTestUtils.assertNumberOfEvents(expectedNum, filter, options);``. diff --git a/toolkit/components/telemetry/docs/collection/webextension-api.rst b/toolkit/components/telemetry/docs/collection/webextension-api.rst index 997f2363b3bb..186cbb9d8482 100644 --- a/toolkit/components/telemetry/docs/collection/webextension-api.rst +++ b/toolkit/components/telemetry/docs/collection/webextension-api.rst @@ -139,7 +139,7 @@ Instead, use :doc:`Glean event definitions <../../glean/user/glean_for_legacy_ev browser.telemetry.setEventRecordingEnabled(category, enabled); -Enable recording of events in a category. Events default to recording disabled. This allows to toggle recording for all events in the specified category. +Enable recording of events in a category. Events default to recording enabled. This allows to toggle recording for all events in the specified category. * ``category`` - *(string)* The category name. * ``enabled`` - *(boolean)* Whether recording is enabled for events in that category. diff --git a/toolkit/components/telemetry/tests/gtest/TestEvents.cpp b/toolkit/components/telemetry/tests/gtest/TestEvents.cpp index 80ba982afc1a..51f03487e9f7 100644 --- a/toolkit/components/telemetry/tests/gtest/TestEvents.cpp +++ b/toolkit/components/telemetry/tests/gtest/TestEvents.cpp @@ -40,6 +40,9 @@ TEST_F(TelemetryTestFixture, RecordEventNative) { "this extra value is much too long and must be truncated to fit in the " "limit which at time of writing was 80 bytes."); + // Ensure "telemetry.test" is disabled + Telemetry::SetEventRecordingEnabled(category, false); + // Try recording before category's enabled. TelemetryEvent::RecordEventNative( Telemetry::EventID::TelemetryTest_Test1_Object1, Nothing(), Nothing()); @@ -133,7 +136,6 @@ TEST_F(TelemetryTestFixture, GIFFTValue) { const nsCString category("telemetry.test"); const nsCString method("mirror_with_extra"); const nsCString object("object1"); - Telemetry::SetEventRecordingEnabled(category, true); // Record in Glean. // We include an extra extra key (extra1, here) to ensure there's always six diff --git a/toolkit/components/telemetry/tests/integration/tests/conftest.py b/toolkit/components/telemetry/tests/integration/tests/conftest.py index 57067bedb73f..411a0fc60c43 100644 --- a/toolkit/components/telemetry/tests/integration/tests/conftest.py +++ b/toolkit/components/telemetry/tests/integration/tests/conftest.py @@ -105,20 +105,6 @@ class Browser(object): ) self.marionette.set_pref("datareporting.healthreport.uploadEnabled", False) - def enable_search_events(self): - """ - Event Telemetry categories are disabled by default. - Search events are in the "navigation" category and are not enabled by - default in builds of Firefox, so we enable them here. - """ - - script = """\ - Services.telemetry.setEventRecordingEnabled("navigation", true); - """ - - with self.marionette.using_context(self.marionette.CONTEXT_CHROME): - self.marionette.execute_script(textwrap.dedent(script)) - def enable_telemetry(self): self.marionette.instance.profile.set_persistent_preferences( {"datareporting.healthreport.uploadEnabled": True} diff --git a/toolkit/components/telemetry/tests/integration/tests/test_event_ping.py b/toolkit/components/telemetry/tests/integration/tests/test_event_ping.py index 9209c562ebe2..fa440518f9a5 100644 --- a/toolkit/components/telemetry/tests/integration/tests/test_event_ping.py +++ b/toolkit/components/telemetry/tests/integration/tests/test_event_ping.py @@ -10,7 +10,6 @@ def test_event_ping(browser, helpers): Barebones test for "event" ping: Search, close Firefox, check "event" ping for search events. """ - browser.enable_search_events() browser.wait_for_search_service_init() browser.search("mozilla firefox") diff --git a/toolkit/components/telemetry/tests/unit/test_TelemetryEvents.js b/toolkit/components/telemetry/tests/unit/test_TelemetryEvents.js index 3d5e8269141c..5c20a70a54cb 100644 --- a/toolkit/components/telemetry/tests/unit/test_TelemetryEvents.js +++ b/toolkit/components/telemetry/tests/unit/test_TelemetryEvents.js @@ -170,7 +170,10 @@ add_task(async function test_recording_state() { ["telemetry.test.second", "test", "object1"], ]; - // Both test categories should be off by default. + Telemetry.setEventRecordingEnabled("telemetry.test", false); + Telemetry.setEventRecordingEnabled("telemetry.test.second", false); + + // Both test categories should now be off. events.forEach(e => Telemetry.recordEvent(...e)); TelemetryTestUtils.assertEvents([]); checkEventSummary( diff --git a/toolkit/components/telemetry/tests/unit/test_TelemetryEvents_buildFaster.js b/toolkit/components/telemetry/tests/unit/test_TelemetryEvents_buildFaster.js index c7e9e5aaba58..9643e95f89bf 100644 --- a/toolkit/components/telemetry/tests/unit/test_TelemetryEvents_buildFaster.js +++ b/toolkit/components/telemetry/tests/unit/test_TelemetryEvents_buildFaster.js @@ -244,7 +244,8 @@ add_task(async function test_dynamicBuiltinEventsDisabledByDefault() { }); // Record some events. - // Explicitely _don't_ enable the category + // Explicitely disable the category + Telemetry.setEventRecordingEnabled(TEST_EVENT_NAME, false); Telemetry.recordEvent(TEST_EVENT_NAME, "test1", "object1"); // Now check that the snapshot contains the expected data.