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

Webhook processing time limit #9573

Merged
merged 13 commits into from
Feb 13, 2021

Conversation

escopecz
Copy link
Sponsor Member

@escopecz escopecz commented Jan 11, 2021

Please be sure you are submitting this against the staging branch.

Q A
Bug fix? Y
New feature? Y
Automated tests included? N
Related user documentation PR URL /
Related developer documentation PR URL /
Issues addressed (#s or URLs) /
BC breaks? N
Deprecations? N

Description:

There are issues with webhook processing when there are too many events to send. The processing method is called recursively until the queue is empty. This was causing timeout issues in our system as the webhook processing never stopped due to new records in the webhook queue. This PR is adding 10 minutes time limit (configurable) to stop processing after the limit is hit.

This PR also improves the query getting rows from the webhook queue table and adds a new index that speeds up the query even more.

Steps to reproduce the bug:

  1. Set 'webhook_limit' => 1, in local.php which means send 1 event in 1 request (so it will take longer, that's what we need for our test).
  2. Create a webhook which will send the events to some website. The slower the website respond the better.
  3. Create around 50 webhook events. Sequel Pro has option to duplicate row with command + D which will speed it up.
  4. Run app/console m:w:p -i ID. Replace the ID with the ID of your webhook.
  5. It will process the whole queue at once. Check in the webhook_logs table that processing the first and the last log was longer than 1 second.

Steps to test this PR:

  1. Set 'webhook_time_limit' => 1, in local.php which means timeout the processing after 1 second.
  2. Repeat the test again. Now only part of the queue will be processed. Only those events that can be processed in 1 second. Execute the command again and another amount of events will be processed.

@cla-bot cla-bot bot added the cla-signed The PR contributors have signed the contributors agreement label Jan 11, 2021
@codecov
Copy link

codecov bot commented Jan 11, 2021

Codecov Report

Merging #9573 (ecfcbc0) into features (77f0da0) will increase coverage by 0.16%.
The diff coverage is 58.33%.

Impacted file tree graph

@@              Coverage Diff               @@
##             features    #9573      +/-   ##
==============================================
+ Coverage       38.33%   38.50%   +0.16%     
- Complexity      33939    33940       +1     
==============================================
  Files            1982     1982              
  Lines          104948   104952       +4     
==============================================
+ Hits            40236    40409     +173     
+ Misses          64712    64543     -169     
Impacted Files Coverage Δ Complexity Δ
app/bundles/WebhookBundle/Model/WebhookModel.php 71.56% <42.85%> (+33.68%) 71.00 <1.00> (+2.00)
...hookBundle/Command/ProcessWebhookQueuesCommand.php 68.75% <50.00%> (+39.71%) 8.00 <0.00> (ø)
...es/WebhookBundle/Entity/WebhookQueueRepository.php 88.88% <100.00%> (+88.88%) 4.00 <0.00> (-1.00) ⬆️
app/bundles/CoreBundle/Entity/CommonRepository.php 62.71% <0.00%> (+0.13%) 320.00% <0.00%> (ø%)
app/bundles/WebhookBundle/Entity/WebhookQueue.php 94.59% <0.00%> (+8.10%) 10.00% <0.00%> (ø%)
app/bundles/WebhookBundle/Entity/Event.php 73.33% <0.00%> (+11.11%) 10.00% <0.00%> (ø%)
app/bundles/WebhookBundle/Entity/Webhook.php 51.36% <0.00%> (+13.66%) 53.00% <0.00%> (ø%)
app/bundles/WebhookBundle/Entity/LogRepository.php 17.02% <0.00%> (+17.02%) 8.00% <0.00%> (ø%)
... and 4 more

@npracht npracht added bug Issues or PR's relating to bugs enhancement Any improvement to an existing feature or functionality ready-to-test PR's that are ready to test webhooks Anything related to webhooks labels Jan 12, 2021
@npracht npracht added this to the 3.3 milestone Jan 12, 2021
Copy link
Member

@kuzmany kuzmany left a comment

Choose a reason for hiding this comment

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

Works perfectly 👍
Approve it

@kuzmany kuzmany added pending-test-confirmation PR's that require one test before they can be merged and removed ready-to-test PR's that are ready to test labels Jan 29, 2021
@RCheesley RCheesley added ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged T1 Low difficulty to fix (issue) or test (PR) needs-documentation PR's that need documentation before they can be merged and removed pending-test-confirmation PR's that require one test before they can be merged labels Feb 12, 2021
@RCheesley
Copy link
Sponsor Member

I have added an issue to add this to the Dev Docs in the webhooks section: mautic/developer-documentation#187

@RCheesley RCheesley merged commit 82a8edf into mautic:features Feb 13, 2021
@escopecz escopecz deleted the webhook-processing-time-limit branch February 14, 2021 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issues or PR's relating to bugs cla-signed The PR contributors have signed the contributors agreement enhancement Any improvement to an existing feature or functionality needs-documentation PR's that need documentation before they can be merged ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged T1 Low difficulty to fix (issue) or test (PR) webhooks Anything related to webhooks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants