perf(admin): slim audit payloads and gzip admin responses - #603
Merged
Conversation
A 25-row audit list page weighed ~27 MB for agent traffic: every entry shipped its full request body, response body, per-attempt error bodies, and one rewritten body per request revision, uncompressed. The Interactions drawer then requested up to 120 such entries and routinely timed out behind proxies. Four changes, one per bottleneck: - /admin/audit/log entries are slimmed server-side: bodies, attempt error payloads, and revision bodies are stripped; scalar metadata, attempt pips data, and revision savings survive. New bodies_omitted and conversation_payload fields tell the dashboard to lazy-load /admin/audit/detail on row expand (that fetch path already existed) and preserve the Interactions-drawer eligibility signal it previously sniffed from the bodies. - /admin/audit/conversation entries drop attempts, request revisions, and header maps; the drawer builds its transcript solely from id/timestamp/request_body/response_body/error_message. - Admin API responses are gzipped (the live-log SSE stream is exempt, it must flush per event). - New LOGGING_LOG_REVISION_BODIES (logging.log_revision_bodies, default true) lets operators keep revision metadata but skip storing the full rewritten body per compressed request, roughly halving audit storage growth under GoModel Pro token compression. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughAudit logging adds configurable rewritten-body capture, slimmed admin list and conversation payloads, omission metadata, partial conversation results on deadline, dashboard detail fetching, updated API documentation, lookup indexes, and gzip compression for admin routes. ChangesAudit payload handling
Admin response compression
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Dashboard
participant AuditLogAPI
participant AuditProjection
participant AuditDetailAPI
Dashboard->>AuditLogAPI: request audit list
AuditLogAPI->>AuditProjection: slim entries
AuditProjection-->>Dashboard: entries with bodies_omitted
Dashboard->>AuditDetailAPI: fetch omitted payload
AuditDetailAPI-->>Dashboard: full entry and clear omission state
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/server/request_rewrite_test.go (1)
376-458: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConvert the revision body logging matrix to a table-driven test.
TestRequestRewriteMiddlewareRecordsRevisionschecks theLogBodies/LogRevisionBodiescombinations via a sharedrun()helper and duplicate assertions. Represent these cases as table entries with per-case setup/expectations to reduce duplication and make the matrix explicit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/server/request_rewrite_test.go` around lines 376 - 458, Refactor the subtests in TestRequestRewriteMiddlewareRecordsRevisions into a table-driven matrix covering each LogBodies/LogRevisionBodies combination. Keep the shared server/request setup from run, but move case-specific expectations into table fields and execute common revision count, body, size, and detail assertions through one loop, preserving each case’s expected body and metadata behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/server/request_rewrite_test.go`:
- Around line 376-458: Refactor the subtests in
TestRequestRewriteMiddlewareRecordsRevisions into a table-driven matrix covering
each LogBodies/LogRevisionBodies combination. Keep the shared server/request
setup from run, but move case-specific expectations into table fields and
execute common revision count, body, size, and detail assertions through one
loop, preserving each case’s expected body and metadata behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: cbd1bce9-6c94-45b8-afb1-29b7ee3a8eec
⛔ Files ignored due to path filters (2)
internal/admin/dashboard/static/dist/assets/index-Dy9UJDJP.jsis excluded by!**/dist/**internal/admin/dashboard/static/dist/index.htmlis excluded by!**/dist/**
📒 Files selected for processing (19)
cmd/gomodel/docs/docs.goconfig/config.example.yamlconfig/config.goconfig/config_test.goconfig/logging.godocs/advanced/configuration.mdxdocs/openapi.jsoninternal/admin/audit_projection.gointernal/admin/audit_projection_test.gointernal/admin/handler.gointernal/admin/handler_audit.gointernal/admin/handler_test.gointernal/auditlog/auditlog.gointernal/auditlog/factory.gointernal/server/http.gointernal/server/request_rewrite.gointernal/server/request_rewrite_test.goweb/dashboard/src/pages/audit-logs/conversation-helpers.jsweb/dashboard/src/pages/audit-logs/live-logs-logic.js
The /admin/audit/conversation chain lookups run `WHERE <json expr> = ? ORDER BY timestamp ASC LIMIT 1`. With the bare expression indexes, PostgreSQL's planner routinely prefers walking the timestamp index and filtering — detoasting every row's JSON blob and scanning the whole table when the value is rare. On a user deployment one request hung until the fronting proxy returned 502 Bad Gateway. Replace both expression indexes with composite (expression, timestamp) indexes so the filter and the order are satisfied by one index and the timestamp-scan plan is never attractive; the old indexes are dropped at store init. As a safety net for degraded stores, the handler now bounds the whole walk with a 10s deadline. A deadline expiring mid-walk returns the partial thread collected so far (ConversationResult.Truncated) instead of failing; only a timeout before the anchor loads surfaces as a 504. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/auditlog/store_sql.go`:
- Around line 181-191: Update jsonPathIndexes() for both SQLite and PostgreSQL
so each replacement composite index is created before dropping the corresponding
legacy indexes. Preserve the existing index definitions and statement order
otherwise, ensuring NewSQLStore() cannot remove the old lookup indexes before
the replacements are available.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 716a4cee-d576-4741-99a6-e807dc4aa66a
📒 Files selected for processing (7)
cmd/gomodel/docs/docs.godocs/openapi.jsoninternal/admin/handler_audit.gointernal/auditlog/conversation_helpers.gointernal/auditlog/conversation_helpers_test.gointernal/auditlog/reader.gointernal/auditlog/store_sql.go
Comment on lines
+181
to
+191
| `DROP INDEX IF EXISTS idx_audit_response_id`, | ||
| `DROP INDEX IF EXISTS idx_audit_previous_response_id`, | ||
| `CREATE INDEX IF NOT EXISTS idx_audit_response_id_ts ON audit_logs(json_extract(data, '$.response_body.id'), timestamp)`, | ||
| `CREATE INDEX IF NOT EXISTS idx_audit_previous_response_id_ts ON audit_logs(json_extract(data, '$.request_body.previous_response_id'), timestamp)`, | ||
| } | ||
| case sqlx.PostgreSQL: | ||
| return []string{ | ||
| `CREATE INDEX IF NOT EXISTS idx_audit_response_id ON audit_logs((data #>> '{response_body,id}'))`, | ||
| `CREATE INDEX IF NOT EXISTS idx_audit_previous_response_id ON audit_logs((data #>> '{request_body,previous_response_id}'))`, | ||
| `DROP INDEX IF EXISTS idx_audit_response_id`, | ||
| `DROP INDEX IF EXISTS idx_audit_previous_response_id`, | ||
| `CREATE INDEX IF NOT EXISTS idx_audit_response_id_ts ON audit_logs((data #>> '{response_body,id}'), timestamp)`, | ||
| `CREATE INDEX IF NOT EXISTS idx_audit_previous_response_id_ts ON audit_logs((data #>> '{request_body,previous_response_id}'), timestamp)`, |
Contributor
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
rg -n -C 20 '\bjsonPathIndexes\s*\(' --glob '*.go'
rg -n -C 20 '\b(Begin|Commit|Rollback|Exec|MustExec)\b' internal/auditlog --glob '*.go'Repository: ENTERPILOT/GoModel
Length of output: 156
🏁 Script executed:
#!/bin/bash
set -u
echo "== files =="
git ls-files | rg '(^|/)store_sql\.go$|internal/auditlog/.+\.go$' || true
echo "== jsonPathIndexes occurrences =="
rg -n -C 30 'jsonPathIndexes|idx_audit_response_id_ts|idx_audit_previous_response_id_ts|jsonPath' --glob '*.go' . || true
echo "== auditlog outlines =="
for f in $(git ls-files 'internal/auditlog/*.go'); do
echo "--- $f"
wc -l "$f"
ast-grep outline "$f" || true
done
echo "== relevant store_sql slice =="
file=$(git ls-files 'internal/auditlog/store_sql.go' | head -n 1)
if [ -n "${file:-}" ]; then
nl -ba "$file" | sed -n '1,260p'
fiRepository: ENTERPILOT/GoModel
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -u
echo "== sqlx package imports =="
rg -n "'github\.com/jmoiron/sqlx'\"|\"github\.com/jmoiron/sqlx\"" --glob '*.go' . | head -n 100
echo "== sqlx.Exec/Schema/AddColumns definitions if vendored/module cache exists =="
for path in $(go env GOPATH 2>/dev/null | tr ':' '\n'); do
for f in $(find "$path/pkg/mod/github.com/jmoiron/sqlx*" "$path/mod/cache/download/github.com/jmoiron/sqlx*" 2>/dev/null | rg 'sqlx.go$|db.go$|schema.go$' | head -n 20); do
echo "--- $f"
rg -n -C 8 'type DB struct|func .*Schema|func .*AddColumns|func (\.Exec|DB\.Exec)\(' "$f" | head -n 80
done
done
echo "== relevant schema migration execution slice =="
file=$(git ls-files 'internal/auditlog/store_sql.go' | head -n 1)
nl -ba "$file" | sed -n '135,196p'
echo "== reader SQL lookup slice =="
file=$(git ls-files 'internal/auditlog/reader_sql.go' | head -n 1)
nl -ba "$file" | sed -n '219,235p'
echo "== lock/index tests =="
rg -n -C 12 'idx_audit_response_id_ts|idx_audit_previous_response_id_ts|response_id_ts|previous_response_id_ts|idx_audit_response_id|idx_audit_previous_response_id' internal/auditlog --glob '*.go'Repository: ENTERPILOT/GoModel
Length of output: 3390
Create replacement indexes before dropping the legacy indexes.
jsonPathIndexes() drops the existing lookup indexes before creating their composite replacements, and NewSQLStore() executes each statement sequentially with only a warning on failure. If creation fails or the process exits between statements, the conversation lookup can fall back to an unindexed scan instead of using the intended composite indexes. Create both replacement indexes first, then drop the old ones.
Proposed ordering
return []string{
- `DROP INDEX IF EXISTS idx_audit_response_id`,
- `DROP INDEX IF EXISTS idx_audit_previous_response_id`,
`CREATE INDEX IF NOT EXISTS idx_audit_response_id_ts ...`,
`CREATE INDEX IF NOT EXISTS idx_audit_previous_response_id_ts ...`,
+ `DROP INDEX IF EXISTS idx_audit_response_id`,
+ `DROP INDEX IF EXISTS idx_audit_previous_response_id`,
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| `DROP INDEX IF EXISTS idx_audit_response_id`, | |
| `DROP INDEX IF EXISTS idx_audit_previous_response_id`, | |
| `CREATE INDEX IF NOT EXISTS idx_audit_response_id_ts ON audit_logs(json_extract(data, '$.response_body.id'), timestamp)`, | |
| `CREATE INDEX IF NOT EXISTS idx_audit_previous_response_id_ts ON audit_logs(json_extract(data, '$.request_body.previous_response_id'), timestamp)`, | |
| } | |
| case sqlx.PostgreSQL: | |
| return []string{ | |
| `CREATE INDEX IF NOT EXISTS idx_audit_response_id ON audit_logs((data #>> '{response_body,id}'))`, | |
| `CREATE INDEX IF NOT EXISTS idx_audit_previous_response_id ON audit_logs((data #>> '{request_body,previous_response_id}'))`, | |
| `DROP INDEX IF EXISTS idx_audit_response_id`, | |
| `DROP INDEX IF EXISTS idx_audit_previous_response_id`, | |
| `CREATE INDEX IF NOT EXISTS idx_audit_response_id_ts ON audit_logs((data #>> '{response_body,id}'), timestamp)`, | |
| `CREATE INDEX IF NOT EXISTS idx_audit_previous_response_id_ts ON audit_logs((data #>> '{request_body,previous_response_id}'), timestamp)`, | |
| case sqlx.SQLite: | |
| return []string{ | |
| `CREATE INDEX IF NOT EXISTS idx_audit_response_id_ts ON audit_logs(json_extract(data, '$.response_body.id'), timestamp)`, | |
| `CREATE INDEX IF NOT EXISTS idx_audit_previous_response_id_ts ON audit_logs(json_extract(data, '$.request_body.previous_response_id'), timestamp)`, | |
| `DROP INDEX IF EXISTS idx_audit_response_id`, | |
| `DROP INDEX IF EXISTS idx_audit_previous_response_id`, | |
| } | |
| case sqlx.PostgreSQL: | |
| return []string{ | |
| `CREATE INDEX IF NOT EXISTS idx_audit_response_id_ts ON audit_logs((data #>> '{response_body,id}'), timestamp)`, | |
| `CREATE INDEX IF NOT EXISTS idx_audit_previous_response_id_ts ON audit_logs((data #>> '{request_body,previous_response_id}'), timestamp)`, | |
| `DROP INDEX IF EXISTS idx_audit_response_id`, | |
| `DROP INDEX IF EXISTS idx_audit_previous_response_id`, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/auditlog/store_sql.go` around lines 181 - 191, Update
jsonPathIndexes() for both SQLite and PostgreSQL so each replacement composite
index is created before dropping the corresponding legacy indexes. Preserve the
existing index definitions and statement order otherwise, ensuring NewSQLStore()
cannot remove the old lookup indexes before the replacements are available.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
A Pro user reported the audit list call (
/admin/audit/log?days=30&limit=25) taking 14 s / 27 MB, with the Interactions drawer's/admin/audit/conversationstill pending behind it (and eventually surfacing "Unable to load interactions."). Each list entry shipped ~1 MB: full request body + response body + per-attempt upstream error bodies + one full rewritten body per request revision (the Pro compression audit trail effectively doubles request bytes), all uncompressed. The drawer then asked for up to 120 such entries.Changes
1. Slim
/admin/audit/logentries (internal/admin/audit_projection.go)List rows keep scalar metadata, attempt-pip fields, revision savings/detail, headers, and error fields — but drop
request_body,response_body,attempts[].response_body/response_headers, andrequest_revisions[].body. Two new response fields compensate:bodies_omitted— tells the dashboard the full entry lives behindGET /admin/audit/detail(also implies the entry is persisted, not in-flight).conversation_payload— server-computed replacement for the drawer-eligibility sniff the client did on the removed bodies (matters for/v1/messagesand passthrough paths).The dashboard's on-expand detail fetch already existed (
AuditEntryRow→fetchAuditEntryDetail→ merge); the only changes are: the fetch gate honorsbodies_omitted, the detail merge clears it, andhasConversationPayloadaccepts the flag. Live-SSE entries are untouched.2. Slim
/admin/audit/conversationentriesThe drawer builds its transcript from
id,timestamp,request_body,response_body,error_messageonly — so thread entries now dropattempts,request_revisions, and header maps.3. Gzip admin responses (
internal/server/http.go)middleware.Gzipon both admin route groups; the/live/logsSSE stream is exempt (per-event flushing). Audit/usage JSON compresses roughly an order of magnitude.4.
LOGGING_LOG_REVISION_BODIES/logging.log_revision_bodies(defaulttrue)Refines
LOGGING_LOG_BODIES: when disabled, request rewriters (e.g. GoModel Pro token compression) still record revision metadata — rewriter, sizes, tokens saved, change detail — but skip storing the full rewritten body, roughly halving audit storage growth per compressed request. Documented inconfig.example.yamlanddocs/advanced/configuration.mdx.Impact
For the reported workload the list response drops from ~27 MB to tens of KB on the wire (slimming × gzip), and the conversation call sheds the revision/attempt copies plus gzip on what remains. Expanded rows hydrate on demand from
/admin/audit/detail.Tests
audit_projection_test.go(list slimming incl. survivors, body-less entries unmarked, detail keeps full payload, conversation slimming,hasConversationPayloadtable).request_rewrite_test.go; existing revision tests updated for the new config field.TestAuditLog_Successnow asserts slim entries.go test ./...green (72 pkgs), dashboard JS tests 362/362, dist rebuilt and in sync, pre-commit (race, lint, perf guard) all passing.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Documentation
Follow-up: the 502 root cause (PostgreSQL)
The reporting user is on PostgreSQL, and the
/admin/audit/conversationrequest ultimately died as a 502 from the fronting proxy after ~an hour. That pinned the real root cause: the chain lookups runWHERE data #>> '{response_body,id}' = $1 ORDER BY timestamp ASC LIMIT 1, and with the bare expression index Postgres' planner routinely prefers walking the timestamp index and filtering — detoasting every row's ~1 MB JSONB along the way; effectively a full-table scan when the chained value is rare. With no server-side deadline, the request just ran until the proxy gave up.Second commit (
d982710c):(expression, timestamp)indexes replace the bare expression indexes on both dialects (old ones dropped at store init), so the filter and the order come from one index and the timestamp-scan plan is never attractive.ConversationResult.Truncated: true) instead of failing — the drawer renders the turns nearest the anchor. Only a timeout before the anchor loads surfaces as a 504. Postgres honors context cancellation, so the runaway query is actually killed server-side.Note for operators: the first startup after this change builds the two composite indexes; on a large
audit_logstable that is a one-time cost at boot.