Add extensionTelemetry #10057

strseb · 2024-11-20T13:37:20Z

Description

This PR adds commands for the extension to send telemetry to the client.
It's a bit bigger, therefore i've split this across 3 commits, for easyier reivew.

In : d6e1d77
I've only added the metrics, the data review happens in a google doc, i can link that if it's done.
In a897f14
I add 3 commands: {t:telemetry, name: metric_name: args: any} {t:start_session} {t:stop_session} - Those allow the extension to send any events or counts. For the bools we save them and attach them to the next ping, similar how we do that with our settings related bools.
fa836d6
I tried the least refactoring solution to allow us to enable telemetry either for the extension or the client. All data is attached to a ping with a name. I've added the ability for glean to filter out on a per ping_name basis. I use that to filter the extensions pings if they are not enabled and the client pings if those are disabled.

mcleinman · 2024-11-25T17:44:58Z

This can have an updated title (dropping WIP), right?

strseb · 2024-11-25T17:58:47Z

@mcleinman yes sorry :)

mcleinman

Thanks for this! Several questions.

It seems like we're doing a bunch of work to allow the user to have glean enabled for the extension but not the client (or the reverse). I'm not sure this is worth the code needed for it - was that a product directive?

src/telemetry/pings.yaml

src/telemetry/extension_metrics.yaml

mcleinman · 2024-11-25T17:58:08Z

src/telemetry/extension_metrics.yaml

+      description: >
+        True if the user has disabled the VPN for firefox


Over what length of time are we measuring this? I believe we default to ping in Glean... is this supposed to be "has disabled this ever"? Or "has disabled this in the current session"?

The metric has a lifetime of ping (and will be attached similar to using_system_language) , the underlying info lives in the Settings and will be cleaned on logout.
We want to measure "has this user ever ever used feature x?"

But unlike using_system_language, we're sending it in every start ping (in recordAllFlags). This seems more appropriate for a lifetime of user - a lifetime of ping where we record it from a app-saved setting doesn't feel like how we'd typically handle Glean.

Removed that part :)

src/telemetry/extension_metrics.yaml

src/glean/mzglean.cpp

strseb · 2024-11-25T21:34:35Z

It seems like we're doing a bunch of work to allow the user to have glean enabled for the extension but not the client (or the reverse). I'm not sure this is worth the code needed for it - was that a product directive?

Yeeeep. Product ask (q_q )" - for the extension we have to offer an extra data opt-in flow just for data related to the extension.
And we the fear is otherwise people press "yes" to collect data but still have glean disabled so nothing gets out.

oskirby · 2024-11-25T22:32:59Z

Maybe this is a silly question to ask, but why do we need an API for the extension send telemetry to the client, only for the client to turn around and send to glean? Couldn't the extension send the telemetry to glean on its own? Surely Mozilla already has some solid mechanisms for intstrumenting extensions in this way?

mcleinman · 2024-11-26T00:03:09Z

src/settingslist.h

@@ -783,3 +785,23 @@ SETTING_BOOL(addonApiSetting,        // getter
             false                   // sensitive (do not log)
 )
 #endif
