Unified Storage Performance Optimizations - #1
Conversation
…#97529) * dont lazy init unified storage * Inits index when creating new resource server. Fixes trace propagation by passing span ctx. Update some logging. * Use finer grained cache locking when building indexes to speed things up. Locking the whole function was slowing things down. * formatting * linter fix * go mod * make update-workspace * fix workspaces check error * update dependency owner in mod file * wait 1 second before querying metrics * try with big timeout, see if fixes CI. Wont fail locally. * skips postgres integration test. Only fails in drone. Will fix later. * put delay back to 500 ms
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| }() | ||
|
|
||
| end := time.Now().Unix() | ||
| s.log.Info("search index initialized", "duration_secs", end-start, "total_docs", s.search.TotalDocs()) |
There was a problem hiding this comment.
Startup log can trigger map panic
Medium Severity
The new startup log calls s.search.TotalDocs() immediately after launching the watch goroutine. TotalDocs() iterates bleveBackend.cache without locking, so a concurrent index build/write can cause a runtime concurrent map iteration and map write panic in pkg/storage/unified/resource/search.go.
Additional Locations (1)
| if err := s.Init(ctx); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
There was a problem hiding this comment.
Stopped server still serves requests
Medium Severity
Per-request s.Init(ctx) checks were removed from RPC handlers, so s.initErr set by Stop() is no longer enforced. After shutdown begins, methods like Create, Update, and Read can still execute backend operations instead of failing fast.
Additional Locations (2)
|
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 2 weeks if no further activity occurs. Please feel free to give a status update or ping for review. Thank you for your contributions! |
2 participants
Test 10nn
Summary by CodeRabbit
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.
nn---n*Replicated from [ai-code-review-evaluation/grafana-coderabbit#10](https://github.com/ai-code-review-evaluation/grafana-coderabbit/pull/10)*Note
Medium Risk
Changes resource server startup/initialization sequencing and removes lazy init from request paths, which can impact service boot behavior and early request handling if any init assumptions were relied upon.
Overview
Ensures the unified
ResourceServeris fully initialized at construction time by callingInit()fromNewResourceServer, and removes per-requestInit()calls across gRPC handlers; initialization order is adjusted so the search index builds before watch streaming starts.Fixes OpenTelemetry context propagation by consistently using the returned
ctxfromtracer.Startin search (searchSupport.init/build) and SQL backend stats (GetResourceStats). Search indexing now logs init duration/total docs, uses the resource logger for high-latency warnings, and the Bleve backend narrows its cache lock to only protect the final cache write during index build.Integration testing is updated to skip the instrumentation/server test when running against Postgres in CI (Drone).
Written by Cursor Bugbot for commit d762a96. Configure here.