Fire webhooks from internal action matching results by Twixes · Pull Request #479 · PostHog/plugin-server

Twixes · 2021-06-16T10:26:32Z

Changes

Resolves #459. Webhooks fired directly here instead of in Python with Celery.

Checklist

~~Updated Settings section in README.md, if settings are affected~~
Jest tests

neilkakkar · 2021-06-18T10:03:43Z

+    let markdown = ''
+
+    if (tokenParts[0] === 'user') {
+        if (tokenParts[1] == 'name') {


Suggested change

if (tokenParts[1] == 'name') {

if (tokenParts[1] === 'name') {

and a few below, too! Python->JS conversion can get so annoying 😂

neilkakkar · 2021-06-18T10:10:45Z

+        } else {
+            const propertyName = `$${tokenParts[1]}`
+            const property = event.properties?.[propertyName]
+            text = markdown = stringify(property)


I'm not sure multiple assignment is equivalent in JS & Python 👀 : https://stackoverflow.com/questions/1758576/multiple-left-hand-assignment-with-javascript

But since text and markdown are already defined, this should be the same?

👍 Rearranged to avoid weird multiple assigment

neilkakkar · 2021-06-18T10:12:48Z

+    return [messageText, messageMarkdown]
+}
+
+export class HookCannon {


what is Cannon supposed to imply here? (a bit lost with the naming 😅 )

Is it because it finds and fires hooks?

Literally yes, it definitely may be too "fun" LOL. Feel free to propose something clearer

As someone who called a thread-killer the Terminator, yes, I'm very happy with a hook cannon 😂 .

Although hookCannon.findAndFireHooks feels like it's a HookCannonAndLogistician.

To make things clearer, perhaps a HookManager? It seems to fit in well with our other managers, and performs a similar role to the others (although most of them fetch, while this one fires)

Renamed to, drum roll, HookCommander

neilkakkar · 2021-06-18T10:34:17Z


        if (database === 'clickhouse') {
-            expect(queryCounter).toBe(11 + 14 /* event & prop definitions */)
+            expect(queryCounter).toBe(10 + 14 /* event & prop definitions */)


👀

which query doesn't happen anymore?

We don't do shouldSendWebhooks anymore 🎉

neilkakkar · 2021-06-18T10:41:18Z

+            expect(organization!.name).toEqual('TEST ORG')
+            expect(hub.db.postgresQuery).toHaveBeenCalledTimes(0)
+
+            jest.spyOn(global.Date, 'now').mockImplementation(() => new Date('2020-02-27 11:00:36').getTime())


Is this testing the cache TTL ?

Yes, this whole organization-manager thing is almost totally copied from TeamManager code, because it's very much alike

neilkakkar

A couple of questions, but otherwise looks good to me!

yakkomajuri

didn't run it yet but mostly looks good - let me try it out

yakkomajuri · 2021-06-18T11:05:18Z

 import { UUID } from './utils/utils'
 import { ActionManager } from './worker/ingestion/action-manager'
 import { ActionMatcher } from './worker/ingestion/action-matcher'
+import { HookCannon } from './worker/ingestion/hooks'


yakkomajuri · 2021-06-18T11:08:31Z

+    switch (typeof value) {
+        case 'string':
+            return value
+        case 'undefined':


I think JSON.stringify(undefined) returns "undefined" already

It in fact returns undefined instead of "undefined", which caught me off guard for a bit 😅

yakkomajuri · 2021-06-18T11:12:59Z

+    if (url.includes('discord.com')) {
+        return WebhookType.Discord
+    }
+    return WebhookType.Teams


Is this a good approach? Probably legacy so should be fine, but while this might not be harmful, I think I'd like some explicit Teams checking with an additional unknown/unspecified type, just for clarity.

I regularly use https://webhook.site/ for example

Well, Teams is just the default behavior this way. I don't know what webhook.site accepts and I don't want to support it, so Teams format it is.

That makes sense yeah - webhook.site is just a webhook dump.

When I added this comment I didn't see what the type was being used for, but yeah, if it's just formatting the message/payload, that's perfectly fine

yakkomajuri · 2021-06-18T11:20:13Z

+    const actionName = stringify(action.name)
+    let actionMarkdown: string
+    if (webhookType === WebhookType.Slack) {
+        actionMarkdown = `<${siteUrl}/action/${action.id}|${action.name}>`


maybe use actionName instead of action.name here too? (and below)

yakkomajuri · 2021-06-18T11:21:55Z

+}
+
+export function getTokens(messageFormat: string): [string[], string] {
+    const matchedTokens = messageFormat.match(/(?<=\[)(.*?)(?=\])/g) || []


nit: as you know, I always appreciate a regex match example in a comment

yakkomajuri · 2021-06-18T11:30:36Z

+
+        const timeout = timeoutGuard(`Still running "fetchOrganization". Timeout warning after 30 sec!`)
+        try {
+            const organizationQueryResult = await this.db.postgresQuery(


genuine q: we have SQL queries spread in multiple areas of the codebase, db, sql, and individual queries in some files here and there. do we have any sort of convention for where they should be?

I think for some more bespoke stuff (like TeamManager.updateEventNamesAndProperties), it doesn't make sense to make it common via DB, BUT thanks for bringing this line up – I updated it to use DB.fetchOrganization (and similarly in TeamManager with fetchTeam)

yakkomajuri · 2021-06-18T14:03:23Z

Ran this locally. Action calculated fine the first time and sent the relevant webhooks.

Second time though the UI told me the action was recalculated but the events didn't update and I didn't get the webhooks.

Got some errors for posthog:

ConnectionResetError: [Errno 54] Connection reset by peer

Don't have time to investigate, could be a fluke - just putting it out here for context.

Twixes · 2021-06-18T14:06:07Z

What do you mean by "the events didn't update and I didn't get the webhooks"? You shouldn't get webhooks on action recalculation with this, the way Django does resend webhooks on action recalculation is unintended behavior.

yakkomajuri · 2021-06-18T14:10:55Z

Right yeah, ofc - webhooks should fire as events come in. I guess the correlation I saw was what got me back into the old thinking model. Either way, the action didn't recalculate a second time and I didn't get webhooks for all events that were sent in 🤔

yakkomajuri · 2021-06-18T14:11:57Z

I'd say feel free to ignore though if this works for you - I did indeed get some webhooks so the system is working

Twixes · 2021-06-18T14:22:26Z

Hm, all OK for me, recalculation should definitely not be affected because that's Django and not touched by PostHog/posthog#4794. I think we should be safe to go ahead

…ostHog/plugin-server#479) * Fire webhooks from internal action matching results * Add Zapier `postEventToRestHook` * Remove offloading of hooks to Python via Celery * Fire Zapier in the plugin server * Port webhook formatting tests over * Update queryCounter expectations * Update queryCounter expectations again * Fully test and fix webhook formatting * Reorganize hook firing into HookCannon * Remove duplicated test * Test OrganizationManager * Add end-to-end test for action matching saving * Add (almost) end-to-end tests for firing hooks * Run prettier * Rearrange `text = markdown = foo` * Fix `==`s * Rename `HookCannon` * Add TOKENS_REGEX comment * Use `fetchOrganization` and `fetchTeam` from DB

Twixes added 5 commits June 16, 2021 11:14

Fire webhooks from internal action matching results

5485895

Add Zapier postEventToRestHook

a75f204

Remove offloading of hooks to Python via Celery

0fbdb6c

Fire Zapier in the plugin server

9bf7514

Port webhook formatting tests over

31c1451

This was referenced Jun 17, 2021

Fire core webhooks and Zapier triggers from plugin server #459

Closed

Don't fire webhooks and Zapier in case of PLUGIN_SERVER_ACTION_MATCHING PostHog/posthog#4794

Merged

Twixes added 6 commits June 17, 2021 23:10

Update queryCounter expectations

4a2af03

Update queryCounter expectations again

f1bc859

Fully test and fix webhook formatting

0b28dd5

Reorganize hook firing into HookCannon

c52fe1a

Remove duplicated test

e9df1cd

Test OrganizationManager

f56816d

Twixes added the bump patch Bump patch version when this PR gets merged label Jun 17, 2021

Twixes added 3 commits June 18, 2021 03:25

Add end-to-end test for action matching saving

1895f5f

Add (almost) end-to-end tests for firing hooks

648cbab

Run prettier

301b962

Twixes requested review from neilkakkar and yakkomajuri June 18, 2021 09:32

Twixes marked this pull request as ready for review June 18, 2021 09:33

neilkakkar reviewed Jun 18, 2021

View reviewed changes

Comment thread src/worker/ingestion/hooks.ts

neilkakkar reviewed Jun 18, 2021

View reviewed changes

Comment thread src/worker/ingestion/hooks.ts

neilkakkar reviewed Jun 18, 2021

View reviewed changes

Twixes added 2 commits June 18, 2021 13:28

Rearrange text = markdown = foo

0b3340f

Fix ==s

7797515

Rename HookCannon

e14664f

yakkomajuri reviewed Jun 18, 2021

View reviewed changes

Twixes added 2 commits June 18, 2021 13:39

Add TOKENS_REGEX comment

c6905c9

Use fetchOrganization and fetchTeam from DB

ddd878f

Twixes requested a review from yakkomajuri June 18, 2021 11:59

Merge branch 'master' into hooks-firing

bbbed8b

Twixes merged commit f11a9f1 into master Jun 18, 2021

Twixes deleted the hooks-firing branch June 18, 2021 14:23

posthog-bot mentioned this pull request Jun 18, 2021

Update plugin server to 0.21.25 PostHog/posthog#4803

Merged

Twixes mentioned this pull request Jun 18, 2021

PropertyOperator ==, >, and < comparisons more loose via casting #481

Merged

2 tasks

	if (tokenParts[1] == 'name') {
	if (tokenParts[1] === 'name') {

Conversation

Twixes commented Jun 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Checklist

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neilkakkar Jun 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neilkakkar left a comment

Choose a reason for hiding this comment

Uh oh!

yakkomajuri left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yakkomajuri commented Jun 18, 2021

Uh oh!

Twixes commented Jun 18, 2021

Uh oh!

yakkomajuri commented Jun 18, 2021

Uh oh!

yakkomajuri commented Jun 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Twixes commented Jun 18, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Twixes commented Jun 16, 2021 •

edited

Loading

neilkakkar Jun 18, 2021 •

edited

Loading

yakkomajuri commented Jun 18, 2021 •

edited

Loading