Skip to content

Commit

Permalink
Decrease outgoing webhook timeouts from 10secs to 4secs (#3639)
Browse files Browse the repository at this point in the history
# Which issue(s) this PR fixes

See all the context
[here](https://raintank-corp.slack.com/archives/C025VMT6SPK/p1704802171131009?thread_ts=1704762857.043879&cid=C025VMT6SPK)

<img width="690" alt="Screenshot 2024-01-09 at 15 26 33"
src="https://github.com/grafana/oncall/assets/9406895/e4c794a3-508d-4f24-af22-0f800828271d">


## Checklist

- [ ] Unit, integration, and e2e (if applicable) tests updated
- [x] Documentation added (or `pr:no public docs` PR label added if not
required)
- [x] `CHANGELOG.md` updated (or `pr:no changelog` PR label added if not
required)
  • Loading branch information
joeyorlando authored Jan 10, 2024
1 parent 4cc4099 commit 006ee4b
Show file tree
Hide file tree
Showing 4 changed files with 20 additions and 14 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
### Changed

- Do not retry `firebase.messaging.UnregisteredError` exceptions for FCM relay tasks by @joeyorlando ([#3637](https://github.com/grafana/oncall/pull/3637))
- Decrease outgoing webhook timeouts from 10secs to 4secs by @joeyorlando ([#3639](https://github.com/grafana/oncall/pull/3639))

### Fixed

Expand Down
17 changes: 10 additions & 7 deletions docs/sources/outgoing-webhooks/_index.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,13 @@ weight: 500

# Outgoing Webhooks

> ⚠️ A note about **(Legacy)** webhooks: Webhooks that were created before version **v1.3.11** are marked as
> **(Legacy)**. Do not worry! They are still connected to their respective escalation chains and will continue to to
> ⚠️ A note about **(Legacy)** webhooks: Webhooks that were created before version **v1.3.11** are marked as
> **(Legacy)**. Do not worry! They are still connected to their respective escalation chains and will continue to to
> execute as they always have.
> <br/><br/>
> The **(Legacy)** webhook is no longer editable due to changes to the internal representation. If you need to edit it
> you must use the `Make a copy` action in the menu and make your changes there. This will create the webhook in the
> new format. Be sure to change your escalation chains to point to the new copy otherwise it will not be active. The
> you must use the `Make a copy` action in the menu and make your changes there. This will create the webhook in the
> new format. Be sure to change your escalation chains to point to the new copy otherwise it will not be active. The
> **(Legacy)** webhook can then be deleted.
Outgoing webhooks are used by Grafana OnCall to send data to a URL in a flexible way. These webhooks can be
Expand All @@ -33,7 +33,7 @@ To create an outgoing webhook navigate to **Outgoing Webhooks** and click **+ Cr
webhooks can be viewed, edited and deleted. To create the outgoing webhook click **New Outgoing Webhook** and then
select a preset based on what you want to do. A simple webhook will POST alert group data as a selectable escalation
step to the specified url. If you require more customization use the advanced webhook which provides all of the
fields described below.
fields described below.

### Outgoing webhook fields

Expand Down Expand Up @@ -63,7 +63,7 @@ Controls whether the outgoing webhook will trigger or is ignored.

#### Assign to Team

Sets which team owns the outgoing webhook for filtering and visibility.
Sets which team owns the outgoing webhook for filtering and visibility.
This setting does not restrict outgoing webhook execution to events from the selected team.

| Required | [Template Accepted](#outgoing-webhook-templates) | Default Value |
Expand Down Expand Up @@ -111,6 +111,9 @@ If no integrations are selected the outgoing webhook will trigger for any integr

The destination URL the outgoing webhook will make a request to. This must be a FQDN.

> ⚠️ **Note** the destination server must respond back within 4 seconds or it will result in a timeout
> (this can be seen in the "Response Body" under the "Last Run" section)
| Required | [Template Accepted](#outgoing-webhook-templates) | Default Value |
| :------: | :----------------------------------------------: | :-----------: |
| ✔️ | ✔️ | _Empty_ |
Expand Down Expand Up @@ -467,7 +470,7 @@ otherwise it will only display the value. Fields which are not used are not show

### Using trigger template field

The [trigger template field](#trigger-type) can be used to provide control over whether a webhook will execute.
The [trigger template field](#trigger-type) can be used to provide control over whether a webhook will execute.
This is useful in situations where many different kinds of alerts are going to the same integration but only some of
them should call the webhook. To accomplish this the trigger template field can contain a template that will process
data from the alert group and evaluate to empty, True or 1 if the webhook should execute, any other values will result
Expand Down
14 changes: 8 additions & 6 deletions engine/apps/webhooks/tests/test_trigger_webhook.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@
from apps.webhooks.tasks.trigger_webhook import NOT_FROM_SELECTED_INTEGRATION
from settings.base import WEBHOOK_RESPONSE_LIMIT

TIMEOUT = 4


class MockResponse:
def __init__(self, status_code=200, content=None):
Expand Down Expand Up @@ -161,7 +163,7 @@ def test_execute_webhook_ok(
assert mock_requests.post.called
expected_call = call(
"https://something/{}/".format(alert_group.public_primary_key),
timeout=10,
timeout=TIMEOUT,
headers={"some-header": alert_group.public_primary_key},
json={"value": alert_group.public_primary_key},
)
Expand Down Expand Up @@ -324,7 +326,7 @@ def test_execute_webhook_ok_forward_all(
}
expected_call = call(
"https://something/{}/".format(alert_group.public_primary_key),
timeout=10,
timeout=TIMEOUT,
headers={},
json=expected_data,
)
Expand Down Expand Up @@ -405,7 +407,7 @@ def test_execute_webhook_using_responses_data(
expected_data = {"value": "updated"}
expected_call = call(
"https://something/third-party-id/",
timeout=10,
timeout=TIMEOUT,
headers={},
json=expected_data,
)
Expand Down Expand Up @@ -547,7 +549,7 @@ def test_response_content_limit(
assert mock_requests.post.called
expected_call = call(
"https://test/",
timeout=10,
timeout=TIMEOUT,
headers={},
)
assert mock_requests.post.call_args == expected_call
Expand Down Expand Up @@ -595,7 +597,7 @@ def test_manually_retried_exceptions(
# should retry
execute_webhook(*execute_webhook_args)

mock_requests.post.assert_called_once_with("https://test/", timeout=10, headers={})
mock_requests.post.assert_called_once_with("https://test/", timeout=TIMEOUT, headers={})
spy_execute_webhook.apply_async.assert_called_once_with((*execute_webhook_args, 1), countdown=10)

mock_requests.reset_mock()
Expand All @@ -607,5 +609,5 @@ def test_manually_retried_exceptions(
except Exception:
pytest.fail()

mock_requests.post.assert_called_once_with("https://test/", timeout=10, headers={})
mock_requests.post.assert_called_once_with("https://test/", timeout=TIMEOUT, headers={})
spy_execute_webhook.apply_async.assert_not_called()
2 changes: 1 addition & 1 deletion engine/apps/webhooks/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
from apps.schedules.ical_utils import list_users_to_notify_from_ical
from common.jinja_templater import apply_jinja_template

OUTGOING_WEBHOOK_TIMEOUT = 10
OUTGOING_WEBHOOK_TIMEOUT = 4


class InvalidWebhookUrl(Exception):
Expand Down

0 comments on commit 006ee4b

Please sign in to comment.