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: delete capture events in reverse proxy #26625

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

surbhi-posthog
Copy link
Contributor

@surbhi-posthog surbhi-posthog commented Dec 3, 2024

Problem

Adding a capture event with a reverse proxy is deleted.

Sorry for the small changes but I've been battling with testing these logs locally. Once I can verify them locally, I will push larger changes for the remaining team features.

Changes

Verified managed reverse proxy's can be created and deleted in the frontend

👉 Stay up-to-date with PostHog coding conventions for a smoother review.

Does this work well for both Cloud and self-hosted?

Cloud: yes

How did you test this code?

Locally by adding and deleting managed reverse proxies

Copy link
Contributor

@patricio-posthog patricio-posthog left a comment

Choose a reason for hiding this comment

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

It looks good to me. Just a minor comment: we are capturing intent here, right? Because the actual action/work runs async, so we don't know if it was succesful or not. Maybe you want to make that explicit in the event? Like managed reverse proxy intent created or something like that? (that sounds better in English haha)

@surbhi-posthog
Copy link
Contributor Author

@zlwaterfield @patricio-posthog, i think it would be better to call the capture in a try block to make sure we only capture success events. If they fail, we should capture failed events separately in my opinion. Let me know what you guys think?

@patricio-posthog
Copy link
Contributor

@zlwaterfield @patricio-posthog, i think it would be better to call the capture in a try block to make sure we only capture success events. If they fail, we should capture failed events separately in my opinion. Let me know what you guys think?

Since this is async you can't do that. You should move the capture code into the async task, since this try/except only checks if the task was dispatched but not the outcome.

@zlwaterfield
Copy link
Contributor

@patricio-posthog - I'm not sure what you mean, this is the record creation so it should be fine as is. We could have another one when the record is verified in the DNS.

@surbhi-posthog - We shouldn't need the try/catch, this is how events are captured throughout the codebase, it's using the PostHog SDK and should be fine.

@patricio-posthog
Copy link
Contributor

@zlwaterfield Yeah I thought we want to send the capture event once the whole process was succesful (creation + verification), but if it's enough only with the creation then it's fine.

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.

3 participants