Skip to content
This repository has been archived by the owner on Feb 8, 2024. It is now read-only.

feat: Support for non transactional consumer #9

Merged
merged 5 commits into from
Dec 21, 2023

Conversation

tomasfarias
Copy link
Contributor

We do not yet want to commit to either transactional or non transactional running modes, so this makes them configurable in the consumer side. There is perhaps some opportunity for abstraction with traits, but if we commit for one of the two options early enough we could end up dropping the other code.

@tomasfarias tomasfarias marked this pull request as ready for review December 14, 2023 13:50
@tomasfarias tomasfarias force-pushed the feat/support-for-non-transactional-consumer branch from 5dc174b to c65b913 Compare December 14, 2023 13:53
hook-consumer/src/main.rs Outdated Show resolved Hide resolved
@tomasfarias tomasfarias force-pushed the feat/support-for-non-transactional-consumer branch from c65b913 to b0f3083 Compare December 15, 2023 10:24
@tomasfarias
Copy link
Contributor Author

tomasfarias commented Dec 15, 2023

Looks like this PR has some conflicts with #4. The crux of the issue was that we updated the WebhookJobParameters. I originally envisioned this struct matching the interface returned by a composeWebhook app, which is defined in plugin-scaffold: https://github.com/PostHog/plugin-scaffold/blob/main/src/types.ts#L15.

@bretthoerner I don't think we'll get team_id, plugin_config_id, plugin_id as the job parameters, given that they are not part of the Webhook interface, so I'm thinking of dropping these.

EDIT: In line with this, I've nested the WebhookJobParameters struct under a parameters key. And added the extra parameters as top level keys. Happy to discuss this, we can also iterate further as we see how this is called from plugin server.

@bretthoerner
Copy link
Contributor

bretthoerner commented Dec 15, 2023

@bretthoerner I don't think we'll get team_id, plugin_config_id, plugin_id as the job parameters, given that they are not part of the Webhook interface, so I'm thinking of dropping these.

We should discuss, because I'm not sure where we'd get them except the from plugin-server POST. I agree they won't come directly from the plugin itself, but we have other metadata we'll need, and if it can't fit into the parameters column then we'll need a second column (which feels specific to Webhooks?) or something, and then we'd still need a type that wraps the Webhook parameters for the HTTP endpoint:

https://github.com/PostHog/posthog/blob/fbb93232c4f4d07dce9c1c6ec6ff541a2ce3a8b9/plugin-server/src/worker/plugins/run.ts#L79-L141

edit: Ah, I hadn't read the new WebhookPostRequestBody when I wrote this. That seems fine to me, too, we'll just need to adjust plugin-server to send that shape. Although, does the full WebhookPostRequestBody make it into the DB parameters column? Those fields are necessary for after the job run (sending metrics/logs).

@tomasfarias tomasfarias force-pushed the feat/support-for-non-transactional-consumer branch from 62db01f to bd4dc4b Compare December 20, 2023 13:20
@bretthoerner
Copy link
Contributor

👍

@tomasfarias tomasfarias merged commit 934a0e0 into main Dec 21, 2023
4 checks passed
@tomasfarias tomasfarias deleted the feat/support-for-non-transactional-consumer branch December 21, 2023 11:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants