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: re-add frontend amplitude and simplify backend #176

Merged
merged 7 commits into from
Sep 19, 2023
Merged

Conversation

cabreraalex
Copy link
Member

Description

Our backend calls aren't consistent or ID-ed enough to do real tracking. I also am not too concerned about GDPR with amplitude at our scale, so think the frontend tracking is okay. Eventually we should have some cookie banner.

@@ -344,24 +344,6 @@ def get_project_state(
project.editor = True
return select.project_state(project_uuid, project)

@api_app.post("/project/{owner}/{project}", response_model=Project, tags=["zeno"])
def get_project(owner_name: str, project_name: str, request: Request):
Copy link
Member Author

Choose a reason for hiding this comment

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

no longer called with the big project_stats call

type="submit"
variant="raised"
class="mt-5"
on:click={() => amplitude.track('User Registered')}
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we track this only if registration was successful?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm probably, but the amplitude events don't work well in the backend js. Maybe since we track login events amplitude can infer registrations and we can remove this.

import '../app.css';

export let data;
featureFlags.set({ ...zenoFeatureFlags, ...data.features });

if (browser)
amplitude.init('9e6755badfe6f970b509d3325a6a6678', {
Copy link
Member

Choose a reason for hiding this comment

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

Should this be in here? I'd guess we might want different ones depending on dev/prod and also custom installations. Further, this should probably not be exposed, should it?

Copy link
Member Author

Choose a reason for hiding this comment

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

re: dev vs. prod yeah maybe we want to read from ENV but changing ENV variables is a pain in prod aha.

It's okay to be exposed, it's usually just in the raw HTML.

Copy link
Member Author

Choose a reason for hiding this comment

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

made it an env variable

backend/zeno_backend/server.py Outdated Show resolved Hide resolved
@cabreraalex cabreraalex merged commit 9dbb07d into main Sep 19, 2023
6 checks passed
@cabreraalex cabreraalex deleted the ampliredo branch September 19, 2023 14:10
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