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

Fix FOSS webhook, update actions calculation #3610

Closed
wants to merge 2 commits into from

Conversation

yakkomajuri
Copy link
Contributor

Changes

Works in conjunction with PostHog/plugin-server#233

On PostHog FOSS:

  • Calculates actions for an event when that event is ingested rather than instance-wide periodically
  • Triggers the webhook after the calculation if the event should trigger a webhook

Discussion about the approach on Slack.

Checklist

  • All querysets/queries filter by Organization, by Team, and by User
  • Django backend tests
  • Jest frontend tests
  • Cypress end-to-end tests

@yakkomajuri yakkomajuri changed the title fix foss webhook Fix FOSS webhook, update actions calculation Mar 9, 2021
@yakkomajuri yakkomajuri requested a review from Twixes March 9, 2021 20:25
@timgl timgl temporarily deployed to posthog-fix-foss-webhoo-d2njv7 March 9, 2021 20:27 Inactive
@mariusandra
Copy link
Collaborator

The code seems fine for me, but at the end of the day, this comment (added here) still scares me:

image

(and this PR removes this comment)

Sure it's not "sync" anymore, but we'll be doing action matching in "unbatched-async", which is different from "batched-async". Without any performance tests, we could break heavy volume OSS users with so many potentially difficult queries running in parallel.

Or it could be perfectly fine, but I'm just not sure and afraid of again breaking some setups...

Here's an alternative way to do this entire action matching though: PostHog/plugin-server#235

@Twixes
Copy link
Member

Twixes commented Mar 11, 2021

Is this still applicable after recent merges?

@mariusandra
Copy link
Collaborator

I think not. And given what we discussed on slack, running a catch-all action.calculate_events(start=action.last_calculated_at) for every event is probably a recipe for deadlocks and funny business.

@paolodamico paolodamico deleted the fix-foss-webhook branch October 25, 2021 19:59
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.

4 participants