Skip to content
This repository was archived by the owner on Oct 22, 2024. It is now read-only.

feat: integrating analytics and capturing event paired#450

Merged
vinistevam merged 7 commits intomainfrom
415-integrate-analytics-capture-event-paired
Jan 27, 2023
Merged

feat: integrating analytics and capturing event paired#450
vinistevam merged 7 commits intomainfrom
415-integrate-analytics-capture-event-paired

Conversation

@vinistevam
Copy link
Copy Markdown
Contributor

@vinistevam vinistevam commented Jan 19, 2023

Overview

The aim of this PR is to integrate Segment into the desktop app and send/capture events whenever the desktop app is paired with the extension.

Changes

  • added new dependency: analytics-node and types.
  • analytics.ts provides an instance to metrics-service.ts and allows the usage of Segment SDK
  • metrics-service.ts uses the electron store to save events sent and buffer events in case users didn't opt in as a participant to send metrics.
  • desktop-app by MetricsService.track an event is emitted to Segment every time the desktop app receives a paired event from the extension.

Others

In order to test Sentry locally:

Create a Sentry account using your consensys email address.
Add a new electron/node project.
Access project Settings -> API Keys
Copy the write key into environment variables in .metamaskrc called SEGMENT_WRITE_KEY.

image

Issues

resolves: #415

@vinistevam vinistevam force-pushed the 415-integrate-analytics-capture-event-paired branch 3 times, most recently from 407182a to b28b0be Compare January 20, 2023 08:25
@vinistevam vinistevam marked this pull request as ready for review January 20, 2023 08:26
@vinistevam vinistevam requested a review from a team January 20, 2023 08:26
@vinistevam vinistevam force-pushed the 415-integrate-analytics-capture-event-paired branch 3 times, most recently from 84da497 to bc5eb7d Compare January 24, 2023 18:01
Copy link
Copy Markdown
Member

@OGPoyraz OGPoyraz Jan 25, 2023

Choose a reason for hiding this comment

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

Even if we send an event from UI to the main process to set this property, we still need to read it once the desktop-app is restarted.

Rather than doing that, we could use a sync operation called readPersistedSettingFromAppState and read metametricsOptIn value. This will return the value stored in the UI (redux store)

By the way, readPersistedSettingFromAppState is not working as expected today, it returns the stringified value of the property, it's an easy fix but just for the heads up.
Please see this thread: https://consensys.slack.com/archives/C03RV0Q4TGB/p1674563207800129
And the task: https://github.com/orgs/MetaMask/projects/37?pane=issue&itemId=18772949

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe the event approach could work for the 'initial events' before chosing metametrics opt-in/out.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I saw that Bernardo used it on the Sentry PR. Initially, I thought of using the context bridge to fire an event from the UI to update this property but as UI also need that and we have readPersistedSettingFromAppState I will change it on the ticket that I'm currently working on.

Just one detail that we can discuss further in the meeting today is to contemplate the case where a user has not optIn/optOut using the UI but we already fire an initial event that should be stored and flushed if the user optIn. In this case, what about metametricsOptIn accepting undefined and whenever is undefined it reflects that the user has not optIn/optOut using the UI yet?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's hard to understand why they choose this package name 😂

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

😆 this looks like a rebranding that is ongoing.

Copy link
Copy Markdown
Member

@OGPoyraz OGPoyraz left a comment

Choose a reason for hiding this comment

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

🚀🚀🚀

@vinistevam vinistevam force-pushed the 415-integrate-analytics-capture-event-paired branch from bc5eb7d to a01ebe8 Compare January 27, 2023 10:12
@vinistevam vinistevam merged commit 273a167 into main Jan 27, 2023
@vinistevam vinistevam deleted the 415-integrate-analytics-capture-event-paired branch February 1, 2023 08:03
@cryptotavares cryptotavares mentioned this pull request Mar 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[OKR1] Capture the event when a Desktop app is paired for the first time [Metric PoC]

3 participants