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

[IMPROVEMENT] Refactor Analytics Events #5454

Merged
merged 12 commits into from
Jan 5, 2023
Merged

Conversation

gantunesr
Copy link
Member

@gantunesr gantunesr commented Jan 4, 2023

Description

Write a short description of the changes included in this pull request, also include relevant motivation and context. Have in mind the following questions,
1. What is the reason for the change?

As part of the Segment integration, it's required to do a series of refactors on the app analytics system. The PR #5139 is the main migration development, but getting the events refactor merged will help to reduce the number of code conflicts.

2. What is the improvement/solution?

Refactor the analytics events and centralize them into a single file app/core/Analytics/MetaMetrics.events.ts

Screenshots/Recordings

If applicable, add screenshots and/or recordings to visualize the before and after of your change

Issue

Progresses https://github.com/MetaMask/mobile-planning/issues/372

Related to PR #5139

Checklist

  • There is a related GitHub issue
  • Tests are included if applicable
  • Any added code is fully documented

@gantunesr gantunesr added the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Jan 4, 2023
@gantunesr gantunesr marked this pull request as ready for review January 4, 2023 13:55
@gantunesr gantunesr requested a review from a team as a code owner January 4, 2023 13:55
Copy link
Contributor

@owencraston owencraston left a comment

Choose a reason for hiding this comment

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

I didn't run the app but all the events seem to match.
Unrelated but I created this util for generateDeviceAnalyticsMetaData. Perhaps this is something we want to move in future changes or even make it a part of all the events? Just thought i'd mention it here to create the paper trail. Nice work 👍

app/core/Analytics/MetaMetrics.events.ts Outdated Show resolved Hide resolved
app/core/Analytics/MetaMetrics.events.ts Outdated Show resolved Hide resolved
app/util/analytics.js Show resolved Hide resolved
@gantunesr gantunesr added No QA Needed/E2E Only Apply this label when your PR does not need any QA effort. release-5.14.0 and removed needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) labels Jan 5, 2023
@gantunesr
Copy link
Member Author

Nice @owencraston, I'll keep it in mind for the next analytics PR.

@gantunesr gantunesr merged commit db9007b into main Jan 5, 2023
@gantunesr gantunesr deleted the refactor/analytics-events branch January 5, 2023 21:57
@github-actions github-actions bot locked and limited conversation to collaborators Jan 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
No QA Needed/E2E Only Apply this label when your PR does not need any QA effort. release-5.14.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants