diff --git a/.github/workflows/notify-perf-on-db-queries.yml b/.github/workflows/notify-perf-on-db-queries.yml new file mode 100644 index 00000000000..5dffa53f052 --- /dev/null +++ b/.github/workflows/notify-perf-on-db-queries.yml @@ -0,0 +1,95 @@ +name: Notify Perf On DB Queries + +on: + pull_request: + branches: + - main + - litellm_internal_staging + - litellm_oss_staging + - "litellm_**" + +permissions: {} + +concurrency: + group: ${{ github.workflow }}-${{ github.event.pull_request.number }} + cancel-in-progress: true + +jobs: + notify-perf: + name: notify-perf + runs-on: ubuntu-latest + timeout-minutes: 10 + permissions: + contents: read + pull-requests: write + steps: + - uses: actions/checkout@08eba0b27e820071cde6df949e0beb9ba4906955 # v4.3.0 + with: + ref: ${{ github.event.pull_request.head.sha }} + fetch-depth: 1 + persist-credentials: false + + - uses: actions/setup-python@a26af69be951a213d495a4c3e4e4022e16d87065 # v5.6.0 + with: + python-version: "3.12" + + - name: Install detector dependencies + run: pip install --no-cache-dir "pydantic==2.13.4" + + - name: Detect added DB queries + id: detect + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + PR_NUMBER: ${{ github.event.pull_request.number }} + REPO: ${{ github.repository }} + run: | + gh api "repos/${REPO}/pulls/${PR_NUMBER}/files" --paginate \ + --jq '[.[] | {filename, patch}]' > changed_files.json + if python scripts/detect_new_db_queries.py < changed_files.json > findings.txt; then + echo "found=true" >> "$GITHUB_OUTPUT" + { + echo "### New DB access in this PR" + echo + echo '```' + cat findings.txt + echo '```' + } >> "$GITHUB_STEP_SUMMARY" + else + echo "found=false" >> "$GITHUB_OUTPUT" + fi + + - name: Request perf review + if: steps.detect.outputs.found == 'true' && github.event.pull_request.head.repo.fork == false + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + PR_NUMBER: ${{ github.event.pull_request.number }} + REPO: ${{ github.repository }} + PERF_REVIEWER: yassin-berriai + PR_AUTHOR: ${{ github.event.pull_request.user.login }} + run: | + if [ "$PR_AUTHOR" = "$PERF_REVIEWER" ]; then + echo "PR author is the perf reviewer; nothing to request." + exit 0 + fi + reviewed=$(gh api "repos/${REPO}/pulls/${PR_NUMBER}/reviews" --paginate \ + --jq "[.[] | select(.user.login == \"${PERF_REVIEWER}\")] | length") + if [ "$reviewed" != "0" ]; then + echo "${PERF_REVIEWER} already reviewed this PR." + exit 0 + fi + gh api --method POST "repos/${REPO}/pulls/${PR_NUMBER}/requested_reviewers" \ + -f "reviewers[]=${PERF_REVIEWER}" >/dev/null + marker="" + existing=$(gh api "repos/${REPO}/issues/${PR_NUMBER}/comments" --paginate \ + --jq "[.[] | select(.body | contains(\"${marker}\"))] | length") + if [ "$existing" != "0" ]; then + echo "Perf review comment already posted." + exit 0 + fi + { + echo "$marker" + echo "This PR adds Prisma queries or changes a Prisma schema, so @${PERF_REVIEWER} has been requested for a performance review." + echo + echo "The matched lines are in the \`notify-perf\` job summary. Reminder: dashboards and management endpoints should read the daily aggregate tables, since a query against \`LiteLLM_SpendLogs\` scales with request volume and a date-bounded window is not a bounded amount of work." + } > perf_comment.md + gh pr comment "${PR_NUMBER}" --repo "${REPO}" --body-file perf_comment.md diff --git a/scripts/detect_new_db_queries.py b/scripts/detect_new_db_queries.py new file mode 100644 index 00000000000..59bba1eb7a1 --- /dev/null +++ b/scripts/detect_new_db_queries.py @@ -0,0 +1,65 @@ +import json +import re +import sys +from dataclasses import dataclass +from pathlib import PurePosixPath + +from pydantic import BaseModel, TypeAdapter + +RAW_SQL_CALL = re.compile(r"\b(?:query_raw|query_first|execute_raw)\b") +PRISMA_MODEL_CALL = re.compile( + r"\.db\.[A-Za-z_][A-Za-z0-9_]*\.(?:find_many|find_first|find_unique|find_unique_or_raise|count|aggregate|group_by|create_many|update_many|delete_many)\b" +) +SCHEMA_SUFFIX = ".prisma" +IGNORED_PREFIXES = ("tests/", "ui/", "docs/", "litellm/proxy/_experimental/out/") + + +class ChangedFile(BaseModel): + filename: str + patch: str | None = None + + +CHANGED_FILES = TypeAdapter(tuple[ChangedFile, ...]) + + +@dataclass(frozen=True, slots=True) +class Finding: + path: str + detail: str + + +def _added_lines(patch: str) -> tuple[str, ...]: + return tuple(line[1:] for line in patch.splitlines() if line.startswith("+") and not line.startswith("+++")) + + +def _is_ignored(path: str) -> bool: + return path.startswith(IGNORED_PREFIXES) + + +def findings_for_file(changed: ChangedFile) -> tuple[Finding, ...]: + if _is_ignored(changed.filename): + return () + if PurePosixPath(changed.filename).suffix == SCHEMA_SUFFIX: + return (Finding(path=changed.filename, detail="prisma schema changed"),) + if changed.patch is None or PurePosixPath(changed.filename).suffix != ".py": + return () + return tuple( + Finding(path=changed.filename, detail=line.strip()) + for line in _added_lines(changed.patch) + if RAW_SQL_CALL.search(line) or PRISMA_MODEL_CALL.search(line) + ) + + +def detect(files: tuple[ChangedFile, ...]) -> tuple[Finding, ...]: + return tuple(finding for changed in files for finding in findings_for_file(changed)) + + +def main() -> int: + findings = detect(CHANGED_FILES.validate_python(json.load(sys.stdin))) + for finding in findings: + print(f"{finding.path}: {finding.detail}") + return 0 if findings else 1 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/tests/test_litellm/test_detect_new_db_queries.py b/tests/test_litellm/test_detect_new_db_queries.py new file mode 100644 index 00000000000..f6f8cf27e84 --- /dev/null +++ b/tests/test_litellm/test_detect_new_db_queries.py @@ -0,0 +1,102 @@ +"""Tests for scripts/detect_new_db_queries.py. + +The detector's contract is "a PR that adds DB access is flagged": raw SQL and Prisma +model calls on added lines count, removed lines and tests/UI paths do not, and a +schema edit counts on its own. The regression that motivated it is #33978, whose +`prisma_client.db.query_raw` against LiteLLM_SpendLogs shipped without a perf review. +""" + +import importlib.util +from pathlib import Path + +_MODULE_PATH = Path(__file__).resolve().parents[2] / "scripts" / "detect_new_db_queries.py" +_spec = importlib.util.spec_from_file_location("detect_new_db_queries", _MODULE_PATH) +detector = importlib.util.module_from_spec(_spec) +_spec.loader.exec_module(detector) + + +def _changed(filename: str, patch: str | None): + return detector.ChangedFile(filename=filename, patch=patch) + + +def test_flags_added_raw_sql_query(): + findings = detector.detect( + ( + _changed( + "litellm/proxy/management_endpoints/tool_management_endpoints.py", + "@@ -1,0 +1,2 @@\n+ rows = await prisma_client.db.query_raw(\n+ 'SELECT 1'\n", + ), + ) + ) + assert [f.detail for f in findings] == ["rows = await prisma_client.db.query_raw("] + + +def test_flags_added_prisma_model_call(): + findings = detector.detect( + ( + _changed( + "litellm/proxy/spend_tracking/spend_management_endpoints.py", + '@@ -1,0 +1,1 @@\n+ rows = await prisma_client.db.litellm_spendlogs.find_many(where={"a": 1})\n', + ), + ) + ) + assert len(findings) == 1 + + +def test_ignores_removed_and_context_lines(): + findings = detector.detect( + ( + _changed( + "litellm/proxy/db/db_spend_update_writer.py", + "@@ -1,3 +1,1 @@\n- await prisma_client.db.query_raw('SELECT 1')\n await unrelated()\n", + ), + ) + ) + assert findings == () + + +def test_ignores_tests_and_ui_paths(): + patch = "@@ -1,0 +1,1 @@\n+ await prisma_client.db.query_raw('SELECT 1')\n" + findings = detector.detect( + ( + _changed("tests/test_litellm/test_something.py", patch), + _changed("ui/litellm-dashboard/src/thing.py", patch), + ) + ) + assert findings == () + + +def test_flags_schema_change_without_a_patch(): + findings = detector.detect((_changed("litellm/proxy/schema.prisma", None),)) + assert [f.detail for f in findings] == ["prisma schema changed"] + + +def test_ignores_unrelated_python_change(): + findings = detector.detect( + ( + _changed( + "litellm/main.py", + "@@ -1,0 +1,1 @@\n+ response = await client.chat.completions.create(**kwargs)\n", + ), + _changed("README.md", "@@ -1,0 +1,1 @@\n+docs\n"), + ) + ) + assert findings == () + + +def test_main_exit_codes_signal_whether_anything_was_found(monkeypatch, capsys, tmp_path): + flagged = tmp_path / "flagged.json" + flagged.write_text( + '[{"filename": "litellm/proxy/db/x.py",' + ' "patch": "@@ -1,0 +1,1 @@\\n+ await prisma_client.db.query_raw(sql)\\n"}]' + ) + with flagged.open() as handle: + monkeypatch.setattr(detector.sys, "stdin", handle) + assert detector.main() == 0 + assert "litellm/proxy/db/x.py" in capsys.readouterr().out + + clean = tmp_path / "clean.json" + clean.write_text('[{"filename": "README.md", "patch": "@@ -1,0 +1,1 @@\\n+docs\\n"}]') + with clean.open() as handle: + monkeypatch.setattr(detector.sys, "stdin", handle) + assert detector.main() == 1