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

feat: add backend proxy for segment #5474

Merged
merged 5 commits into from May 8, 2024
Merged

feat: add backend proxy for segment #5474

merged 5 commits into from May 8, 2024

Conversation

davidsoderberg
Copy link
Contributor

No description provided.

Copy link

netlify bot commented Apr 29, 2024

Deploy Preview for dev-web-novu ready!

Name Link
🔨 Latest commit 40c1248
🔍 Latest deploy log https://app.netlify.com/sites/dev-web-novu/deploys/6638723a59229f0008b6fbea
😎 Deploy Preview https://deploy-preview-5474--dev-web-novu.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

netlify bot commented Apr 29, 2024

Deploy Preview for novu-design ready!

Name Link
🔨 Latest commit 40c1248
🔍 Latest deploy log https://app.netlify.com/sites/novu-design/deploys/6638723afc74bc0008b9fc65
😎 Deploy Preview https://deploy-preview-5474--novu-design.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

@rifont rifont left a comment

Choose a reason for hiding this comment

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

It's not currently covered in this PR - do we need to address(/remove) the Segment Identify/Group call that happens too?

apps/api/src/app/analytics/analytics.controller.ts Outdated Show resolved Hide resolved
apps/api/src/app/analytics/analytics.controller.ts Outdated Show resolved Hide resolved
@davidsoderberg
Copy link
Contributor Author

It's not currently covered in this PR - do we need to address(/remove) the Segment Identify/Group call that happens too?

Group is not used by frontend and identify only sends user id so we should already know that as we can get the user id from the user session.

Copy link
Contributor

@rifont rifont left a comment

Choose a reason for hiding this comment

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

LGTM! Let's 🚢 it

@SokratisVidros
Copy link
Contributor

Should we feature flag this endpoint based on the presence of MIXPANEL_TOKEN and SEGMENT_TOKEN? We would like this endpoint to be opt-in in standalone deployments.

@davidsoderberg
Copy link
Contributor Author

Should we feature flag this endpoint based on the presence of MIXPANEL_TOKEN and SEGMENT_TOKEN? We would like this endpoint to be opt-in in standalone deployments.

Not sure I understand your comment 💯 but there is a check in the analytics service for the SEGMENT_TOKEN so the standalone deployments will not get an error if the endpoint will be called without it being present.

@scopsy
Copy link
Contributor

scopsy commented May 3, 2024

@davidsoderberg I think he meant don't make the http request on the first place if ff is not present to avoid the network activity.

@davidsoderberg
Copy link
Contributor Author

@davidsoderberg I think he meant don't make the http request on the first place if ff is not present to avoid the network activity.

So this should be it then :)

5e6e0cf

@SokratisVidros
Copy link
Contributor

Thanks, @davidsoderberg, for the clarification. My initial comment was about not registering the Analytics module at all unless both env vars are set. We might be able to abstract the env var check via a static isEnabled check on the module class.

@davidsoderberg davidsoderberg merged commit 8de8c20 into next May 8, 2024
28 checks passed
@davidsoderberg davidsoderberg deleted the add-segment-proxy branch May 8, 2024 06:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants