-
-
Notifications
You must be signed in to change notification settings - Fork 610
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: remove segment.io support, fixes #5365 #5997
Conversation
Download the artifacts for this pull request:
See Testing a PR. You can also test this PR on Gitpod. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested this manually and I can see my user events in Amplitude with this PR.
To remove: SegmentKey
in Makefile
and in .github/workflows/master-build.yml
.
I noticed comments for InitAmplitude
, should it be done here as well?:
Lines 14 to 16 in 55053f0
// Initialization is currently done before via init() func somewhere while | |
// creating the ddevapp. This should be cleaned up. | |
amplitude.InitAmplitude() |
eb7143f
to
4d43aa4
Compare
Thanks for the good eye. turned out that the check on On
That was Simon's hope to remove init() usage in the future, which is definitely a valid hope. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noticed one more leftover for SegmentKey
in .github/workflows/master-build.yml
I guess I got distracted fixing that and adding the warning to Turned out that hadn't been working for ages; we changed a long time ago to only check on |
5c8465a
to
b058c5e
Compare
Rebased this and retested, and it seems to be right. I checked |
The Issue
We stopped actually using Segment.io in v1.22.0 because it's too expensive and we switched to using Amplitude
Now for v1.23, we can just remove that.
Manual Testing Instructions
Manual testing is super-critical here. We need to make absolutely sure that we haven't broken Amplitude
Automated Testing Overview
Related Issue Link(s)
Release/Deployment Notes