go mod fixes - updates dashboard migrations#2190
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (13)
📒 Files selected for processing (16)
✅ Files skipped from review due to trivial changes (13)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughBumps many Go module versions and moves expensive dashboard backfill/index work out of migrations into an asynchronous, advisory-locked background routine run after startup. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Startup as Startup/Migrations
participant Store as PostgresLogStore
participant BG as DashboardEnhancer (goroutine)
participant Lock as AdvisoryLock
participant DB as PostgreSQL
Startup->>Store: run migrations (blocking)
Store-->>Startup: migrations complete (returns store)
Store->>BG: spawn ensureDashboardEnhancements() (non-blocking)
BG->>Lock: acquire advisory lock
Lock->>DB: pg_try_advisory_lock/request
DB-->>Lock: grant lock to one node
Lock-->>BG: lock acquired
BG->>DB: UPDATE ... SET cached_read_tokens = ... WHERE cached_read_tokens = 0 AND COALESCE(...) > 0
DB-->>BG: rows updated
BG->>DB: CHECK pg_index.indisvalid for idx_logs_histogram_cover / idx_mcp_logs_histogram_cover
DB-->>BG: validity results
BG->>DB: DROP INDEX CONCURRENTLY IF EXISTS ... (if invalid)
DB-->>BG: index dropped
BG->>DB: CREATE INDEX CONCURRENTLY ... (recreate or create if missing)
DB-->>BG: index created
BG-->>Store: log success or warn on error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
|
Contributor
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Contributor
🧪 Test Suite AvailableThis PR can be tested by a repository admin. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
framework/logstore/migrations.go (1)
1993-2012: Consider usingCREATE INDEX CONCURRENTLYto avoid blocking writes.The index recreation (lines 1997-2004) and MCP index creation (lines 2007-2012) use standard
CREATE INDEXwhich acquires an exclusive lock, blocking writes during the build. On large tables, this could cause write latency spikes.For consistency with
ensureMetadataGINIndexandensurePerformanceIndexes, consider usingCREATE INDEX CONCURRENTLY. This would require dropping the existing index withDROP INDEX CONCURRENTLY IF EXISTSas well.That said, the current approach still achieves the primary goal of not blocking pod startup, and the B-tree index build should be relatively fast compared to GIN indexes.
♻️ Optional: Use CONCURRENTLY for index operations
- if err := db.WithContext(ctx).Exec("DROP INDEX IF EXISTS idx_logs_histogram_cover").Error; err != nil { + if err := db.WithContext(ctx).Exec("DROP INDEX CONCURRENTLY IF EXISTS idx_logs_histogram_cover").Error; err != nil { return fmt.Errorf("failed to drop old covering index: %w", err) } - createLogsIndexSQL := `CREATE INDEX IF NOT EXISTS idx_logs_histogram_cover ON logs( + createLogsIndexSQL := `CREATE INDEX CONCURRENTLY IF NOT EXISTS idx_logs_histogram_cover ON logs( status, timestamp, selected_key_id, virtual_key_id, routing_rule_id, provider, object_type, model, cost, prompt_tokens, completion_tokens, total_tokens, cached_read_tokens )` if err := db.WithContext(ctx).Exec(createLogsIndexSQL).Error; err != nil { return fmt.Errorf("failed to create updated covering index: %w", err) } // Create MCP histogram covering index if missing. - createMCPIndexSQL := `CREATE INDEX IF NOT EXISTS idx_mcp_logs_histogram_cover ON mcp_tool_logs( + createMCPIndexSQL := `CREATE INDEX CONCURRENTLY IF NOT EXISTS idx_mcp_logs_histogram_cover ON mcp_tool_logs( status, timestamp, tool_name, server_label, virtual_key_id, cost )`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/logstore/migrations.go` around lines 1993 - 2012, The index creation/dropping in this migration uses blocking statements; change the DROP and CREATE statements to use CONCURRENTLY (i.e., "DROP INDEX CONCURRENTLY IF EXISTS idx_logs_histogram_cover" and "CREATE INDEX CONCURRENTLY IF NOT EXISTS idx_logs_histogram_cover" and similarly for idx_mcp_logs_histogram_cover) so writes aren't blocked during builds; update the Exec calls in this migration function (the block that constructs createLogsIndexSQL and createMCPIndexSQL and their preceding DROP) and ensure error handling remains the same (keep the same fmt.Errorf messages) while using the CONCURRENTLY variants.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@framework/logstore/migrations.go`:
- Around line 1993-2012: The index creation/dropping in this migration uses
blocking statements; change the DROP and CREATE statements to use CONCURRENTLY
(i.e., "DROP INDEX CONCURRENTLY IF EXISTS idx_logs_histogram_cover" and "CREATE
INDEX CONCURRENTLY IF NOT EXISTS idx_logs_histogram_cover" and similarly for
idx_mcp_logs_histogram_cover) so writes aren't blocked during builds; update the
Exec calls in this migration function (the block that constructs
createLogsIndexSQL and createMCPIndexSQL and their preceding DROP) and ensure
error handling remains the same (keep the same fmt.Errorf messages) while using
the CONCURRENTLY variants.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e6d0c316-1544-4d1c-a490-c5efdcca8af7
⛔ Files ignored due to path filters (13)
cli/go.sumis excluded by!**/*.sumcore/go.sumis excluded by!**/*.sumframework/go.sumis excluded by!**/*.sumplugins/governance/go.sumis excluded by!**/*.sumplugins/jsonparser/go.sumis excluded by!**/*.sumplugins/litellmcompat/go.sumis excluded by!**/*.sumplugins/logging/go.sumis excluded by!**/*.sumplugins/maxim/go.sumis excluded by!**/*.sumplugins/mocker/go.sumis excluded by!**/*.sumplugins/otel/go.sumis excluded by!**/*.sumplugins/semanticcache/go.sumis excluded by!**/*.sumplugins/telemetry/go.sumis excluded by!**/*.sumtransports/go.sumis excluded by!**/*.sum
📒 Files selected for processing (15)
cli/go.modcore/go.modframework/go.modframework/logstore/migrations.goframework/logstore/postgres.goplugins/governance/go.modplugins/jsonparser/go.modplugins/litellmcompat/go.modplugins/logging/go.modplugins/maxim/go.modplugins/mocker/go.modplugins/otel/go.modplugins/semanticcache/go.modplugins/telemetry/go.modtransports/go.mod
akshaydeo
force-pushed
the
03-21-go_mod_fixes_-_updates_dashboard_migrations
branch
from
18846cc to
3d406f8
Compare
18 tasks
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@framework/logstore/migrations.go`:
- Around line 1984-1991: The backfill currently treats cached_read_tokens = 0 as
"not backfilled", which conflicts with 0 being a valid final value; change the
sentinel to NULL (or another distinct marker) so rows that truly have 0 aren't
repeatedly processed: update the backfillSQL variable to filter WHERE
cached_read_tokens IS NULL (instead of = 0) and ensure any schema/default logic
sets the column default to 0 while leaving existing unbackfilled rows as NULL
(or explicitly set NULL for pre-backfill rows), so the Exec in migrations.go
only updates NULL entries and subsequent runs become no-ops.
- Around line 1994-2011: Replace the blocking index statements with concurrent
ones: change the DROP INDEX and CREATE INDEX SQL for idx_logs_histogram_cover
and idx_mcp_logs_histogram_cover (the strings assigned to createLogsIndexSQL and
createMCPIndexSQL and the Exec calls that run them via db.WithContext(ctx).Exec)
to use DROP INDEX CONCURRENTLY and CREATE INDEX CONCURRENTLY; ensure these Exec
calls are not executed inside a transaction (Postgres forbids CONCURRENTLY
inside a transaction), so remove or avoid any surrounding transaction context
for these statements and handle/log any errors as before.
🪄 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: CHILL
Plan: Pro
Run ID: 2402e5cf-5f32-47b1-8cec-74e39c36a23f
⛔ Files ignored due to path filters (13)
cli/go.sumis excluded by!**/*.sumcore/go.sumis excluded by!**/*.sumframework/go.sumis excluded by!**/*.sumplugins/governance/go.sumis excluded by!**/*.sumplugins/jsonparser/go.sumis excluded by!**/*.sumplugins/litellmcompat/go.sumis excluded by!**/*.sumplugins/logging/go.sumis excluded by!**/*.sumplugins/maxim/go.sumis excluded by!**/*.sumplugins/mocker/go.sumis excluded by!**/*.sumplugins/otel/go.sumis excluded by!**/*.sumplugins/semanticcache/go.sumis excluded by!**/*.sumplugins/telemetry/go.sumis excluded by!**/*.sumtransports/go.sumis excluded by!**/*.sum
📒 Files selected for processing (15)
cli/go.modcore/go.modframework/go.modframework/logstore/migrations.goframework/logstore/postgres.goplugins/governance/go.modplugins/jsonparser/go.modplugins/litellmcompat/go.modplugins/logging/go.modplugins/maxim/go.modplugins/mocker/go.modplugins/otel/go.modplugins/semanticcache/go.modplugins/telemetry/go.modtransports/go.mod
✅ Files skipped from review due to trivial changes (12)
- framework/go.mod
- plugins/mocker/go.mod
- plugins/jsonparser/go.mod
- cli/go.mod
- plugins/telemetry/go.mod
- core/go.mod
- plugins/maxim/go.mod
- plugins/logging/go.mod
- plugins/governance/go.mod
- plugins/litellmcompat/go.mod
- plugins/otel/go.mod
- plugins/semanticcache/go.mod
🚧 Files skipped from review as they are similar to previous changes (1)
- transports/go.mod
akshaydeo
force-pushed
the
03-21-go_mod_fixes_-_updates_dashboard_migrations
branch
from
3d406f8 to
5fe923c
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
framework/logstore/migrations.go (1)
1939-1943: Inconsistent argument style forAddColumn.Line 1940 uses the struct field name
"CachedReadTokens"while all other migrations in this file use the snake_case column name (e.g.,"parent_request_id","responses_output","cost"). While GORM accepts either form, using the column name would be consistent with the established pattern.Proposed fix for consistency
if !dbMigrator.HasColumn(&Log{}, "cached_read_tokens") { - if err := dbMigrator.AddColumn(&Log{}, "CachedReadTokens"); err != nil { + if err := dbMigrator.AddColumn(&Log{}, "cached_read_tokens"); err != nil { return fmt.Errorf("failed to add cached_read_tokens column: %w", err) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/logstore/migrations.go` around lines 1939 - 1943, The migration uses AddColumn with the struct field name "CachedReadTokens" which is inconsistent with other migrations that use snake_case column names; update the AddColumn call on dbMigrator for the Log model to use "cached_read_tokens" (i.e., replace "CachedReadTokens" with "cached_read_tokens") so it matches the existing convention used for columns like "parent_request_id" and "responses_output".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@framework/logstore/migrations.go`:
- Around line 1939-1943: The migration uses AddColumn with the struct field name
"CachedReadTokens" which is inconsistent with other migrations that use
snake_case column names; update the AddColumn call on dbMigrator for the Log
model to use "cached_read_tokens" (i.e., replace "CachedReadTokens" with
"cached_read_tokens") so it matches the existing convention used for columns
like "parent_request_id" and "responses_output".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e7129a1d-db31-420b-b6aa-93df7ab2528f
⛔ Files ignored due to path filters (13)
cli/go.sumis excluded by!**/*.sumcore/go.sumis excluded by!**/*.sumframework/go.sumis excluded by!**/*.sumplugins/governance/go.sumis excluded by!**/*.sumplugins/jsonparser/go.sumis excluded by!**/*.sumplugins/litellmcompat/go.sumis excluded by!**/*.sumplugins/logging/go.sumis excluded by!**/*.sumplugins/maxim/go.sumis excluded by!**/*.sumplugins/mocker/go.sumis excluded by!**/*.sumplugins/otel/go.sumis excluded by!**/*.sumplugins/semanticcache/go.sumis excluded by!**/*.sumplugins/telemetry/go.sumis excluded by!**/*.sumtransports/go.sumis excluded by!**/*.sum
📒 Files selected for processing (15)
cli/go.modcore/go.modframework/go.modframework/logstore/migrations.goframework/logstore/postgres.goplugins/governance/go.modplugins/jsonparser/go.modplugins/litellmcompat/go.modplugins/logging/go.modplugins/maxim/go.modplugins/mocker/go.modplugins/otel/go.modplugins/semanticcache/go.modplugins/telemetry/go.modtransports/go.mod
✅ Files skipped from review due to trivial changes (11)
- framework/go.mod
- cli/go.mod
- plugins/jsonparser/go.mod
- plugins/logging/go.mod
- plugins/mocker/go.mod
- plugins/otel/go.mod
- transports/go.mod
- plugins/telemetry/go.mod
- plugins/litellmcompat/go.mod
- core/go.mod
- plugins/governance/go.mod
🚧 Files skipped from review as they are similar to previous changes (2)
- plugins/maxim/go.mod
- plugins/semanticcache/go.mod
akshaydeo
force-pushed
the
03-21-go_mod_fixes_-_updates_dashboard_migrations
branch
from
5fe923c to
da62438
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
framework/logstore/migrations.go (1)
1946-1952:⚠️ Potential issue | 🟡 MinorDon't swallow rollback failures.
This is the only rollback in the surrounding migrations that discards
DropColumnerrors. If the drop fails, the migrator will still report success while the schema is unchanged.Suggested fix
Rollback: func(tx *gorm.DB) error { tx = tx.WithContext(ctx) dbMigrator := tx.Migrator() if dbMigrator.HasColumn(&Log{}, "cached_read_tokens") { - _ = dbMigrator.DropColumn(&Log{}, "cached_read_tokens") + if err := dbMigrator.DropColumn(&Log{}, "cached_read_tokens"); err != nil { + return err + } } return nil },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/logstore/migrations.go` around lines 1946 - 1952, The rollback currently swallows errors from dbMigrator.DropColumn inside the Rollback func (tx *gorm.DB) error, so a failed drop of the "cached_read_tokens" column on Log{} will be ignored; change it to capture the returned error from dbMigrator.DropColumn(&Log{}, "cached_read_tokens") and return that error (or wrap it) instead of discarding it so the migrator reports failure when the drop fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@framework/logstore/migrations.go`:
- Around line 1995-2014: The migration currently always drops/recreates
idx_logs_histogram_cover and blindly runs CREATE INDEX CONCURRENTLY for
idx_mcp_logs_histogram_cover; instead adopt the same pattern used in
ensureMetadataGINIndex/ensurePerformanceIndexes: for each index
(idx_logs_histogram_cover on logs and idx_mcp_logs_histogram_cover on
mcp_tool_logs) query pg_index.indisvalid (joining pg_class/pg_index) to
determine if the index exists and is valid, skip creation when valid, otherwise
DROP INDEX CONCURRENTLY IF EXISTS to remove invalid remnants, then run the
CREATE INDEX CONCURRENTLY SQL; implement this logic around the
createLogsIndexSQL and createMCPIndexSQL blocks so valid indexes are not rebuilt
and interrupted concurrent builds are cleaned before creating.
---
Outside diff comments:
In `@framework/logstore/migrations.go`:
- Around line 1946-1952: The rollback currently swallows errors from
dbMigrator.DropColumn inside the Rollback func (tx *gorm.DB) error, so a failed
drop of the "cached_read_tokens" column on Log{} will be ignored; change it to
capture the returned error from dbMigrator.DropColumn(&Log{},
"cached_read_tokens") and return that error (or wrap it) instead of discarding
it so the migrator reports failure when the drop fails.
🪄 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: CHILL
Plan: Pro
Run ID: c242bd5b-14c6-4e39-b423-50eba617c648
⛔ Files ignored due to path filters (13)
cli/go.sumis excluded by!**/*.sumcore/go.sumis excluded by!**/*.sumframework/go.sumis excluded by!**/*.sumplugins/governance/go.sumis excluded by!**/*.sumplugins/jsonparser/go.sumis excluded by!**/*.sumplugins/litellmcompat/go.sumis excluded by!**/*.sumplugins/logging/go.sumis excluded by!**/*.sumplugins/maxim/go.sumis excluded by!**/*.sumplugins/mocker/go.sumis excluded by!**/*.sumplugins/otel/go.sumis excluded by!**/*.sumplugins/semanticcache/go.sumis excluded by!**/*.sumplugins/telemetry/go.sumis excluded by!**/*.sumtransports/go.sumis excluded by!**/*.sum
📒 Files selected for processing (16)
cli/go.modcore/go.modframework/go.modframework/logstore/migrations.goframework/logstore/postgres.goplugins/governance/go.modplugins/jsonparser/go.modplugins/litellmcompat/go.modplugins/logging/go.modplugins/maxim/go.modplugins/mocker/go.modplugins/otel/go.modplugins/semanticcache/go.modplugins/telemetry/go.modtransports/go.modui/app/workspace/logs/views/logsVolumeChart.tsx
✅ Files skipped from review due to trivial changes (13)
- ui/app/workspace/logs/views/logsVolumeChart.tsx
- framework/go.mod
- cli/go.mod
- plugins/mocker/go.mod
- plugins/jsonparser/go.mod
- core/go.mod
- plugins/maxim/go.mod
- plugins/logging/go.mod
- plugins/semanticcache/go.mod
- plugins/governance/go.mod
- plugins/otel/go.mod
- plugins/telemetry/go.mod
- plugins/litellmcompat/go.mod
🚧 Files skipped from review as they are similar to previous changes (2)
- framework/logstore/postgres.go
- transports/go.mod
akshaydeo
force-pushed
the
03-21-go_mod_fixes_-_updates_dashboard_migrations
branch
from
da62438 to
8c647b1
Compare
Contributor
Author
Merge activity
|
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
2 participants
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.
Summary
Updates Go dependencies across all modules to their latest versions, including golang.org/x packages, Google Cloud libraries, and third-party dependencies like buger/jsonparser and cel.dev/expr.
Changes
Type of change
Affected areas
How to test
Verify dependency updates and migration behavior:
The migration changes are PostgreSQL-specific and designed to be non-blocking for pod startup.
Breaking changes
Related issues
Routine dependency maintenance to keep packages current with security patches and bug fixes.
Security considerations
Dependency updates include security patches from the Go standard library and third-party packages. The migration refactoring improves system stability by preventing long-running database operations from blocking application startup.
Checklist
docs/contributing/README.mdand followed the guidelines