+
+SETTING_UINT(extensionTelemetryFlags,        // getter


This feels like an anti-pattern for Glean, and has some possible data retention concerns. The point of Glean is that it handles recording our metrics, and the lifetimes should be able to handle our use cases.

You are totally right, TIL about lifetime =! ping - ripped all of that code out and moved that into the metrics.yaml

strseb · 2024-12-03T18:58:16Z

Maybe this is a silly question to ask, but why do we need an API for the extension send telemetry to the client, only for the client to turn around and send to glean? Couldn't the extension send the telemetry to glean on its own? Surely Mozilla already has some solid mechanisms for intstrumenting extensions in this way?

That is a fair question! - Glean.js is not supported for extensions anymore, so noone knows what would break, there is browser.telemetry forwarding events to firefox glean but for that we need to be privileged, which is a no for a lot of reasons.
Therefore we're here

mcleinman · 2024-12-03T20:43:50Z

src/telemetry/pings.yaml

@@ -73,3 +73,23 @@ daemonsession:
      (Android only) Sent every three hours during an active VPN session.
    daemon_end: >
      Sent after as session ending metric is recorded.
+extensionsession:


Nit: There were two line breaks, I thought there should be just one (but wasn't clear, sorry). It was updated to be zero.

src/telemetry/extension_metrics.yaml

mcleinman

Looks good. A few nits, and one metric typo that I think should be fixed before merging. Thank you!

src/glean/mzglean.cpp

mcleinman · 2024-12-03T20:47:27Z

src/telemetry/extension_metrics.yaml

+        - [email protected]
+      expires: 2025-06-30
+
+    fx_protetion_disabled:


More than other typos, I think this typo should be fixed before merging as it's difficult to change metric names.

Suggested change

fx_protetion_disabled:

fx_protection_disabled:

mcleinman · 2024-12-03T20:48:05Z

src/telemetry/extension_metrics.yaml

+              subscription_needed, signin_needed, split_tunneled
+          type: string
+      expires: 2025-06-30
+    fx_protection_enabled:


Suggested change

fx_protection_enabled:

fx_protection_enabled:

Nit: Line break

- Added extension_metrics.yaml file to the telemetry directory. - Defined new metrics for extension usage, including onboarding completion, feature usage, and website exclusions. - Added extension_metrics.yaml to the list of metrics sent in the extensionsession ping. - Updated pings.yaml to include extensionsession as a new ping type. - Included client ID in extensionsession ping. - Updated bugs, data reviews, and notification emails for the new metrics. - Added reasons for sending extensionsession ping on session start and end. - Updated comments to reflect changes.

- Added a new `WebExtensionTelemetry` class to handle WebExtension telemetry events. - Implemented `fromJson` function to parse JSON telemetry data into `TelemetryInfo` objects. - Added `recordTelemetry` function to record telemetry events to Glean. - Implemented `startSession` and `stopSession` functions to track session start and end events. - Added new handlers for various telemetry events and flags in `handlers.cpp`. - Updated `webextensionadapter.cpp` to handle telemetry requests. - Updated `pings.yaml` to include new telemetry pings. - Updated `webextensiontelemetry.h` to define new telemetry functions and structures.

- Update MZGlean::upadateUploadEnabled() to MZGlean::updateUploadEnabled() for consistency in naming. - Ensure that the static function updateUploadEnabled() is called correctly in the MZGlean class. - Update the documentation comments for clarity and consistency.

- Changed the function declaration of `flag` from `consteval` to `constexpr` to ensure that it is a compile-time constant expression. - This change ensures that the function is not evaluated at runtime, improving performance and avoiding potential issues with runtime evaluation.

This reverts commit 2aa9528.

* Add extension metrics to extension session ping - Added extension_metrics.yaml file to the telemetry directory. - Defined new metrics for extension usage, including onboarding completion, feature usage, and website exclusions. - Added extension_metrics.yaml to the list of metrics sent in the extensionsession ping. - Updated pings.yaml to include extensionsession as a new ping type. - Included client ID in extensionsession ping. - Updated bugs, data reviews, and notification emails for the new metrics. - Added reasons for sending extensionsession ping on session start and end. - Updated comments to reflect changes. * Add support for WebExtension Telemetry - Added a new `WebExtensionTelemetry` class to handle WebExtension telemetry events. - Implemented `fromJson` function to parse JSON telemetry data into `TelemetryInfo` objects. - Added `recordTelemetry` function to record telemetry events to Glean. - Implemented `startSession` and `stopSession` functions to track session start and end events. - Added new handlers for various telemetry events and flags in `handlers.cpp`. - Updated `webextensionadapter.cpp` to handle telemetry requests. - Updated `pings.yaml` to include new telemetry pings. - Updated `webextensiontelemetry.h` to define new telemetry functions and structures. * Allow filtering on a per-ping basis * Use glean lifetimes instead of homebrewed stuff * Address feedback in metrics_yaml * Address feedback in pings.yaml * fix typos in c++ * Update upload enablement in MZGlean - Update MZGlean::upadateUploadEnabled() to MZGlean::updateUploadEnabled() for consistency in naming. - Ensure that the static function updateUploadEnabled() is called correctly in the MZGlean class. - Update the documentation comments for clarity and consistency. * Update flag handling in webextensiontelemetry.cpp - Changed the function declaration of `flag` from `consteval` to `constexpr` to ensure that it is a compile-time constant expression. - This change ensures that the function is not evaluated at runtime, improving performance and avoiding potential issues with runtime evaluation.

strseb force-pushed the basti/telemetry_ext branch 3 times, most recently from f61f638 to fa836d6 Compare November 25, 2024 13:48

strseb requested review from oskirby, lesleyjanenorton and mcleinman and removed request for oskirby and lesleyjanenorton November 25, 2024 13:57

strseb marked this pull request as ready for review November 25, 2024 13:58

strseb changed the title ~~WIP Add extensionTelemetry~~ Add extensionTelemetry Nov 25, 2024

mcleinman reviewed Nov 25, 2024

View reviewed changes

src/glean/mzglean.cpp Outdated Show resolved Hide resolved

mcleinman reviewed Nov 26, 2024

View reviewed changes

strseb requested a review from mcleinman December 3, 2024 18:53

mcleinman reviewed Dec 3, 2024

View reviewed changes

mcleinman approved these changes Dec 3, 2024

View reviewed changes

strseb added 7 commits December 4, 2024 15:05

Allow filtering on a per-ping basis

de51b27

Use glean lifetimes instead of homebrewed stuff

7c25c91

Address feedback in metrics_yaml

61efc04

Address feedback in pings.yaml

aed0e0e

fix typos in c++

dd36fa3

strseb added 2 commits December 4, 2024 15:07

strseb force-pushed the basti/telemetry_ext branch from 077c633 to 5646001 Compare December 4, 2024 14:11

strseb merged commit 2aa9528 into main Dec 4, 2024
120 checks passed

strseb deleted the basti/telemetry_ext branch December 4, 2024 15:28

strseb added a commit that referenced this pull request Dec 4, 2024

Revert "Add extensionTelemetry (#10057)"

fa90d03

This reverts commit 2aa9528.

strseb mentioned this pull request Dec 10, 2024

basti/uplifts #10119

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add extensionTelemetry #10057

Add extensionTelemetry #10057

strseb commented Nov 20, 2024 •

edited

Loading

mcleinman commented Nov 25, 2024

strseb commented Nov 25, 2024

mcleinman left a comment

mcleinman Nov 25, 2024

strseb Nov 25, 2024 •

edited

Loading

mcleinman Nov 26, 2024

strseb Dec 3, 2024

strseb commented Nov 25, 2024

oskirby commented Nov 25, 2024

mcleinman Nov 26, 2024

strseb Dec 3, 2024

strseb commented Dec 3, 2024

mcleinman Dec 3, 2024

mcleinman left a comment

mcleinman Dec 3, 2024

mcleinman Dec 3, 2024

mcleinman Dec 3, 2024

		description: >
		True if the user has disabled the VPN for firefox

Add extensionTelemetry #10057

Add extensionTelemetry #10057

Conversation

strseb commented Nov 20, 2024 • edited Loading

Description

mcleinman commented Nov 25, 2024

strseb commented Nov 25, 2024

mcleinman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

strseb Nov 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

strseb commented Nov 25, 2024

oskirby commented Nov 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

strseb commented Dec 3, 2024

Choose a reason for hiding this comment

mcleinman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

strseb commented Nov 20, 2024 •

edited

Loading

strseb Nov 25, 2024 •

edited

Loading