Skip to content

Commit 23e68ea

Browse files
perf(tests): skip stack walk in silo transaction validation unless it would raise (#127937)
`validate_transaction_using_for_silo_mode` runs on every `transaction.atomic` / `on_commit` / `get_connection` call. In tests it called `is_in_test_case_body()` first, which `str()`s every stack frame and runs regexes over them. That walk happened about 500 times per test, even though its result only matters when the silo check would raise. This PR: - Runs the cheap silo/`using` checks first and walks the stack only on a mismatch, just before raising. The function raises in exactly the same cases as before. Production behavior doesn't change, because the stack walk only ran under `in_test_environment()`. - Changes `is_in_test_case_body()` to compare `co_name`/`co_filename` on `sys._getframe()` frames instead of using `str(frame)` + regex. It matches the same frames. **Evidence** - cProfile on `tests/sentry/api/bases/test_organization.py::GetProjectIdsTest` (25 tests): `validate_transaction_using_for_silo_mode` went from 5.7s to 0.43s cumulative, and `is_in_test_case_body` no longer runs. The call phase went from 27.5s to 20.3s. - Local `tests/sentry/api/bases/`: 54.3s to 45.6s (avg of 2 runs). - Full backend suite on CI (workflow_dispatch, all 22 shards, 46,855 tests, green): the sum of per-test setup+call+teardown went from 26,753s to 23,775s (-11%). Runner speed varies by up to about 2.5x between shards, so I normalized by each shard's collection time, which this change doesn't touch. The per-shard median is -9%. Postgres-heavy directories like `tests/sentry/api/` and `tests/sentry/integrations/` drop 15-17%. Snuba-heavy ones drop about 3%. - `tests/sentry/silo/test_silo_aware_transaction_patch.py` passes, including the mismatch-raises cases. <!-- junior-request-attribution:start --> via @joshuarli. <!-- junior-request-attribution:end --> <!-- junior-session-footer:start --> <!-- junior-conversation-id:slack%3ACUHS29QJ0%3A1792248286.713619 --> -- [View Junior Session](https://junior-prod.sentry.dev/conversations/slack%3ACUHS29QJ0%3A1792248286.713619) [[Sentry]](https://sentry.sentry.io/explore/conversations/slack%3ACUHS29QJ0%3A1792248286.713619/?project=4510944073809921) <!-- junior-session-footer:end --> --------- Co-authored-by: sentry-junior[bot] <264270552+sentry-junior[bot]@users.noreply.github.com> Co-authored-by: Joshua Li <14209781+joshuarli@users.noreply.github.com>
1 parent ccfc7ab commit 23e68ea

1 file changed

Lines changed: 41 additions & 27 deletions

File tree

‎src/sentry/silo/patches/silo_aware_transaction_patch.py‎

Lines changed: 41 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
1-
import re
2-
import traceback
1+
import functools
2+
import sys
33
from collections.abc import Callable
4+
from types import CodeType, FrameType
45
from typing import TYPE_CHECKING, Any
56

67
from django.db import router, transaction
@@ -74,17 +75,30 @@ def is_in_test_case_body() -> bool:
7475
This should not be used as a general-purpose utility function. Avoid calling it
7576
in places other than `validate_transaction_using_for_silo_mode` if at all possible.
7677
"""
77-
frames = [str(frame) for (frame, _) in traceback.walk_stack(None)]
78-
79-
def seek(module_path: str, function_name: str) -> bool:
80-
"""Check whether the named function has been called in the current stack."""
81-
pattern = re.compile(rf"/{re.escape(module_path)}\b.*\b{re.escape(function_name)}>$")
82-
return any(pattern.search(frame) for frame in frames)
83-
84-
return seek("_pytest/runner.py", "pytest_runtest_call") and not (
85-
seek("django/test/testcases.py", "_pre_setup")
86-
or seek("django/test/testcases.py", "_post_teardown")
78+
test_call, setup_teardown = _test_frame_codes()
79+
in_test_call = False
80+
frame: FrameType | None = sys._getframe(1)
81+
while frame is not None:
82+
if frame.f_code in setup_teardown:
83+
return False
84+
in_test_call = in_test_call or frame.f_code is test_call
85+
frame = frame.f_back
86+
return in_test_call
87+
88+
89+
@functools.cache
90+
def _test_frame_codes() -> tuple[CodeType, frozenset[CodeType]]:
91+
# Imported lazily: this module also loads in production, where pytest isn't installed.
92+
from _pytest.runner import pytest_runtest_call
93+
from django.test import testcases
94+
95+
setup_teardown = frozenset(
96+
getattr(method, "__func__", method).__code__
97+
for cls in (testcases.SimpleTestCase, testcases.TransactionTestCase)
98+
for name in ("_pre_setup", "_post_teardown")
99+
if (method := vars(cls).get(name)) is not None
87100
)
101+
return pytest_runtest_call.__code__, setup_teardown
88102

89103

90104
def validate_transaction_using_for_silo_mode(using: str | None) -> None:
@@ -94,29 +108,29 @@ def validate_transaction_using_for_silo_mode(using: str | None) -> None:
94108
if using is None:
95109
raise TransactionMissingDBException("'using' must be specified when creating a transaction")
96110

97-
if in_test_environment() and not is_in_test_case_body():
98-
# During setup and teardown of a test environment, allow treating it as one
99-
# DB. But make sure we enforce as normal during the actual test case.
111+
current_silo_mode = SiloMode.get_current_mode()
112+
if current_silo_mode == SiloMode.MONOLITH:
100113
return
101114

102-
current_silo_mode = SiloMode.get_current_mode()
103115
control_db = _get_db_for_model_if_available(ControlOutbox)
104116
cell_db = _get_db_for_model_if_available(CellOutbox)
117+
if control_db == cell_db:
118+
return
105119

106-
both_silos_route_to_same_db = control_db == cell_db
107-
108-
if both_silos_route_to_same_db or current_silo_mode == SiloMode.MONOLITH:
120+
if using == control_db and current_silo_mode != SiloMode.CONTROL:
121+
error = f"Cannot use transaction.atomic({using}) except in Control Mode"
122+
elif using == cell_db and current_silo_mode != SiloMode.CELL:
123+
error = f"Cannot use transaction.atomic({using}) except in Cell Mode"
124+
else:
109125
return
110126

111-
elif using == control_db and current_silo_mode != SiloMode.CONTROL:
112-
raise MismatchedSiloTransactionError(
113-
f"Cannot use transaction.atomic({using}) except in Control Mode"
114-
)
127+
# During setup and teardown of a test environment, allow treating it as one
128+
# DB. But make sure we enforce as normal during the actual test case. Checked
129+
# last because walking the stack is expensive.
130+
if in_test_environment() and not is_in_test_case_body():
131+
return
115132

116-
elif using == cell_db and current_silo_mode != SiloMode.CELL:
117-
raise MismatchedSiloTransactionError(
118-
f"Cannot use transaction.atomic({using}) except in Cell Mode"
119-
)
133+
raise MismatchedSiloTransactionError(error)
120134

121135

122136
def patch_silo_aware_atomic() -> None:

0 commit comments

Comments
 (0)