feat: Add Snap export handler usage tracking #3281

david0xd · 2025-03-31T15:59:18Z

This PR moves MetaMetrics tracking of Snap export usage to Snap Controller.

Related issue: #3259

Extension integration PR: MetaMask/metamask-extension#31553
Mobile integration PR: MetaMask/metamask-mobile#14428

packages/snaps-controllers/src/snaps/SnapController.ts

Use registry for metadata Replace controller with hook approach Fix broken unit tests Add unit tests

codecov · 2025-04-08T10:13:06Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.80%. Comparing base (88142f9) to head (a28d1d7).
Report is 11 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3281      +/-   ##
==========================================
+ Coverage   94.73%   97.80%   +3.07%     
==========================================
  Files         518      364     -154     
  Lines       11947     9940    -2007     
  Branches     1836     1622     -214     
==========================================
- Hits        11318     9722    -1596     
+ Misses        629      218     -411

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

packages/snaps-controllers/src/snaps/SnapController.ts

packages/snaps-controllers/src/utils.ts

packages/snaps-controllers/src/snaps/SnapController.test.tsx

packages/snaps-controllers/src/snaps/SnapController.ts

FrederikBolding · 2025-04-11T11:09:59Z

packages/snaps-controllers/src/snaps/SnapController.ts

@@ -1025,6 +1046,25 @@ export class SnapController extends BaseController<
    Object.values(this.state?.snaps ?? {}).forEach((snap) =>
      this.#setupRuntime(snap.id),
    );
+
+    this.#trackSnapExport = throttleTracking(


We should check with Christian if we want to track exports for preinstalled Snaps too

Can you explain what would be the difference here? Why aren't they tracked with this?

For other metrics, we don't always track preinstalled Snaps because they have a lot of usage and spam the metrics a bit.

E.g. https://github.com/MetaMask/metamask-extension/blob/main/app/scripts/metamask-controller.js#L3048

It makes sense to skip it then. Added some logic to skip tracking of preinstalled Snaps.

FrederikBolding · 2025-04-11T11:14:39Z

packages/snaps-controllers/src/snaps/registry/json.ts

-  async #getMetadata(snapId: string): Promise<SnapsRegistryMetadata | null> {
-    const database = await this.#getDatabase();
-    return database?.verifiedSnaps[snapId]?.metadata ?? null;
+  #getMetadata(snapId: string): SnapsRegistryMetadata | null {


Technically this is a breaking change, but I don't think we use this anywhere?

cc @Mrtenz

Doesn't seem to be used anywhere in the extension outside of the SnapController at least, but like you said it's still a breaking change technically. If we want to avoid breaking it, we could keep the function async for now, and pair removing it with another breaking change in the future.

I haven't found use case for this method anywhere so far 🤔
And it is working this way because we have update called at first.

So the snap_category is available and taken from the cached registry metadata.

PS. this is done because we wanted to avoid having async inside tracking.

IMO not worth it to keep it async, we don't want the performance hit of potentially updating the registry just for analytics

I mean keeping the function signature the same, but returning the result without fetching it asynchronously.

#3281 (comment)

FrederikBolding · 2025-04-11T11:16:39Z

packages/snaps-controllers/src/utils.ts

