diff --git a/.claude/settings.json b/.claude/settings.json index f838f81..8134b81 100644 --- a/.claude/settings.json +++ b/.claude/settings.json @@ -51,7 +51,8 @@ "Skill(sentry-skills:agents-md)", "Skill(sentry-skills:brand-guidelines)", "Skill(sentry-skills:doc-coauthoring)", - "Skill(sentry-skills:security-review)" + "Skill(sentry-skills:security-review)", + "Skill(sentry-skills:django-perf-review)" ], "deny": [] }, diff --git a/plugins/sentry-skills/skills/claude-settings-audit/SKILL.md b/plugins/sentry-skills/skills/claude-settings-audit/SKILL.md index 8ff42fb..b33ea9d 100644 --- a/plugins/sentry-skills/skills/claude-settings-audit/SKILL.md +++ b/plugins/sentry-skills/skills/claude-settings-audit/SKILL.md @@ -18,28 +18,29 @@ find . -maxdepth 2 \( -name "*.toml" -o -name "*.json" -o -name "*.lock" -o -nam Check for these indicator files: -| Category | Files to Check | -|----------|---------------| -| **Python** | `pyproject.toml`, `setup.py`, `requirements.txt`, `Pipfile`, `poetry.lock`, `uv.lock` | -| **Node.js** | `package.json`, `package-lock.json`, `yarn.lock`, `pnpm-lock.yaml` | -| **Go** | `go.mod`, `go.sum` | -| **Rust** | `Cargo.toml`, `Cargo.lock` | -| **Ruby** | `Gemfile`, `Gemfile.lock` | -| **Java** | `pom.xml`, `build.gradle`, `build.gradle.kts` | -| **Build** | `Makefile`, `Dockerfile`, `docker-compose.yml` | -| **Infra** | `*.tf` files, `kubernetes/`, `helm/` | -| **Monorepo** | `lerna.json`, `nx.json`, `turbo.json`, `pnpm-workspace.yaml` | +| Category | Files to Check | +| ------------ | ------------------------------------------------------------------------------------- | +| **Python** | `pyproject.toml`, `setup.py`, `requirements.txt`, `Pipfile`, `poetry.lock`, `uv.lock` | +| **Node.js** | `package.json`, `package-lock.json`, `yarn.lock`, `pnpm-lock.yaml` | +| **Go** | `go.mod`, `go.sum` | +| **Rust** | `Cargo.toml`, `Cargo.lock` | +| **Ruby** | `Gemfile`, `Gemfile.lock` | +| **Java** | `pom.xml`, `build.gradle`, `build.gradle.kts` | +| **Build** | `Makefile`, `Dockerfile`, `docker-compose.yml` | +| **Infra** | `*.tf` files, `kubernetes/`, `helm/` | +| **Monorepo** | `lerna.json`, `nx.json`, `turbo.json`, `pnpm-workspace.yaml` | ## Phase 2: Detect Services Check for service integrations: -| Service | Detection | -|---------|-----------| +| Service | Detection | +| ---------- | ------------------------------------------------------------------------------- | | **Sentry** | `sentry-sdk` in deps, `@sentry/*` packages, `.sentryclirc`, `sentry.properties` | -| **Linear** | Linear config files, `.linear/` directory | +| **Linear** | Linear config files, `.linear/` directory | Read dependency files to identify frameworks: + - `package.json` → check `dependencies` and `devDependencies` - `pyproject.toml` → check `[project.dependencies]` or `[tool.poetry.dependencies]` - `Gemfile` → check gem names @@ -98,42 +99,42 @@ Only include commands for tools actually detected in the project. #### Python (if any Python files or config detected) -| If Detected | Add These Commands | -|-------------|-------------------| -| Any Python | `python --version`, `python3 --version` | -| `poetry.lock` | `poetry show`, `poetry env info` | -| `uv.lock` | `uv pip list`, `uv tree` | -| `Pipfile.lock` | `pipenv graph` | -| `requirements.txt` (no other lock) | `pip list`, `pip show`, `pip freeze` | +| If Detected | Add These Commands | +| ---------------------------------- | --------------------------------------- | +| Any Python | `python --version`, `python3 --version` | +| `poetry.lock` | `poetry show`, `poetry env info` | +| `uv.lock` | `uv pip list`, `uv tree` | +| `Pipfile.lock` | `pipenv graph` | +| `requirements.txt` (no other lock) | `pip list`, `pip show`, `pip freeze` | #### Node.js (if package.json detected) -| If Detected | Add These Commands | -|-------------|-------------------| -| Any Node.js | `node --version` | -| `pnpm-lock.yaml` | `pnpm list`, `pnpm why` | -| `yarn.lock` | `yarn list`, `yarn info`, `yarn why` | -| `package-lock.json` | `npm list`, `npm view`, `npm outdated` | -| TypeScript (`tsconfig.json`) | `tsc --version` | +| If Detected | Add These Commands | +| ---------------------------- | -------------------------------------- | +| Any Node.js | `node --version` | +| `pnpm-lock.yaml` | `pnpm list`, `pnpm why` | +| `yarn.lock` | `yarn list`, `yarn info`, `yarn why` | +| `package-lock.json` | `npm list`, `npm view`, `npm outdated` | +| TypeScript (`tsconfig.json`) | `tsc --version` | #### Other Languages -| If Detected | Add These Commands | -|-------------|-------------------| -| `go.mod` | `go version`, `go list`, `go mod graph`, `go env` | -| `Cargo.toml` | `rustc --version`, `cargo --version`, `cargo tree`, `cargo metadata` | -| `Gemfile` | `ruby --version`, `bundle list`, `bundle show` | -| `pom.xml` | `java --version`, `mvn --version`, `mvn dependency:tree` | -| `build.gradle` | `java --version`, `gradle --version`, `gradle dependencies` | +| If Detected | Add These Commands | +| -------------- | -------------------------------------------------------------------- | +| `go.mod` | `go version`, `go list`, `go mod graph`, `go env` | +| `Cargo.toml` | `rustc --version`, `cargo --version`, `cargo tree`, `cargo metadata` | +| `Gemfile` | `ruby --version`, `bundle list`, `bundle show` | +| `pom.xml` | `java --version`, `mvn --version`, `mvn dependency:tree` | +| `build.gradle` | `java --version`, `gradle --version`, `gradle dependencies` | #### Build Tools -| If Detected | Add These Commands | -|-------------|-------------------| -| `Dockerfile` | `docker --version`, `docker ps`, `docker images` | -| `docker-compose.yml` | `docker-compose ps`, `docker-compose config` | -| `*.tf` files | `terraform --version`, `terraform providers`, `terraform state list` | -| `Makefile` | `make --version`, `make -n` | +| If Detected | Add These Commands | +| -------------------- | -------------------------------------------------------------------- | +| `Dockerfile` | `docker --version`, `docker ps`, `docker images` | +| `docker-compose.yml` | `docker-compose ps`, `docker-compose config` | +| `*.tf` files | `terraform --version`, `terraform providers`, `terraform state list` | +| `Makefile` | `make --version`, `make -n` | ### Skills (for Sentry Projects) @@ -150,13 +151,15 @@ If this is a Sentry project (or sentry-skills plugin is installed), include: "Skill(sentry-skills:agents-md)", "Skill(sentry-skills:brand-guidelines)", "Skill(sentry-skills:doc-coauthoring)", - "Skill(sentry-skills:security-review)" + "Skill(sentry-skills:security-review)", + "Skill(sentry-skills:django-perf-review)" ] ``` ### WebFetch Domains #### Always Include (Sentry Projects) + ```json [ "WebFetch(domain:docs.sentry.io)", @@ -168,21 +171,21 @@ If this is a Sentry project (or sentry-skills plugin is installed), include: #### Framework-Specific -| If Detected | Add Domains | -|-------------|-------------| -| **Django** | `docs.djangoproject.com` | -| **Flask** | `flask.palletsprojects.com` | -| **FastAPI** | `fastapi.tiangolo.com` | -| **React** | `react.dev` | -| **Next.js** | `nextjs.org` | -| **Vue** | `vuejs.org` | -| **Express** | `expressjs.com` | -| **Rails** | `guides.rubyonrails.org`, `api.rubyonrails.org` | -| **Go** | `pkg.go.dev` | -| **Rust** | `docs.rs`, `doc.rust-lang.org` | -| **Docker** | `docs.docker.com` | -| **Kubernetes** | `kubernetes.io` | -| **Terraform** | `registry.terraform.io` | +| If Detected | Add Domains | +| -------------- | ----------------------------------------------- | +| **Django** | `docs.djangoproject.com` | +| **Flask** | `flask.palletsprojects.com` | +| **FastAPI** | `fastapi.tiangolo.com` | +| **React** | `react.dev` | +| **Next.js** | `nextjs.org` | +| **Vue** | `vuejs.org` | +| **Express** | `expressjs.com` | +| **Rails** | `guides.rubyonrails.org`, `api.rubyonrails.org` | +| **Go** | `pkg.go.dev` | +| **Rust** | `docs.rs`, `doc.rust-lang.org` | +| **Docker** | `docs.docker.com` | +| **Kubernetes** | `kubernetes.io` | +| **Terraform** | `registry.terraform.io` | ### MCP Server Suggestions @@ -195,6 +198,7 @@ cat .mcp.json 2>/dev/null || echo "No existing .mcp.json" #### Sentry MCP (if Sentry SDK detected) Add to `.mcp.json` (replace `{org-slug}` and `{project-slug}` with your Sentry organization and project slugs): + ```json { "mcpServers": { @@ -209,6 +213,7 @@ Add to `.mcp.json` (replace `{org-slug}` and `{project-slug}` with your Sentry o #### Linear MCP (if Linear usage detected) Add to `.mcp.json`: + ```json { "mcpServers": { @@ -239,24 +244,24 @@ Example output structure: ```markdown ## Detected Tech Stack -| Category | Found | -|----------|-------| -| Languages | Python 3.x | -| Package Manager | poetry | -| Frameworks | Django, Celery | -| Services | Sentry | -| Build Tools | Docker, Make | +| Category | Found | +| --------------- | -------------- | +| Languages | Python 3.x | +| Package Manager | poetry | +| Frameworks | Django, Celery | +| Services | Sentry | +| Build Tools | Docker, Make | ## Recommended .claude/settings.json \`\`\`json { - "permissions": { - "allow": [ - // ... grouped by category with comments - ], - "deny": [] - } +"permissions": { +"allow": [ +// ... grouped by category with comments +], +"deny": [] +} } \`\`\` @@ -268,12 +273,14 @@ If you use Sentry or Linear, add the MCP config to `.mcp.json`... ## Important Rules ### What to Include + - Only READ-ONLY commands that cannot modify state - Only tools that are actually used by the project (detected via lock files) - Standard system commands (ls, cat, find, etc.) - The `:*` suffix allows any arguments to the base command ### What to NEVER Include + - **Absolute paths** - Never include user-specific paths like `/home/user/scripts/foo` or `/Users/name/bin/bar` - **Custom scripts** - Never include project scripts that may have side effects (e.g., `./scripts/deploy.sh`) - **Alternative package managers** - If the project uses pnpm, do NOT include npm/yarn commands @@ -283,13 +290,13 @@ If you use Sentry or Linear, add the MCP config to `.mcp.json`... Only include the package manager actually used by the project: -| If Detected | Include | Do NOT Include | -|-------------|---------|----------------| -| `pnpm-lock.yaml` | pnpm commands | npm, yarn | -| `yarn.lock` | yarn commands | npm, pnpm | -| `package-lock.json` | npm commands | yarn, pnpm | -| `poetry.lock` | poetry commands | pip (unless also has requirements.txt) | -| `uv.lock` | uv commands | pip, poetry | -| `Pipfile.lock` | pipenv commands | pip, poetry | +| If Detected | Include | Do NOT Include | +| ------------------- | --------------- | -------------------------------------- | +| `pnpm-lock.yaml` | pnpm commands | npm, yarn | +| `yarn.lock` | yarn commands | npm, pnpm | +| `package-lock.json` | npm commands | yarn, pnpm | +| `poetry.lock` | poetry commands | pip (unless also has requirements.txt) | +| `uv.lock` | uv commands | pip, poetry | +| `Pipfile.lock` | pipenv commands | pip, poetry | If multiple lock files exist, include only the commands for each detected manager. diff --git a/plugins/sentry-skills/skills/django-perf-review/LICENSE b/plugins/sentry-skills/skills/django-perf-review/LICENSE new file mode 100644 index 0000000..5c9a43c --- /dev/null +++ b/plugins/sentry-skills/skills/django-perf-review/LICENSE @@ -0,0 +1,4 @@ +Apache License 2.0 + +This skill contains original content. +See repository root LICENSE for full terms. diff --git a/plugins/sentry-skills/skills/django-perf-review/SKILL.md b/plugins/sentry-skills/skills/django-perf-review/SKILL.md new file mode 100644 index 0000000..3aead71 --- /dev/null +++ b/plugins/sentry-skills/skills/django-perf-review/SKILL.md @@ -0,0 +1,398 @@ +--- +name: django-perf-review +description: Django performance code review. Use when asked to "review Django performance", "find N+1 queries", "optimize Django", "check queryset performance", "database performance", "Django ORM issues", or audit Django code for performance problems. +model: sonnet +allowed-tools: Read Grep Glob Bash Task +license: LICENSE +--- + +# Django Performance Review + +Review Django code for **validated** performance issues. Research the codebase to confirm issues before reporting. Report only what you can prove. + +## Review Approach + +1. **Research first** - Trace data flow, check for existing optimizations, verify data volume +2. **Validate before reporting** - Pattern matching is not validation +3. **Zero findings is acceptable** - Don't manufacture issues to appear thorough +4. **Severity must match impact** - If you catch yourself writing "minor" in a CRITICAL finding, it's not critical. Downgrade or skip it. + +## Impact Categories + +Issues are organized by impact. Focus on CRITICAL and HIGH - these cause real problems at scale. + +| Priority | Category | Impact | +|----------|----------|--------| +| 1 | N+1 Queries | **CRITICAL** - Multiplies with data, causes timeouts | +| 2 | Unbounded Querysets | **CRITICAL** - Memory exhaustion, OOM kills | +| 3 | Missing Indexes | **HIGH** - Full table scans on large tables | +| 4 | Write Loops | **HIGH** - Lock contention, slow requests | +| 5 | Inefficient Patterns | **LOW** - Rarely worth reporting | + +--- + +## Priority 1: N+1 Queries (CRITICAL) + +**Impact:** Each N+1 adds `O(n)` database round trips. 100 rows = 100 extra queries. 10,000 rows = timeout. + +### Rule: Prefetch related data accessed in loops + +Validate by tracing: View → Queryset → Template/Serializer → Loop access + +```python +# PROBLEM: N+1 - each iteration queries profile +def user_list(request): + users = User.objects.all() + return render(request, 'users.html', {'users': users}) + +# Template: +# {% for user in users %} +# {{ user.profile.bio }} ← triggers query per user +# {% endfor %} + +# SOLUTION: Prefetch in view +def user_list(request): + users = User.objects.select_related('profile') + return render(request, 'users.html', {'users': users}) +``` + +### Rule: Prefetch in serializers, not just views + +DRF serializers accessing related fields cause N+1 if queryset isn't optimized. + +```python +# PROBLEM: SerializerMethodField queries per object +class UserSerializer(serializers.ModelSerializer): + order_count = serializers.SerializerMethodField() + + def get_order_count(self, obj): + return obj.orders.count() # ← query per user + +# SOLUTION: Annotate in viewset, access in serializer +class UserViewSet(viewsets.ModelViewSet): + def get_queryset(self): + return User.objects.annotate(order_count=Count('orders')) + +class UserSerializer(serializers.ModelSerializer): + order_count = serializers.IntegerField(read_only=True) +``` + +### Rule: Model properties that query are dangerous in loops + +```python +# PROBLEM: Property triggers query when accessed +class User(models.Model): + @property + def recent_orders(self): + return self.orders.filter(created__gte=last_week)[:5] + +# Used in template loop = N+1 + +# SOLUTION: Use Prefetch with custom queryset, or annotate +``` + +### Validation Checklist for N+1 +- [ ] Traced data flow from view to template/serializer +- [ ] Confirmed related field is accessed inside a loop +- [ ] Searched codebase for existing select_related/prefetch_related +- [ ] Verified table has significant row count (1000+) +- [ ] Confirmed this is a hot path (not admin, not rare action) + +--- + +## Priority 2: Unbounded Querysets (CRITICAL) + +**Impact:** Loading entire tables exhausts memory. Large tables cause OOM kills and worker restarts. + +### Rule: Always paginate list endpoints + +```python +# PROBLEM: No pagination - loads all rows +class UserListView(ListView): + model = User + template_name = 'users.html' + +# SOLUTION: Add pagination +class UserListView(ListView): + model = User + template_name = 'users.html' + paginate_by = 25 +``` + +### Rule: Use iterator() for large batch processing + +```python +# PROBLEM: Loads all objects into memory at once +for user in User.objects.all(): + process(user) + +# SOLUTION: Stream with iterator() +for user in User.objects.iterator(chunk_size=1000): + process(user) +``` + +### Rule: Never call list() on unbounded querysets + +```python +# PROBLEM: Forces full evaluation into memory +all_users = list(User.objects.all()) + +# SOLUTION: Keep as queryset, slice if needed +users = User.objects.all()[:100] +``` + +### Validation Checklist for Unbounded Querysets +- [ ] Table is large (10k+ rows) or will grow unbounded +- [ ] No pagination class, paginate_by, or slicing +- [ ] This runs on user-facing request (not background job with chunking) + +--- + +## Priority 3: Missing Indexes (HIGH) + +**Impact:** Full table scans. Negligible on small tables, catastrophic on large ones. + +### Rule: Index fields used in WHERE clauses on large tables + +```python +# PROBLEM: Filtering on unindexed field +# User.objects.filter(email=email) # full scan if no index + +class User(models.Model): + email = models.EmailField() # ← no db_index + +# SOLUTION: Add index +class User(models.Model): + email = models.EmailField(db_index=True) +``` + +### Rule: Index fields used in ORDER BY on large tables + +```python +# PROBLEM: Sorting requires full scan without index +Order.objects.order_by('-created') + +# SOLUTION: Index the sort field +class Order(models.Model): + created = models.DateTimeField(db_index=True) +``` + +### Rule: Use composite indexes for common query patterns + +```python +class Order(models.Model): + user = models.ForeignKey(User) + status = models.CharField(max_length=20) + created = models.DateTimeField() + + class Meta: + indexes = [ + models.Index(fields=['user', 'status']), # for filter(user=x, status=y) + models.Index(fields=['status', '-created']), # for filter(status=x).order_by('-created') + ] +``` + +### Validation Checklist for Missing Indexes +- [ ] Table has 10k+ rows +- [ ] Field is used in filter() or order_by() on hot path +- [ ] Checked model - no db_index=True or Meta.indexes entry +- [ ] Not a foreign key (already indexed automatically) + +--- + +## Priority 4: Write Loops (HIGH) + +**Impact:** N database writes instead of 1. Lock contention. Slow requests. + +### Rule: Use bulk_create instead of create() in loops + +```python +# PROBLEM: N inserts, N round trips +for item in items: + Model.objects.create(name=item['name']) + +# SOLUTION: Single bulk insert +Model.objects.bulk_create([ + Model(name=item['name']) for item in items +]) +``` + +### Rule: Use update() or bulk_update instead of save() in loops + +```python +# PROBLEM: N updates +for obj in queryset: + obj.status = 'done' + obj.save() + +# SOLUTION A: Single UPDATE statement (same value for all) +queryset.update(status='done') + +# SOLUTION B: bulk_update (different values) +for obj in objects: + obj.status = compute_status(obj) +Model.objects.bulk_update(objects, ['status'], batch_size=500) +``` + +### Rule: Use delete() on queryset, not in loops + +```python +# PROBLEM: N deletes +for obj in queryset: + obj.delete() + +# SOLUTION: Single DELETE +queryset.delete() +``` + +### Validation Checklist for Write Loops +- [ ] Loop iterates over 100+ items (or unbounded) +- [ ] Each iteration calls create(), save(), or delete() +- [ ] This runs on user-facing request (not one-time migration script) + +--- + +## Priority 5: Inefficient Patterns (LOW) + +**Rarely worth reporting.** Include only as minor notes if you're already reporting real issues. + +### Pattern: count() vs exists() + +```python +# Slightly suboptimal +if queryset.count() > 0: + do_thing() + +# Marginally better +if queryset.exists(): + do_thing() +``` + +**Usually skip** - difference is <1ms in most cases. + +### Pattern: len(queryset) vs count() + +```python +# Fetches all rows to count +if len(queryset) > 0: # bad if queryset not yet evaluated + +# Single COUNT query +if queryset.count() > 0: +``` + +**Only flag** if queryset is large and not already evaluated. + +### Pattern: get() in small loops + +```python +# N queries, but if N is small (< 20), often fine +for id in ids: + obj = Model.objects.get(id=id) +``` + +**Only flag** if loop is large or this is in a very hot path. + +--- + +## Validation Requirements + +Before reporting ANY issue: + +1. **Trace the data flow** - Follow queryset from creation to consumption +2. **Search for existing optimizations** - Grep for select_related, prefetch_related, pagination +3. **Verify data volume** - Check if table is actually large +4. **Confirm hot path** - Trace call sites, verify this runs frequently +5. **Rule out mitigations** - Check for caching, rate limiting + +**If you cannot validate all steps, do not report.** + +--- + +## Output Format + +```markdown +## Django Performance Review: [File/Component Name] + +### Summary +Validated issues: X (Y Critical, Z High) + +### Findings + +#### [PERF-001] N+1 Query in UserListView (CRITICAL) +**Location:** `views.py:45` + +**Issue:** Related field `profile` accessed in template loop without prefetch. + +**Validation:** +- Traced: UserListView → users queryset → user_list.html → `{{ user.profile.bio }}` in loop +- Searched codebase: no select_related('profile') found +- User table: 50k+ rows (verified in admin) +- Hot path: linked from homepage navigation + +**Evidence:** +```python +def get_queryset(self): + return User.objects.filter(active=True) # no select_related +``` + +**Fix:** +```python +def get_queryset(self): + return User.objects.filter(active=True).select_related('profile') +``` +``` + +If no issues found: "No performance issues identified after reviewing [files] and validating [what you checked]." + +**Before submitting, sanity check each finding:** +- Does the severity match the actual impact? ("Minor inefficiency" ≠ CRITICAL) +- Is this a real performance issue or just a style preference? +- Would fixing this measurably improve performance? + +If the answer to any is "no" - remove the finding. + +--- + +## What NOT to Report + +- Test files +- Admin-only views +- Management commands +- Migration files +- One-time scripts +- Code behind disabled feature flags +- Tables with <1000 rows that won't grow +- Patterns in cold paths (rarely executed code) +- Micro-optimizations (exists vs count, only/defer without evidence) +- Missing select_related on single object fetches (2 queries vs 1 is not worth reporting) + +### False Positives to Avoid + +**Queryset variable assignment is not an issue:** +```python +# This is FINE - no performance difference +projects_qs = Project.objects.filter(org=org) +projects = list(projects_qs) + +# vs this - identical performance +projects = list(Project.objects.filter(org=org)) +``` +Querysets are lazy. Assigning to a variable doesn't execute anything. + +**Single query patterns are not N+1:** +```python +# This is ONE query, not N+1 +projects = list(Project.objects.filter(org=org)) +``` +N+1 requires a loop that triggers additional queries. A single `list()` call is fine. + +**Missing select_related on single object fetch is not N+1:** +```python +# This is 2 queries, not N+1 - don't report it +state = AutofixState.objects.filter(pr_id=pr_id).first() +project_id = state.request.project_id # second query +``` +N+1 requires a loop. A single object doing 2 queries instead of 1 is a micro-optimization not worth reporting. + +**Style preferences are not performance issues:** +If your only suggestion is "combine these two lines" or "rename this variable" - that's style, not performance. Don't report it.