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

Async event action mapping to 5 min, add warning #3717

Merged
merged 8 commits into from Mar 22, 2021
Merged

Conversation

mariusandra
Copy link
Collaborator

Changes

  • Users were maxing out their postgres CPUs
  • Sets async event action mapping to 5min
  • Adds an env ACTION_EVENT_MAPPING_INTERVAL_SECONDS to further specify the duration if needed
  • Adds a warning describing the situation. Dismissing the warning dismisses it forever.
    image

Checklist

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

message={'Webhooks and actions delayed up to 5 minutes'}
className="demo-warning"
description={
<>Due to temporary limitations, webhooks and actions might be delayed by up to 5 minutes.</>
Copy link
Collaborator

@timgl timgl Mar 22, 2021

Choose a reason for hiding this comment

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

I don't think this will be temporary? Just Webhooks and actions might be delayed by up to 5 minutes. should to the trick

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Until we move action event mapping to the plugin server, as discussed here: PostHog/plugin-server#235

Copy link
Collaborator

Choose a reason for hiding this comment

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

But will action mapping in the plugin server by itself make things better? 🤔 Postgres is the bottleneck in this case and action mapping on the fly will (did?) in fact hammer it even more, because every event is a whole new request. I think what may need optimization is action mapping queries/data layout itself. Otherwise action matching without significant delays seems to be hard to pull off. Feel free to correct me

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Action event mapping in the plugin server wouldn't make extra queries, that's the thing :).

Except if a) needing to fetch cohorts to see if a person belongs to them due to a filter, or b) fetching a person to match person properties in a filter. The person is more often than not already fetched, and we could inline adding the cohorts with a subquery if needed.

@@ -117,6 +118,7 @@ export function App(): JSX.Element | null {
{!sceneConfig.hideTopNav && <TopNavigation />}
{scene ? (
<Layout.Content className="main-app-content" data-attr="layout-content">
{user.is_async_event_action_mapping_enabled && <AsyncActionWarning />}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wasn't suggesting always showing this, just when you are creating a webhook (and maybe when you are looking at an action).

@Twixes Twixes temporarily deployed to posthog-async-warning-3-pa0w58 March 22, 2021 13:46 Inactive
@mariusandra mariusandra temporarily deployed to posthog-async-warning-3-pa0w58 March 22, 2021 14:02 Inactive
@mariusandra
Copy link
Collaborator Author

I removed the top banner and just added these lines:

image

image

@mariusandra mariusandra requested a review from timgl March 22, 2021 14:04
@timgl
Copy link
Collaborator

timgl commented Mar 22, 2021

yeah looks good

@mariusandra
Copy link
Collaborator Author

WTF is this now... 🤔

image

Considering I didn't touch that file and want to cherry-pick this into a patch, I won't touch that line and will just merge once everything else is green.

@Twixes
Copy link
Collaborator

Twixes commented Mar 22, 2021

It was broken in #3666.

Comment on lines +119 to +122
<p>
Please note that webhooks and actions may be delayed up to 5 minutes due to open-source PostHog
configuration.
</p>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I committed this notice. I'd really like to be clear for users so that they get why there's a delay, WDYT here @mariusandra?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good enough for me

@mariusandra mariusandra temporarily deployed to posthog-async-warning-3-pa0w58 March 22, 2021 14:28 Inactive
@mariusandra mariusandra merged commit 6dcb18f into master Mar 22, 2021
@mariusandra mariusandra deleted the async-warning-300 branch March 22, 2021 14:38
mariusandra added a commit that referenced this pull request Mar 22, 2021
* Set ACTION_EVENT_MAPPING_INTERVAL_SECONDS to 300, move to settings

* Add async warning

* Use smarter get_from_env

* Async event action mapping warning when creating a webhook

* Async event action mapping warning when editing an action

* remove top async warning

* Use common reworded notice component for async mapping

Co-authored-by: Michael Matloka <dev@twixes.com>
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.

None yet

3 participants