+      origin,
+    ];
+
+    if (!previousCalls.has(key) || now - lastCall >= timeout) {


Suggested change

if (!previousCalls.has(key) || now - lastCall >= timeout) {

if (now - lastCall >= timeout) {

I don't think we need the extra condition since we have a default for lastCall

FrederikBolding · 2025-04-11T11:21:12Z

packages/snaps-controllers/src/snaps/SnapController.ts

-  async getRegistryMetadata(
-    snapId: SnapId,
-  ): Promise<SnapsRegistryMetadata | null> {
+  getRegistryMetadata(snapId: SnapId): SnapsRegistryMetadata | null {
    this.#assertCanUsePlatform();
-    return await this.messagingSystem.call('SnapsRegistry:getMetadata', snapId);
+    return this.messagingSystem.call('SnapsRegistry:getMetadata', snapId);
  }


Honestly this function should probably be deleted, I have no idea why you would want to call SnapController:getRegistryMetadata when you can just use the registry directly

I don't know, probably I saw it exist and wanted to be consistent in some way. But, it's probably redundant.

Updated now with direct call to SnapsRegistry:getMetadata, and also removed getRegistryMetadata function.

Oh I don't mean why you specifically would do it for this PR. I mean why would anyone call this function that we expose 😂

packages/snaps-controllers/src/snaps/SnapController.ts

david0xd self-assigned this Mar 31, 2025

FrederikBolding reviewed Apr 1, 2025

View reviewed changes

packages/snaps-controllers/src/snaps/SnapController.ts Outdated Show resolved Hide resolved

FrederikBolding reviewed Apr 1, 2025

View reviewed changes

packages/snaps-controllers/src/snaps/SnapController.ts Outdated Show resolved Hide resolved

This was referenced Apr 3, 2025

feat(Snaps): Change Snap export tracking approach MetaMask/metamask-extension#31553

Draft

feat(snaps): Add Snap export tracking hook to Snap controller init MetaMask/metamask-mobile#14428

Draft

david0xd force-pushed the dd/add-handler-tracking branch from d7e1569 to 5006f1a Compare April 4, 2025 12:41

david0xd added 2 commits April 8, 2025 12:03

Add Snap export handler usage tracking

1aeb847

Use registry for metadata Replace controller with hook approach Fix broken unit tests Add unit tests

Fix coverage after rebase

8bd8e34

david0xd force-pushed the dd/add-handler-tracking branch from 0683058 to 8bd8e34 Compare April 8, 2025 10:06

david0xd marked this pull request as ready for review April 8, 2025 10:17

david0xd requested a review from a team as a code owner April 8, 2025 10:17

FrederikBolding reviewed Apr 9, 2025

View reviewed changes

packages/snaps-controllers/src/snaps/SnapController.ts Outdated Show resolved Hide resolved

FrederikBolding reviewed Apr 9, 2025

View reviewed changes

packages/snaps-controllers/src/utils.ts Show resolved Hide resolved

david0xd added 3 commits April 9, 2025 15:21

Remove category

1b270f5

Remove async from registry metadata and tracking

db91709

Update throttling logic and tests

3c315dd

FrederikBolding reviewed Apr 11, 2025

View reviewed changes

packages/snaps-controllers/src/snaps/SnapController.test.tsx Outdated Show resolved Hide resolved

FrederikBolding reviewed Apr 11, 2025

View reviewed changes

packages/snaps-controllers/src/snaps/SnapController.ts Show resolved Hide resolved

FrederikBolding reviewed Apr 11, 2025

View reviewed changes

Rearrange unit tests

79460d7

FrederikBolding reviewed Apr 11, 2025

View reviewed changes

packages/snaps-controllers/src/snaps/SnapController.ts Outdated Show resolved Hide resolved

david0xd added 5 commits April 11, 2025 13:37

Update call to get registry metadata

5ab593b

Remove unused getRegistryMetadata

318f0c4

Refactor tracking call

c4dd8fb

Add logic optimization for throttling function

670dbbc

Skip preinstalled Snaps tracking

a28d1d7

david0xd changed the title ~~Add Snap export handler usage tracking~~ feat: Add Snap export handler usage tracking Apr 16, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add Snap export handler usage tracking #3281

feat: Add Snap export handler usage tracking #3281

david0xd commented Mar 31, 2025 •

edited

Loading

codecov bot commented Apr 8, 2025 •

edited

Loading

FrederikBolding Apr 11, 2025

david0xd Apr 11, 2025

FrederikBolding Apr 11, 2025

david0xd Apr 16, 2025

FrederikBolding Apr 11, 2025

Mrtenz Apr 11, 2025

david0xd Apr 11, 2025

FrederikBolding Apr 11, 2025

Mrtenz Apr 11, 2025

FrederikBolding Apr 11, 2025

FrederikBolding Apr 11, 2025 •

edited

Loading

david0xd Apr 11, 2025

FrederikBolding Apr 11, 2025

david0xd Apr 11, 2025

FrederikBolding Apr 11, 2025

	if (!previousCalls.has(key) \|\| now - lastCall >= timeout) {
	if (now - lastCall >= timeout) {

feat: Add Snap export handler usage tracking #3281

Are you sure you want to change the base?

feat: Add Snap export handler usage tracking #3281

Conversation

david0xd commented Mar 31, 2025 • edited Loading

codecov bot commented Apr 8, 2025 • edited Loading

Codecov Report

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FrederikBolding Apr 11, 2025 • edited Loading

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

Choose a reason for hiding this comment

david0xd commented Mar 31, 2025 •

edited

Loading

codecov bot commented Apr 8, 2025 •

edited

Loading

FrederikBolding Apr 11, 2025 •

edited

Loading