Skip to content
Merged
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/seer/anomaly_detection/get_anomaly_data.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import logging

from django.conf import settings
from urllib3 import Retry
from urllib3.exceptions import MaxRetryError, TimeoutError

from sentry.conf.server import SEER_ANOMALY_DETECTION_ENDPOINT_URL
Expand Down Expand Up @@ -31,6 +32,8 @@
timeout=settings.SEER_ANOMALY_DETECTION_TIMEOUT,
)

SEER_RETRIES = Retry(total=2, backoff_factor=0.5)
Copy link

Choose a reason for hiding this comment

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

Bug: The urllib3.Retry object is not configured to retry on TimeoutError exceptions, causing immediate failure instead of retries.
Severity: MEDIUM | Confidence: High

🔍 Detailed Analysis

The urllib3.Retry object, initialized with Retry(total=2, backoff_factor=0.5), does not automatically retry on TimeoutError exceptions. By default, it only retries on connection-related errors and specific HTTP status codes. Consequently, if a TimeoutError occurs during a Seer API call, the request will fail immediately without retrying, which contradicts the pull request's stated intention to handle minor availability blips.

💡 Suggested Fix

Configure the urllib3.Retry object to explicitly retry on TimeoutError by either adding TimeoutError to raise_on_status or raise_on_connection_error (if applicable) or by implementing a custom retry strategy.

🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: src/sentry/seer/anomaly_detection/get_anomaly_data.py#L35

Potential issue: The `urllib3.Retry` object, initialized with `Retry(total=2,
backoff_factor=0.5)`, does not automatically retry on `TimeoutError` exceptions. By
default, it only retries on connection-related errors and specific HTTP status codes.
Consequently, if a `TimeoutError` occurs during a Seer API call, the request will fail
immediately without retrying, which contradicts the pull request's stated intention to
handle minor availability blips.

Did we get this right? 👍 / 👎 to inform future reviews.
Reference_id: 2866257



# TODO: delete this once we deprecate the AlertRule model
def get_anomaly_data_from_seer_legacy(
Expand Down Expand Up @@ -95,6 +98,7 @@ def get_anomaly_data_from_seer_legacy(
SEER_ANOMALY_DETECTION_CONNECTION_POOL,
SEER_ANOMALY_DETECTION_ENDPOINT_URL,
data,
retries=SEER_RETRIES,
)
except (TimeoutError, MaxRetryError):
logger.warning("Timeout error when hitting anomaly detection endpoint", extra=extra_data)
Expand Down Expand Up @@ -213,6 +217,7 @@ def get_anomaly_data_from_seer(
SEER_ANOMALY_DETECTION_CONNECTION_POOL,
SEER_ANOMALY_DETECTION_ENDPOINT_URL,
json.dumps(detect_anomalies_request).encode("utf-8"),
retries=SEER_RETRIES,
)
except (TimeoutError, MaxRetryError):
logger.warning("Timeout error when hitting anomaly detection endpoint", extra=extra_data)
Expand Down
Loading