feat: Adding "cookie id" to metrics event #26697

NiranjanaBinoy · 2024-08-27T20:39:01Z

Description

The PR sets up a stream communication with the label 'metamask-cookie-handler'. The data sent from the page will be intercepted at the content-script and forwarded to the extension. The 'ga_client_id' sent from the page is saved as marketingCampaignCookieId in the metaMetricsController state and fetched when building the context for each event.

Related issues

Part of https://github.com/MetaMask/MetaMask-planning/issues/2916

Manual testing steps

To test the stream, Open https://metamask.io/ , and write a request in dev tools console with the following window.postMessage would do. This should update the metametricsController state with marketingCampaignCookieId. Logging the payload in _track() in MetaMetricsController will show that the marketingCampaignCookieId is added to the context.

window.postMessage(
      {
        target: 'metamask-contentscript', 
        data: {
          name: 'metamask-cookie-handler', 
          data: {
            jsonrpc: '2.0',
            method: 'getCookieFromMarketingPage',
            params: [{ ga_client_id: 12345, origin: 'https://metamask.io' }],
            id: '1234',
            origin: 'https://metamask.io',
          },
        },
      },
      'https://metamask.io',
    );

Test the track event sending after this has this id in its context as marketingCampaignCookieId.

Screenshots/Recordings

After

Loading a website in the from the white list:

whitelist-website.mov

Initially loading a website not from the whitelist and then loading a website from whitelist to compare the context data:

website_not_in_whitelist.mov

Pre-merge author checklist

I've followed MetaMask Contributor Docs and MetaMask Extension Coding Standards.
I've completed the PR template to the best of my ability
I’ve included tests if applicable
I’ve documented my code using JSDoc format if applicable
I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.

Pre-merge reviewer checklist

I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

github-actions · 2024-08-27T20:39:13Z

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

metamaskbot · 2024-08-28T17:41:18Z

Builds ready [09186f5]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5
- common: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1560 ± 42 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	1387	1696	1566	85	41
		domContentLoaded	1378	1688	1545	88	42
		load	1386	1696	1560	88	42
		domInteractive	21	53	27	9	4

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 2.57 KiB (0.07%)
ui: 0 Bytes (0.00%)
common: 29 Bytes (0.00%)

codecov · 2024-08-28T17:41:39Z

Codecov Report

Attention: Patch coverage is 12.69841% with 110 lines in your changes missing coverage. Please review.

Project coverage is 69.90%. Comparing base (b01f0c9) to head (0677ee6).

Files with missing lines	Patch %	Lines
app/scripts/streams/cookie-handler-stream.ts	0.00%	58 Missing ⚠️
app/scripts/contentscript.js	0.00%	28 Missing ⚠️
app/scripts/metamask-controller.js	0.00%	12 Missing ⚠️
app/scripts/background.js	0.00%	6 Missing ⚠️
app/scripts/constants/marketing-site-whitelist.ts	0.00%	4 Missing ⚠️
app/scripts/streams/shared.ts	0.00%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #26697      +/-   ##
===========================================
- Coverage    70.04%   69.90%   -0.13%     
===========================================
  Files         1435     1439       +4     
  Lines        49920    50044     +124     
  Branches     13980    13995      +15     
===========================================
+ Hits         34963    34983      +20     
- Misses       14957    15061     +104

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

metamaskbot · 2024-09-03T15:36:41Z

Builds ready [df8983a]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5
- common: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1762 ± 102 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	255	2129	1587	453	218
		domContentLoaded	1535	2355	1741	196	94
		load	1540	2394	1762	212	102
		domInteractive	13	103	37	22	11

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 2.95 KiB (0.09%)
ui: 0 Bytes (0.00%)
common: 29 Bytes (0.00%)

metamaskbot · 2024-09-03T20:15:17Z

Builds ready [32f8155]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5
- common: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (2928 ± 291 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	281	3554	1986	1263	606
		domContentLoaded	1716	4236	2899	602	289
		load	1729	4263	2928	607	291
		domInteractive	14	281	78	56	27

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 2.95 KiB (0.09%)
ui: 0 Bytes (0.00%)
common: 29 Bytes (0.00%)

DDDDDanica · 2024-09-10T09:28:10Z

Hey @NiranjanaBinoy I must be missing the implementation of this - we need to modify the body of event sent to segment and add context to it:

Additional technical details are that the method buildContext is where I believe we would add in our ga_client_id key / value pair. This would then pass this along with all track and identify calls.

Can you show me where is it? thank you !

app/scripts/metamask-controller.js

app/scripts/contentscript.js

DDDDDanica

@NiranjanaBinoy Thanks for putting everything together !

Can you add the description in this ticket for manual tests as

test the track event sending after this has this id

It is also valuable to add a screen recording of if works for

marketing whitelist site
site doesn't have this id passing

Thanks a lot !

test/e2e/tests/metrics/attribution/marketing-cookieid.spec.ts

test/e2e/tests/metrics/attribution/mock-page/index.html

test/e2e/tests/metrics/attribution/marketing-cookieid.spec.ts

test/e2e/tests/metrics/attribution/mock-page/index.html

test/e2e/tests/metrics/attribution/marketing-cookieid.spec.ts

metamaskbot · 2024-09-12T15:19:53Z

Builds ready [978f71b]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5
- common: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1943 ± 107 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	473	2430	1858	383	184
		domContentLoaded	1660	2409	1923	213	102
		load	1669	2432	1943	222	107
		domInteractive	12	76	37	15	7

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 2.88 KiB (0.08%)
ui: 0 Bytes (0.00%)
common: 29 Bytes (0.00%)

danjm

Generally looks good. I have left some questions and suggestions

app/scripts/controllers/metametrics.js

danjm · 2024-09-12T15:49:32Z

app/scripts/metamask-controller.js

@@ -5114,6 +5119,42 @@ export default class MetamaskController extends EventEmitter {
    );
  }

+  setUpCookieHandlerCommunication({ connectionStream }) {


We should create a github issue for adding unit tests for this method and for setupPhishingCommunication

done. #27119

app/scripts/contentscript.js

danjm · 2024-09-12T16:14:04Z

app/scripts/streams/cookie-handler-stream.ts

@@ -0,0 +1,193 @@
+import browser from 'webextension-polyfill';


This file looks good

test/e2e/tests/metrics/marketing-cookieid.spec.ts

app/scripts/background.js

…o hold cookie id

…d fixing e2e

sonarcloud · 2024-09-13T02:35:51Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

metamaskbot · 2024-09-13T03:06:54Z

Builds ready [0677ee6]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5
- common: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1686 ± 66 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	220	2011	1478	533	256
		domContentLoaded	1494	1927	1662	118	57
		load	1502	2022	1686	138	66
		domInteractive	16	80	36	16	8

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 2.88 KiB (0.08%)
ui: 0 Bytes (0.00%)
common: 29 Bytes (0.00%)

NiranjanaBinoy · 2024-09-13T18:45:44Z

Hey @NiranjanaBinoy I must be missing the implementation of this - we need to modify the body of event sent to segment and add context to it:

Additional technical details are that the method buildContext is where I believe we would add in our ga_client_id key / value pair. This would then pass this along with all track and identify calls.

Can you show me where is it? thank you !

We are saving the cookieId to the state of the MetaMetrics controller as marketingCampaignCookieId and in _buildContext we are using the value in the variable to populate marketingCampaignCookieId property of the context here

DDDDDanica · 2024-09-13T19:34:07Z

LGTM 🔥

metamaskbot · 2024-09-18T15:09:37Z

Missing release label release-12.3.0 on PR. Adding release label release-12.3.0 on PR and removing other release labels(release-12.6.0), as PR was cherry-picked in branch 12.3.0.

NiranjanaBinoy requested a review from a team as a code owner August 27, 2024 20:39

NiranjanaBinoy self-assigned this Aug 27, 2024

NiranjanaBinoy added the team-extension-platform label Aug 27, 2024

NiranjanaBinoy changed the title ~~Adding "cookie id" to metrics event~~ feat: Adding "cookie id" to metrics event Aug 27, 2024

NiranjanaBinoy force-pushed the feature/2916-cookie branch from 0cde2bb to 09186f5 Compare August 28, 2024 17:09

legobeat requested a review from a team August 28, 2024 21:13

legobeat added the area-privacy label Aug 28, 2024

NiranjanaBinoy force-pushed the feature/2916-cookie branch 3 times, most recently from ee1b204 to df8983a Compare September 3, 2024 15:04

vthomas13 previously approved these changes Sep 9, 2024

View reviewed changes

vthomas13 self-requested a review September 9, 2024 20:07