feat(webhook): Break down SendWebhookJob into 2 jobs (one creates the models, the other sends HTTP) #1993
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
72faba2 to
009c339
Compare
|
Let's merge and deploy Monday 05/20. |
bfcb155 to
40d4b59
Compare
42e701c to
3b385b7
Compare
julienbourdeau
added a commit
that referenced
this pull request
May 30, 2024
## Context I recently [refactored the webhook](#1993) model to split the SendWebhookJob into 2 jobs. The payload was previously stored as a string (json serialized hash) inside a JSON column. It's now a hash stored in a JSON column. I introduced a bug where the webhook sent to the GraphQL API wasn't in the format expected. @lovrocolic fixed in #2053 ## Description Currently, if you open an old webhook, `w.payload` will return a string. Newer payload would return a hash. This PR ensures you always get a hash. Note that there was no release since #1993. Webhooks are deleted after 90 days, so "one day", all webhooks will be stored in hash.
julienbourdeau
added a commit
that referenced
this pull request
Jul 23, 2024
## Description Currently, we often make a query to the DB to figure if we should enqueue a job to send the webhooks. As discussed during our tech meeting about a month ago, we're moving to always enqueuing a job. The job will simply do nothing if there is no webhook_endpoint configured for the given organization. It doesn't show in the the diff but the orgs without endpoints won't receive any webhook. https://github.com/getlago/lago-api/blob/b615f63a405efe81990436667f99c6ee34b408e6/app/services/webhooks/base_service.rb#L24-L27 ### Benefits * Centralized logic of _"should we send webhook?"_ in the job (easier if want to [subscribe to only a subset of webhooks](#1993)) * Saves a query during the main tasks ### Down sides * Enqueues more jobs (very small jobs)
abdussamadbello
pushed a commit
to abdussamadbello/lago-api
that referenced
this pull request
Aug 8, 2024
## Description Currently, we often make a query to the DB to figure if we should enqueue a job to send the webhooks. As discussed during our tech meeting about a month ago, we're moving to always enqueuing a job. The job will simply do nothing if there is no webhook_endpoint configured for the given organization. It doesn't show in the the diff but the orgs without endpoints won't receive any webhook. https://github.com/getlago/lago-api/blob/b615f63a405efe81990436667f99c6ee34b408e6/app/services/webhooks/base_service.rb#L24-L27 ### Benefits * Centralized logic of _"should we send webhook?"_ in the job (easier if want to [subscribe to only a subset of webhooks](getlago#1993)) * Saves a query during the main tasks ### Down sides * Enqueues more jobs (very small jobs)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Last year we added the possibility to add multiple webhook endpoints. It means currently one
SendWebhookJobcan make up to 5 HTTP calls.This PR break down the
Webhook::BaseService. Before the service was responsible for genereating thewebhookmodel (generating payload, attach to WebhookEndpoint...) AND making the HTTP calls.Now, the service create a model and dispatch a new job using this new webhook model. Normally, the HTTP part is where the error happen and the retrying wasn't too obvious. We had to carry the options and the object which we didn't need since the webhook model was already saved
Unintended benefit: I also had to refactor the test and make them much smaller. Notice that it's a separate commit to help with review.
Other things I'd like to do with webhook:
retryingstatus. I believefailedandretryingare different.through:instead of adelegateorganization.webhook_endpointcolumnWEBHOOK_SERVICEShash from job to Webhook modelWhy now?
I was originally looking at how we can let users "subscribe" to only a subset of webhooks. This would help our customers receive less calls and us make way less http requests. With the current code, the retry strategy wasn't too obvious. This PR makes the next step very easy
current_organization.webhook_endpoints.each do |webhook_endpoint| + next unless webhook_endpoint.subscribed_to webhook_type webhook = create_webhook(webhook_endpoint, payload) SendHttpWebhookJob.perform_later(webhook) end