Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: update metrics logic #10545

Merged
merged 21 commits into from
Aug 26, 2024
Merged

fix: update metrics logic #10545

merged 21 commits into from
Aug 26, 2024

Conversation

NicolasMassart
Copy link
Contributor

@NicolasMassart NicolasMassart commented Aug 2, 2024

Description

This PR fixes the logic for the Metametrics anonymous events handling.

What's in this PR:

  • add new tests and update existing ones to enforce the fixed logic
  • fix the test file (typing)
  • reorganise and group some tests
  • clarify variables and values names
  • remove trackAnonymousEvent and migrate all calls to trackEvent with new EventProperties data structure
  • add util functions to preserve backward compatibility on older events structure
  • reduce dependency on Segment's JsonMap

Note

in a follow-up work we will work on refactoring of the events, completely get rid of legacy and only use EventProperties data structure, make events similar to extension.
But this PR focuses on the anonymous event tracking logic. There's already some changes making it closer to how extension works, but only on the anonymous tracking part.

Related issues

Fixes https://github.com/MetaMask/mobile-planning/issues/1886
Fixes #9996

Manual testing steps

Run the app (simulator is fine)

and do an action that tracks anonymously, for instance request swap quotes.

It should show in the logs:

The regular non-anonymous event (name but no props)

 INFO  TRACK event saved {"event": "Swaps Opened", "properties": {}, "type": "track"}

The anonymous event (name and props)

 INFO  TRACK event saved {"event": "Swaps Opened", "properties": {"activeCurrency": undefined, "chain_id": "1", "source": "MainView"}, "type": "track"}

Other pairs:
Quotes Requested:

 INFO  TRACK event saved {"event": "Quotes Requested", "properties": {}, "type": "track"}
 INFO  TRACK event saved {"event": "Quotes Requested", "properties": {"chain_id": "1", "custom_slippage": false, "request_type": "Quote", "token_from": "ETH", "token_from_amount": "12", "token_to": "USDC"}, "type": "track"}

Quotes Received:

 INFO  TRACK event saved {"event": "Quotes Received", "properties": {}, "type": "track"}
 INFO  TRACK event saved {"event": "Quotes Received", "properties": {"available_quotes": 6, "best_quote_source": "zeroEx", "chain_id": "1", "custom_slippage": false, "network_fees_ETH": "0.00275", "network_fees_USD": "$7.27", "request_type": "Quote", "response_time": 1194, "slippage": 2, "token_from": "ETH", "token_from_amount": "12", "token_to": "USDC", "token_to_amount": "31486.164525"}, "type": "track"}

Only two events should be sent, one non-anonymous (with distinct user id), one anonymous (with anonymous 0x000...000 id).

However, to check the IDs are correctly set, you have to go on Segment debug dashboard:
Non-anonymous event (see user id is random UUID):
image
Same event but as anonymous event (see user id is 0x0000000000000000):
image

For cases where you have only one non-anonymous tracking call, no anonymous event should be sent.

For instance on errors:

INFO  TRACK event saved {"event": "Error occurred", "properties": {"error": true, "errorMessage": "Branch:", "type": "The network request was invalid."}, "type": "track"}
image

or on browser view:

 INFO  TRACK event saved {"event": "Browser Opened", "properties": {"active_connected_dapp": [], "chain_id": "1", "number_of_accounts": 1, "number_of_open_tabs": 0, "source": "Navigation Tab"}, "type": "track"}
 INFO  TRACK event saved {"event": "Switched tab within Browser", "properties": {}, "type": "track"}
image image

matrix of tracking use cases based on extension behaviour.

it's also retro compatible with our individual anonymous events.
How to read it? Here are some examples:

  • Test A means non-anonymous tracking (NA) and it has no props at all:
    The result must be only one non-anonymous event without any props (EMPTY) and no anonymous event at all (NONE).
  • Test D means anonymous tracking (A) and it has both non-anonymous and anonymous props:
    The result must be a non-anonymous event with non-anonymous props (NA PROPS) and an anonymous event with all props (NA PROPS + A PROPS).
Test Non-anon prop Anon prop Result non-anon (NA) event Result anon (A) event
A NO NO EMPTY NONE
B YES NO NA PROPS NONE
C0 NO YES(indiv) EMPTY A PROPS
C1 NO YES(group) EMPTY A PROPS
C2 NO YES(mixed) EMPTY A PROPS
D YES YES NA PROPS NA PROPS + A PROPS

For C0/C1/C2:

  • individual prop is one that is mixed with others but is of the form prop = { anonymous: true, value: 'anon value' }
  • group anonymous props are of the form prop = 'anon value' but are grouped in an object implementing the SensitiveProperties interface.
  • mixed means both types in the same event

Screenshots/Recordings

Before

No change on UI

After

No change on UI

Pre-merge author checklist

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.

@NicolasMassart NicolasMassart self-assigned this Aug 2, 2024
Copy link
Contributor

github-actions bot commented Aug 2, 2024

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.

Test do not pass yet on purpose
We have to validate the test use case logic
and then update implementation
Add new type
Keep legacy compatibility
@NicolasMassart NicolasMassart added the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Aug 22, 2024
@NicolasMassart NicolasMassart marked this pull request as ready for review August 23, 2024 15:13
@NicolasMassart NicolasMassart requested review from a team as code owners August 23, 2024 15:13
@NicolasMassart NicolasMassart added the Run Smoke E2E Triggers smoke e2e on Bitrise label Aug 23, 2024
Copy link
Contributor

github-actions bot commented Aug 23, 2024

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: 148d7c0
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/8018132a-baec-4493-9bd4-3890fec4df11

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

@MetaMask MetaMask deleted a comment from codecov-commenter Aug 23, 2024
@NicolasMassart NicolasMassart removed the Run Smoke E2E Triggers smoke e2e on Bitrise label Aug 23, 2024
thanks copilot!
@NicolasMassart NicolasMassart added the Run Smoke E2E Triggers smoke e2e on Bitrise label Aug 23, 2024
Copy link
Contributor

github-actions bot commented Aug 23, 2024

https://bitrise.io/ Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: 7f1cce6
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/ca39a94b-4e13-47d2-bb4f-24a86f10a7b9

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

frankvonhoven
frankvonhoven previously approved these changes Aug 23, 2024
Copy link
Contributor

@frankvonhoven frankvonhoven left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Nice work

add comment
format
Copy link

sonarcloud bot commented Aug 26, 2024

Copy link
Contributor

@tommasini tommasini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work! LGTM!

@NicolasMassart NicolasMassart added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Aug 26, 2024
Copy link
Contributor

github-actions bot commented Aug 26, 2024

https://bitrise.io/ Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: 64e5a39
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/483b8e17-f964-4960-a92f-dcab3fef9746

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

@wachunei
Copy link
Member

wachunei commented Aug 26, 2024

I wish we would have kept the trackAnonymousEvent(event, params) method and internally do the mapping to a nested object with sensitiveProperties key, this is adding overhead and needs to be known by the developer whom might not have this context. trackEvent + trackAnonymousEvent is much simpler to remember than trackEvent + params.sensitiveProperties

I'm proposing we just do a wrapper in MetaMetrics.ts, simplified code as example:

trackAnonymousEvent(event, params) {
  return this.trackEvent(event, { sensitiveProperties: ...params });
}

Open to hear what you think @NicolasMassart

edit: I also think obscuring sensitiveProperties is best since developer can just now add exampleProperty: { anonymous:true, value: "sensitive value" } and don't worry about sensitiveProperties. ie. sensitiveProperties must be an internal key name always.

Copy link
Contributor

@frankvonhoven frankvonhoven left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice :)

Copy link
Member

@wachunei wachunei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. In interest of fixing this bug my suggestion will be discussed later in time.

@NicolasMassart NicolasMassart mentioned this pull request Aug 26, 2024
20 tasks
@NicolasMassart
Copy link
Contributor Author

@wachunei I linked your important comment in the followup issue #10784

@NicolasMassart NicolasMassart merged commit 097f473 into main Aug 26, 2024
39 checks passed
@NicolasMassart NicolasMassart deleted the 1886_metametrics_logic branch August 26, 2024 17:57
@github-actions github-actions bot locked and limited conversation to collaborators Aug 26, 2024
@github-actions github-actions bot removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Aug 26, 2024
@metamaskbot metamaskbot added the release-7.31.0 Issue or pull request that will be included in release 7.31.0 label Aug 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-7.31.0 Issue or pull request that will be included in release 7.31.0 Run Smoke E2E Triggers smoke e2e on Bitrise team-mobile-platform
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

metametrics tracks too many events
5 participants