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(metrics): Segment dependency cleanup #9092

Merged
merged 7 commits into from
Nov 6, 2023
Merged

Conversation

tianrunhe
Copy link
Contributor

@tianrunhe tianrunhe commented Nov 1, 2023

Description

Last piece to completely remove Segment dependencies:

  1. Renamings:
    • client/hooks/useSegmentTrack.tsclient/hooks/useClientSideTrack.ts
    • server/graphql/mutations/helpers/sendAccountRemovedToSegment.tsserver/graphql/mutations/helpers/sendAccountRemovedEvent.ts
    • server/graphql/mutations/helpers/sendPokerMeetingRevoteToSegment.tsserver/graphql/mutations/helpers/sendPokerMeetingRevoteEvent.ts
  2. client/utils/safeIdentify.ts: Replace Segment's identify to Amplitude's identify
  3. server/graphql/webhookGraphQLHandler.ts: I'm not sure why we need the SEGMENT_FN_KEY here and why we need double encryptions?

Final checklist

  • I checked the code review guidelines
  • I have added Metrics Representative as reviewer(s) if my PR invovles metrics/data/analytics related changes
  • I have performed a self-review of my code, the same way I'd do it for any other team member
  • I have tested all cases I listed in the testing scenarios and I haven't found any issues or regressions
  • Whenever I took a non-obvious choice I added a comment explaining why I did it this way
  • I added the label One Review Required if the PR introduces only minor changes, does not contain any architectural changes or does not introduce any new patterns and I think one review is sufficient'
  • PR title is human readable and could be used in changelog

@github-actions github-actions bot added the size/m label Nov 1, 2023
Comment on lines 36 to 39
const hmacDigest = crypto
.createHmac('sha256', SERVER_SECRET!)
.update(timestampStr)
.digest('base64')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't quite understand why we need to use the SEGMENT_FN_KEY here, and also why need to encrypt once with it and then another time with the SERVER_SECRET. I changed it to just encrypt once.
Let me know if it's gonna break!

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, that's an interesting one! Required some digging, but now I know what was going on: Segment used to request additional data for an event from the app, see this PR. For this we sent the signed timestamp with each event.
We're not doing this anymore, this file and its references should be removed.

@tianrunhe tianrunhe requested a review from Dschoordsch November 1, 2023 16:55
Copy link
Contributor

@Dschoordsch Dschoordsch left a comment

Choose a reason for hiding this comment

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

Did not test yet. You've uncovered some long forgotten code there which should be deleted before merging

Comment on lines 36 to 39
const hmacDigest = crypto
.createHmac('sha256', SERVER_SECRET!)
.update(timestampStr)
.digest('base64')
Copy link
Contributor

Choose a reason for hiding this comment

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

Well, that's an interesting one! Required some digging, but now I know what was going on: Segment used to request additional data for an event from the app, see this PR. For this we sent the signed timestamp with each event.
We're not doing this anymore, this file and its references should be removed.

@tianrunhe tianrunhe requested a review from Dschoordsch November 2, 2023 20:46
@tianrunhe tianrunhe merged commit c23494f into master Nov 6, 2023
@tianrunhe tianrunhe deleted the chore/segmentCleanup branch November 6, 2023 16:59
Copy link

sentry-io bot commented Nov 8, 2023

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

  • ‼️ TypeError: null is not an object (evaluating 'window.localStorage.getItem') <anonymous>(packages/client/components/Analytic... View Issue

Did you find this useful? React with a 👍 or 👎

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants