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

Replace account id on rudderstack with hash id #3089

Merged
merged 4 commits into from
Aug 1, 2023

Conversation

hcho112
Copy link
Contributor

@hcho112 hcho112 commented Jul 31, 2023

This PR contains implementation to replace existing account id details on RudderStack metrics with hash id.

The team decided to not track account Id from RudderStack on Transfer Wizard. So similar to how it is implemented on near-discovery we hash the accountId and use it instead.

This change need to be published to prod as soon as it passes the review.

@hcho112 hcho112 requested a review from andy-haynes July 31, 2023 22:49
@render
Copy link

render bot commented Jul 31, 2023

@netlify
Copy link

netlify bot commented Jul 31, 2023

Deploy Preview for unrivaled-zabaione-2fe19c ready!

Name Link
🔨 Latest commit 8d0f602
🔍 Latest deploy log https://app.netlify.com/sites/unrivaled-zabaione-2fe19c/deploys/64c986ffca92ab000853bb63
😎 Deploy Preview https://deploy-preview-3089--unrivaled-zabaione-2fe19c.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@netlify
Copy link

netlify bot commented Jul 31, 2023

Deploy Preview for glittering-pavlova-0e9247 ready!

Name Link
🔨 Latest commit 8d0f602
🔍 Latest deploy log https://app.netlify.com/sites/glittering-pavlova-0e9247/deploys/64c986ff2ef8910008ce1323
😎 Deploy Preview https://deploy-preview-3089--glittering-pavlova-0e9247.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

@andy-haynes andy-haynes left a comment

Choose a reason for hiding this comment

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

Looking good overall, but I remain strongly in favor of computing the hash every time. We don't get any noticeable benefit from caching, we only introduce the possibility of reporting the wrong data.

@@ -88,3 +93,16 @@ export const resetUserState = () => {
}
return rudderanalytics.reset();
};

export function accountIdToHash(accountId) {
if (!accountIdHash) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Introducing state here now has introduced the potential for an event to be reported under the wrong accountId. Can we please compute the hash every time so as not to add any further complexity to event reporting?

@andy-haynes andy-haynes merged commit 6235ab3 into master Aug 1, 2023
13 checks passed
@andy-haynes andy-haynes deleted the reploace-rudderstack-account-id branch August 1, 2023 22:41
@andy-haynes andy-haynes mentioned this pull request Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants