Skip to content

fix(discord): Catch case where response from Discord takes a while #57522

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

Merged
merged 6 commits into from
Oct 5, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/sentry/incidents/logic.py
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
from sentry.services.hybrid_cloud.integration.model import RpcOrganizationIntegration
from sentry.shared_integrations.exceptions import (
ApiError,
ApiTimeoutError,
DuplicateDisplayNameError,
IntegrationError,
)
Expand Down Expand Up @@ -1349,6 +1350,10 @@ def get_alert_rule_trigger_action_discord_channel_id(
)
except IntegrationError:
raise InvalidTriggerActionError("Bad response from Discord channel lookup")
except ApiTimeoutError:
raise ChannelLookupTimeoutError(
"Could not find channel %s. We have timed out trying to look for it." % name
)

return name

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

from sentry.integrations.discord.utils.channel import validate_channel_id
from sentry.services.hybrid_cloud.integration import integration_service
from sentry.shared_integrations.exceptions import IntegrationError
from sentry.shared_integrations.exceptions import ApiTimeoutError, IntegrationError


class DiscordNotifyServiceForm(forms.Form):
Expand Down Expand Up @@ -61,4 +61,6 @@ def clean(self) -> dict[str, object] | None:
self._format_discord_error_message("; ".join(str(e))),
code="invalid",
)
except ApiTimeoutError:
raise forms.ValidationError("Discord channel lookup timed out")
return cleaned_data
13 changes: 12 additions & 1 deletion src/sentry/integrations/discord/utils/channel.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
from __future__ import annotations

from django.core.exceptions import ValidationError
from requests.exceptions import Timeout

from sentry.integrations.discord.client import DiscordClient
from sentry.shared_integrations.exceptions import IntegrationError
from sentry.shared_integrations.exceptions import ApiTimeoutError, IntegrationError
from sentry.shared_integrations.exceptions.base import ApiError

from . import logger
Expand Down Expand Up @@ -65,6 +66,16 @@ def validate_channel_id(
},
)
raise IntegrationError("Bad response from Discord channel lookup.")
except Timeout:
logger.info(
"rule.discord.channel_lookup_timed_out",
extra={
"channel_id": channel_id,
"guild_name": guild_name,
"reason": "channel lookup timed out",
},
)
raise ApiTimeoutError("Discord channel lookup timed out")

if not isinstance(result, dict):
raise IntegrationError("Bad response from Discord channel lookup.")
Expand Down
45 changes: 44 additions & 1 deletion tests/sentry/incidents/test_logic.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
WARNING_TRIGGER_LABEL,
WINDOWED_STATS_DATA_POINTS,
AlertRuleTriggerLabelAlreadyUsedError,
ChannelLookupTimeoutError,
InvalidTriggerActionError,
ProjectsNotAssociatedWithAlertRuleError,
create_alert_rule,
Expand Down Expand Up @@ -67,7 +68,7 @@
)
from sentry.models import ActorTuple, Integration, OrganizationIntegration
from sentry.models.actor import get_actor_id_for_user
from sentry.shared_integrations.exceptions import ApiError, ApiRateLimitedError
from sentry.shared_integrations.exceptions import ApiError, ApiRateLimitedError, ApiTimeoutError
from sentry.snuba.dataset import Dataset
from sentry.snuba.models import QuerySubscription, SnubaQuery, SnubaQueryEventType
from sentry.testutils.cases import BaseIncidentsTest, BaseMetricsTestCase, SnubaTestCase, TestCase
Expand Down Expand Up @@ -1911,6 +1912,48 @@ def test_discord_no_integration(self):
integration_id=None,
)

@responses.activate
@mock.patch("sentry.integrations.discord.utils.channel.validate_channel_id")
def test_discord_timeout(self, mock_validate_channel_id):
mock_validate_channel_id.side_effect = ApiTimeoutError("Discord channel lookup timed out")

base_url: str = "https://discord.com/api/v10"
channel_id = "channel-id"
guild_id = "example-discord-server"
guild_name = "Server Name"

integration = Integration.objects.create(
provider="discord",
name="Example Discord",
external_id=f"{guild_id}",
metadata={
"guild_id": f"{guild_id}",
"name": f"{guild_name}",
},
)

integration.add_organization(self.organization, self.user)
type = AlertRuleTriggerAction.Type.DISCORD
target_type = AlertRuleTriggerAction.TargetType.SPECIFIC
responses.add(
method=responses.GET,
url=f"{base_url}/channels/{channel_id}",
json={
"guild_id": f"{guild_id}",
"name": f"{guild_name}",
},
)

with self.feature("organizations:integrations-discord-metric-alerts"):
with pytest.raises(ChannelLookupTimeoutError):
update_alert_rule_trigger_action(
self.action,
type,
target_type,
target_identifier=channel_id,
integration_id=integration.id,
)


class DeleteAlertRuleTriggerAction(BaseAlertRuleTriggerActionTest, TestCase):
@cached_property
Expand Down
25 changes: 24 additions & 1 deletion tests/sentry/integrations/discord/test_issue_alert.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
from sentry.models.group import GroupStatus
from sentry.models.release import Release
from sentry.services.hybrid_cloud.integration import integration_service
from sentry.shared_integrations.exceptions import ApiTimeoutError
from sentry.testutils.cases import RuleTestCase, TestCase
from sentry.testutils.helpers.datetime import before_now, iso_format
from sentry.testutils.skips import requires_snuba
Expand Down Expand Up @@ -230,14 +231,18 @@ def test_integration_removed(self):
assert len(results) == 0

@responses.activate
@mock.patch("sentry.integrations.discord.actions.issue_alert.form.validate_channel_id")
@mock.patch(
"sentry.integrations.discord.actions.issue_alert.form.validate_channel_id",
return_value=None,
)
def test_get_form_instance(self, mock_validate_channel_id):
form = self.rule.get_form_instance()
form.full_clean()
assert form.is_valid()
assert int(form.cleaned_data["server"]) == self.discord_integration.id
assert form.cleaned_data["channel_id"] == self.channel_id
assert form.cleaned_data["tags"] == self.tags
assert mock_validate_channel_id.call_count == 1

@responses.activate
def test_label(self):
Expand Down Expand Up @@ -337,3 +342,21 @@ def test_invalid_channel_id(self, mock_validate_channel_id):
form.full_clean()
assert not form.is_valid()
assert mock_validate_channel_id.call_count == 1

@mock.patch(
"sentry.integrations.discord.actions.issue_alert.form.validate_channel_id",
side_effect=ApiTimeoutError("Discord channel lookup timed out"),
)
def test_channel_id_lookup_timeout(self, mock_validate_channel_id):
form = DiscordNotifyServiceForm(
data={
"server": self.discord_integration.id,
"channel_id": self.channel_id,
"tags": "environment",
},
integrations=self.integrations,
)

form.full_clean()
assert not form.is_valid()
assert mock_validate_channel_id.call_count == 1
11 changes: 10 additions & 1 deletion tests/sentry/integrations/discord/test_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,11 @@

from django.core.exceptions import ValidationError
from pytest import raises
from requests.exceptions import Timeout

from sentry.integrations.discord.utils.auth import verify_signature
from sentry.integrations.discord.utils.channel import validate_channel_id
from sentry.shared_integrations.exceptions import IntegrationError
from sentry.shared_integrations.exceptions import ApiTimeoutError, IntegrationError
from sentry.shared_integrations.exceptions.base import ApiError
from sentry.testutils.cases import TestCase

Expand Down Expand Up @@ -92,3 +93,11 @@ def test_not_guild_member(self, mock_get_channel):
validate_channel_id(
self.channel_id, self.guild_id, self.integration_id, self.guild_name
)

@mock.patch("sentry.integrations.discord.utils.channel.DiscordClient.get_channel")
def test_timeout(self, mock_get_channel):
mock_get_channel.side_effect = Timeout("foo")
with raises(ApiTimeoutError):
validate_channel_id(
self.channel_id, self.guild_id, self.integration_id, self.guild_name
)