Skip to content
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
7 changes: 1 addition & 6 deletions src/sentry/options/defaults.py
Original file line number Diff line number Diff line change
Expand Up @@ -1246,12 +1246,7 @@
default=False,
flags=FLAG_MODIFIABLE_BOOL | FLAG_AUTOMATOR_MODIFIABLE,
)
register(
"seer.explorer_index.run_frequency.minutes",
type=Int,
default=1440, # 24 hours
flags=FLAG_AUTOMATOR_MODIFIABLE,
)

# Custom model costs mapping for AI Agent Monitoring. Used to map alternative model ids to existing model ids.
# {"alternative_model_id": "gpt-4o", "existing_model_id": "openai/gpt-4o"}
register(
Expand Down
29 changes: 10 additions & 19 deletions src/sentry/tasks/seer_explorer_index.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
from sentry.tasks.base import instrumented_task
from sentry.tasks.statistical_detectors import compute_delay
from sentry.taskworker.namespaces import seer_tasks
from sentry.utils.cache import cache
from sentry.utils.query import RangeQuerySetWrapper

logger = logging.getLogger("sentry.tasks.seer_explorer_indexer")
Expand All @@ -27,8 +26,8 @@
LAST_RUN_CACHE_TIMEOUT = 24 * 60 * 60 # 24 hours

EXPLORER_INDEX_PROJECTS_PER_BATCH = 100
# Use a larger prime number to spread indexing tasks throughout the day
EXPLORER_INDEX_DISPATCH_STEP = timedelta(seconds=127)
# Use a larger prime number to prevent thundering
EXPLORER_INDEX_DISPATCH_STEP = timedelta(seconds=37)

FEATURE_NAMES = ["organizations:gen-ai-features", "organizations:seer-explorer-index"]

Expand All @@ -41,6 +40,7 @@ def get_seer_explorer_enabled_projects() -> Generator[tuple[int, int]]:
Tuple of (project_id, organization_id)
"""
projects = Project.objects.filter(status=ObjectStatus.ACTIVE).select_related("organization")
current_hour = django_timezone.now().hour

for project in RangeQuerySetWrapper(
projects,
Comment on lines 40 to 46
Copy link

Choose a reason for hiding this comment

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

Bug: The sharding logic project.id % 23 will never equal current_hour when the hour is 23, causing projects for that hour to be skipped.
Severity: MEDIUM

Suggested Fix

To ensure all 24 hours are utilized, change the modulo divisor from 23 to 24. The condition should be updated to project.id % 24 == current_hour.

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/tasks/seer_explorer_index.py#L40-L46

Potential issue: The project filtering logic uses the expression `project.id % 23 ==
current_hour` to distribute the indexing workload across 23 hours of the day. However,
when the `current_hour` is 23, this condition can never be met because the result of a
modulo 23 operation is always an integer between 0 and 22, inclusive. As a result, any
projects that would have been scheduled for the 23rd hour will be skipped, and their
data will not be indexed by the Seer service. This is a silent failure that affects a
subset of projects.

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

Expand All @@ -50,6 +50,12 @@ def get_seer_explorer_enabled_projects() -> Generator[tuple[int, int]]:
logger.info("seer.explorer_index.killswitch.enable flag enabled, skipping")
return

if not project.flags.has_transactions:
continue

if project.id % 23 != current_hour:
continue
Copy link
Contributor

Choose a reason for hiding this comment

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

Hour sharding uses modulo 23 but hours range 0-23

Medium Severity

The sharding logic project.id % 23 != django_timezone.now().hour has an off-by-one mismatch. project.id % 23 produces values 0-22 (23 distinct values), but django_timezone.now().hour produces values 0-23 (24 distinct values). When the current hour is 23, no projects will ever match because no project ID modulo 23 can equal 23. This means projects are never indexed during hour 23 (11pm) each day, creating a gap in the indexing schedule.

Fix in Cursor Fix in Web

Copy link
Contributor

Choose a reason for hiding this comment

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

Hour check evaluated repeatedly can cause race condition

Medium Severity

The django_timezone.now().hour is called fresh for each project inside the RangeQuerySetWrapper loop. If the iteration takes time and crosses an hour boundary, projects evaluated before the boundary are checked against hour N, while projects evaluated after are checked against hour N+1. This causes some shard-N projects to be skipped (not processed for another 24 hours) and some shard-N+1 projects to be processed early (then again at the next scheduled run). The hour value needs to be captured once before the loop begins.

Fix in Cursor Fix in Web

Copy link
Member

Choose a reason for hiding this comment

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

Seems unlikely to happen in prod but probably good practice in case it cases instability in tests.


with sentry_sdk.start_span(op="seer_explorer_index.has_feature"):
batch_result = features.batch_has(FEATURE_NAMES, organization=project.organization)

Expand Down Expand Up @@ -90,17 +96,6 @@ def schedule_explorer_index() -> None:
logger.info("seer.explorer_index.enable flag is disabled")
return

last_run = cache.get(LAST_RUN_CACHE_KEY)

# Default is 24 hours, making it an option so we can trigger
# the run at higher frequencies as we roll it out.
explorer_index_run_frequency = timedelta(
minutes=options.get("seer.explorer_index.run_frequency.minutes")
)
if last_run and last_run > django_timezone.now() - explorer_index_run_frequency:
logger.info("Index updated less than 24 hours ago, skiping")
return

now = django_timezone.now()

projects = get_seer_explorer_enabled_projects()
Expand All @@ -111,8 +106,6 @@ def schedule_explorer_index() -> None:
for _ in projects:
scheduled_count += 1

cache.set(LAST_RUN_CACHE_KEY, now, LAST_RUN_CACHE_TIMEOUT)

logger.info(
"Successfully scheduled explorer index jobs for valid projects",
extra={
Expand All @@ -139,9 +132,7 @@ def dispatch_explorer_index_projects(
batch: list[tuple[int, int]] = []
count = 0

explorer_index_run_frequency = timedelta(
minutes=options.get("seer.explorer_index.run_frequency.minutes")
)
explorer_index_run_frequency = timedelta(hours=1)

for project_id, org_id in all_projects:
batch.append((project_id, org_id))
Expand Down
144 changes: 94 additions & 50 deletions tests/sentry/tasks/test_seer_explorer_index.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,11 @@
from sentry.testutils.cases import TestCase
from sentry.testutils.helpers.datetime import freeze_time
from sentry.testutils.pytest.fixtures import django_db_all
from sentry.utils.cache import cache


@django_db_all
class TestGetSeerExplorerEnabledProjects(TestCase):
@freeze_time("2024-01-15 12:00:00")
def test_returns_projects_with_feature_flag(self):
org1 = self.create_organization()
org2 = self.create_organization()
Expand All @@ -34,6 +34,13 @@ def test_returns_projects_with_feature_flag(self):
project4 = self.create_project(organization=org3)
project5 = self.create_project(organization=org4)

project1.flags.has_transactions = True
project1.save()
project2.flags.has_transactions = True
project2.save()
project3.flags.has_transactions = True
project3.save()

PromptsActivity.objects.create(
organization_id=org1.id,
project_id=0,
Expand All @@ -56,18 +63,26 @@ def test_returns_projects_with_feature_flag(self):
result = list(get_seer_explorer_enabled_projects())

project_ids = [p[0] for p in result]
assert project1.id in project_ids
assert project2.id in project_ids
assert project3.id in project_ids
expected_ids = []
for p in [project1, project2, project3]:
if p.id % 23 == 12:
expected_ids.append(p.id)

assert all(pid in project_ids for pid in expected_ids)
assert project4.id not in project_ids
assert project5.id not in project_ids
assert result[0] == (project1.id, org1.id)

@freeze_time("2024-01-15 12:00:00")
def test_excludes_inactive_projects(self):
org = self.create_organization()
active_project = self.create_project(organization=org)
inactive_project = self.create_project(organization=org)

active_project.flags.has_transactions = True
active_project.save()
inactive_project.flags.has_transactions = True
inactive_project.save()

inactive_project.update(status=ObjectStatus.PENDING_DELETION)

PromptsActivity.objects.create(
Expand All @@ -86,7 +101,8 @@ def test_excludes_inactive_projects(self):
result = list(get_seer_explorer_enabled_projects())

project_ids = [p[0] for p in result]
assert active_project.id in project_ids
if active_project.id % 23 == 12:
assert active_project.id in project_ids
assert inactive_project.id not in project_ids

def test_returns_empty_without_feature_flag(self):
Expand Down Expand Up @@ -127,10 +143,14 @@ def test_excludes_projects_with_hide_ai_features(self):
assert len(result) == 0
assert project.id not in [p[0] for p in result]

@freeze_time("2024-01-15 12:00:00")
def test_excludes_projects_without_seer_acknowledgement(self):
org = self.create_organization()
project = self.create_project(organization=org)

project.flags.has_transactions = True
project.save()

with self.feature(
{
"organizations:gen-ai-features": [org.slug],
Expand All @@ -142,25 +162,39 @@ def test_excludes_projects_without_seer_acknowledgement(self):
assert len(result) == 0
assert project.id not in [p[0] for p in result]

@freeze_time("2024-01-15 12:00:00")
def test_excludes_projects_without_transactions(self):
org = self.create_organization()
project = self.create_project(organization=org)

@django_db_all
class TestScheduleExplorerIndex(TestCase):
def setUp(self):
super().setUp()
cache.clear()
PromptsActivity.objects.create(
organization_id=org.id,
project_id=0,
user_id=self.user.id,
feature="seer_autofix_setup_acknowledged",
)

def test_skips_when_option_disabled(self):
with self.options({"seer.explorer_index.enable": False}):
with mock.patch(
"sentry.tasks.seer_explorer_index.get_seer_explorer_enabled_projects"
) as mock_get_projects:
schedule_explorer_index()
mock_get_projects.assert_not_called()
with self.feature(
{
"organizations:gen-ai-features": [org.slug],
"organizations:seer-explorer-index": [org.slug],
}
):
result = list(get_seer_explorer_enabled_projects())

@freeze_time("2024-01-15 12:00:00")
def test_schedules_projects_with_option_enabled(self):
assert len(result) == 0
assert project.id not in [p[0] for p in result]

def test_includes_only_projects_matching_hour_shard(self):
org = self.create_organization()
self.create_project(organization=org)

project1 = self.create_project(organization=org)
project2 = self.create_project(organization=org)

project1.flags.has_transactions = True
project1.save()
project2.flags.has_transactions = True
project2.save()

PromptsActivity.objects.create(
organization_id=org.id,
Expand All @@ -169,26 +203,43 @@ def test_schedules_projects_with_option_enabled(self):
feature="seer_autofix_setup_acknowledged",
)

with self.options(
{"seer.explorer_index.enable": True, "seer.explorer_index.run_frequency.minutes": 1440}
target_hour = project1.id % 23

with self.feature(
{
"organizations:gen-ai-features": [org.slug],
"organizations:seer-explorer-index": [org.slug],
}
):
with self.feature(
{
"organizations:gen-ai-features": [org.slug],
"organizations:seer-explorer-index": [org.slug],
}
):
with mock.patch(
"sentry.tasks.seer_explorer_index.dispatch_explorer_index_projects"
) as mock_dispatch:
mock_dispatch.return_value = iter([])
schedule_explorer_index()
mock_dispatch.assert_called_once()
with freeze_time(f"2024-01-15 {target_hour:02d}:00:00"):
result = list(get_seer_explorer_enabled_projects())

project_ids = [p[0] for p in result]
assert project1.id in project_ids

if project2.id % 23 == target_hour:
assert project2.id in project_ids
else:
assert project2.id not in project_ids


@django_db_all
class TestScheduleExplorerIndex(TestCase):
def test_skips_when_option_disabled(self):
with self.options({"seer.explorer_index.enable": False}):
with mock.patch(
"sentry.tasks.seer_explorer_index.get_seer_explorer_enabled_projects"
) as mock_get_projects:
schedule_explorer_index()
mock_get_projects.assert_not_called()

@freeze_time("2024-01-15 12:00:00")
def test_respects_cache_interval(self):
def test_schedules_projects_with_option_enabled(self):
org = self.create_organization()
self.create_project(organization=org)
project = self.create_project(organization=org)

project.flags.has_transactions = True
project.save()

PromptsActivity.objects.create(
organization_id=org.id,
Expand All @@ -197,9 +248,7 @@ def test_respects_cache_interval(self):
feature="seer_autofix_setup_acknowledged",
)

with self.options(
{"seer.explorer_index.enable": True, "seer.explorer_index.run_frequency.minutes": 1440}
):
with self.options({"seer.explorer_index.enable": True}):
with self.feature(
{
"organizations:gen-ai-features": [org.slug],
Expand All @@ -210,12 +259,8 @@ def test_respects_cache_interval(self):
"sentry.tasks.seer_explorer_index.dispatch_explorer_index_projects"
) as mock_dispatch:
mock_dispatch.return_value = iter([])

schedule_explorer_index()
assert mock_dispatch.call_count == 1

schedule_explorer_index()
assert mock_dispatch.call_count == 1
mock_dispatch.assert_called_once()


@django_db_all
Expand All @@ -225,11 +270,10 @@ def test_batches_projects_with_delays(self):
timestamp = datetime(2024, 1, 15, 12, 0, 0, tzinfo=UTC)
projects = [(i, 1) for i in range(150)]

with self.options({"seer.explorer_index.run_frequency.minutes": 1440}):
with mock.patch(
"sentry.tasks.seer_explorer_index.run_explorer_index_for_projects.apply_async"
) as mock_task:
result = list(dispatch_explorer_index_projects(iter(projects), timestamp))
with mock.patch(
"sentry.tasks.seer_explorer_index.run_explorer_index_for_projects.apply_async"
) as mock_task:
result = list(dispatch_explorer_index_projects(iter(projects), timestamp))

assert mock_task.call_count == 2
assert len(mock_task.call_args_list[0][1]["args"][0]) == 100
Expand Down
Loading