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

chore: Replace Segment SDK Anonymous ID patch by plugin #11219

Closed
1 of 9 tasks
Tracked by #10784
NicolasMassart opened this issue Sep 16, 2024 · 0 comments · Fixed by #11350
Closed
1 of 9 tasks
Tracked by #10784

chore: Replace Segment SDK Anonymous ID patch by plugin #11219

NicolasMassart opened this issue Sep 16, 2024 · 0 comments · Fixed by #11350
Assignees
Labels
release-7.33.0 Issue or pull request that will be included in release 7.33.0 team-mobile-platform

Comments

@NicolasMassart
Copy link
Contributor

NicolasMassart commented Sep 16, 2024

What is this about?

The user ID sent in events depends on the event type: anonymous or not.
Current implementation patches Segment SDK to allow this behaviour.
A Segment SDK plugin can be used instead.

Scenario

No response

Design

No response

Technical Details

Replace the patch with a Segment SDK plugin
Create a custom plugin.

It makes it easier to maintain and allows to update the SDK with less risk.

Steps

  1. Create a new file for the plugin: Create a new file, for example, MetaMaskPrivacyPlugin.ts.

  2. Implement the plugin: Implement the plugin by extending the Segment SDK's Plugin class and overriding the necessary methods.

  3. Register the plugin: Register the plugin with the Segment client.

Example implementation

Important

The following is untested code just to explain the idea.
It requires changes and testing (and unit testing)

Plugin file

MetaMetricsPrivacySegmentPlugin.ts

import { Plugin, SegmentEvent, EventType } from '@segment/analytics-react-native';

class MetaMetricsPrivacySegmentPlugin extends Plugin {
  configure(analytics) {
    this.analytics = analytics;
  }

  async execute(event: SegmentEvent): Promise<SegmentEvent> {
    // We only update id in track events, otherwise return event as is.
    if( event.type !== EventType.TrackEvent){
      return event;
    }

    const isAnonymousEvent = isTrackEvent && event.properties?.anonymous;

    delete event.properties?.anonymous;

    const hasProperties = event.properties && Object.entries(event.properties).length > 0;

    const userInfo = await this.analytics.userInfo.get(true);
    const userId = isAnonymousEvent && hasProperties ? userInfo.anonymousId : userInfo.userId ?? userInfo.anonymousId;

    return {
      ...event,
      userId,
    };
  }
}

export default MetaMaskPrivacyPlugin;

Register the plugin in app/core/Analytics/MetaMetrics.ts

const segmentClient = isE2E ? undefined : createClient(config);
segmentClient.add(new MetaMetricsPrivacySegmentPlugin());

Threat Modeling Framework

No response

Acceptance Criteria

No response

Stakeholder review needed before the work gets merged

  • Engineering (needed in most cases)
  • Design
  • Product
  • QA (automation tests are required to pass before merging PRs but not all changes are covered by automation tests - please review if QA is needed beyond automation tests)
  • Security
  • Legal
  • Marketing
  • Management (please specify)
  • Other (please specify)

References

https://segment.com/docs/connections/sources/catalog/libraries/mobile/react-native/#writing-your-own-plugins

@NicolasMassart NicolasMassart self-assigned this Sep 16, 2024
@NicolasMassart NicolasMassart changed the title Replace Segment SDK Anonymous ID patch by plugin chore: Replace Segment SDK Anonymous ID patch by plugin Sep 20, 2024
github-merge-queue bot pushed a commit that referenced this issue Oct 2, 2024
<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

## **Description**

Replace the Segment SDK patch that was updating events for anonymous
user id by a plugin that does the same

>[!IMPORTANT]
> This PR doesn't change the behaviour of the metrics system.
> It only allows to get rid of the patch.

<!--
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?
2. What is the improvement/solution?
-->

## **Related issues**

Fixes #11219
May also fix #11065

## **Manual testing steps**

1. navigate the app
2. check events landing on Segment, then Mixpanel have the proper user
ID
  - if anonymous: the 0x0000...0000 id
  - if not anonymous the random UUIDv4

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

Same as after, no functional change

### **After**

The non anonymous part of the event (no properties, but userId)

![image](https://github.com/user-attachments/assets/95f7cd98-6a5d-4ca8-9dc3-7a5aeb6a52eb)

The anonymous part of the event (properties, but userId replaced by
anonymous id)

![image](https://github.com/user-attachments/assets/063408d1-e1dc-4f60-8063-fbfc0d57cabd)


## **Pre-merge author checklist**

- [x] I’ve followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile
Coding
Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've completed the PR template to the best of my ability
- [x] I’ve included tests if applicable
- [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
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.
@gauthierpetetin gauthierpetetin added the release-7.33.0 Issue or pull request that will be included in release 7.33.0 label Oct 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-7.33.0 Issue or pull request that will be included in release 7.33.0 team-mobile-platform
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants