Unified Storage Performance Optimizations

[ShashankFC]

Test 10nn

Summary by CodeRabbit

• Bug Fixes

  • Fixed context propagation in distributed tracing for API operations.

• Tests

  • Added conditional skip for Postgres-based test cases.

• Chores

  • Optimized search index building with refined resource locking.
  • Added search index metrics tracking and logging.

_{✏️ 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)*

[ShashankFC]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

📝 Walkthrough

Walkthrough

Resource server initialization is moved to constructor with dependencies initialized before runtime. Search index building reduces lock contention by narrowing the critical section. Tracer contexts are properly captured and propagated across Init and GetResourceStats methods. Test skips are added for postgres test environments.

Changes

Cohort / File(s)	Summary
Test Configuration pkg/server/module_server_test.go	Added conditional test skip for postgres with explanation that test fails in Drone CI but works locally
Resource Server Initialization pkg/storage/unified/resource/server.go	NewResourceServer now calls Init during construction and returns error on failure; watcher startup moved after search initialization; per-call Init boilerplate removed from RPC handlers
Search Index Logging & Context pkg/storage/unified/resource/search.go	Tracer context captured and propagated in Init/Build methods; logger import replaced with structured logger from searchSupport instance; added metrics logging for search index initialization
Search Lock Optimization pkg/storage/unified/search/bleve.go	Removed global cache lock around entire BuildIndex operation; added narrower critical section for cache updates only; silenced conditional success log
Tracing Context Propagation pkg/storage/unified/sql/backend.go	GetResourceStats now captures context returned from tracer.Start for proper context propagation in downstream operations

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 Init once, not thrice—we optimize
Locks held brief, contexts take flight
Search awakens before watchers rise
Tracing follows the proper path
Code flows cleaner, faster, and wise 🌿

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Unified Storage Performance Optimizations' directly relates to the main changes in the changeset, which focus on performance improvements through initialization reordering, finer-grained cache locking, and context propagation optimizations.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (2)

pkg/server/module_server_test.go (1)

35-38: Consider tracking the postgres test skip with an issue.

The skip is a reasonable temporary workaround for CI/local environment differences. However, without an issue to track this, the TODO may be forgotten. The message "fails in Drone but works locally" suggests a potential environment configuration difference worth investigating (e.g., connection timeouts, database initialization timing, or network settings).

Would you like me to open a GitHub issue to track fixing this postgres test in Drone CI?

pkg/storage/unified/search/bleve.go (1)

99-101: TODO comment lacks detail on what RV check should accomplish.

The TODO comment "check last RV so we can see if the numbers have changed" is vague. Consider clarifying the intended behavior: should this skip rebuilding if the RV hasn't changed, or log a warning, or something else?

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 871af07 and d762a96.

📒 Files selected for processing (5)

• pkg/server/module_server_test.go
• pkg/storage/unified/resource/search.go
• pkg/storage/unified/resource/server.go
• pkg/storage/unified/search/bleve.go
• pkg/storage/unified/sql/backend.go

🧰 Additional context used

🧬 Code graph analysis (2)

pkg/storage/unified/sql/backend.go (1)

pkg/infra/tracing/tracing.go (1)

• Start (415-417)

pkg/storage/unified/resource/search.go (1)

pkg/infra/tracing/tracing.go (1)

• Start (415-417)

🔇 Additional comments (8)

pkg/storage/unified/sql/backend.go (1)

125-127: LGTM! Correct trace context propagation.

Capturing the context returned from tracer.Start and passing it to b.db.WithTx ensures the span is properly propagated to downstream database operations. This enables end-to-end tracing visibility.

pkg/storage/unified/search/bleve.go (1)

137-139: Good optimization: narrowed critical section for cache update.

Moving the lock acquisition to just before the cache update significantly reduces lock contention during index building. This allows concurrent BuildIndex calls for different keys to execute in parallel.

Note: If two goroutines concurrently build an index for the same key, both will complete their work but only one will persist in the cache (last writer wins). This isn't a correctness issue but could result in duplicate work. If this becomes a concern, consider using a per-key lock or singleflight pattern.

pkg/storage/unified/resource/search.go (4)

173-174: LGTM! Proper trace context propagation in init.

Capturing the context from tracer.Start ensures the span is propagated to GetResourceStats and other downstream calls during initialization.

---

215-219: Good observability addition.

The log statement provides useful initialization metrics (duration and document count) that will help with performance monitoring and debugging.

---

279-281: Consistent logging approach.

Using the structured logger s.log aligns with the rest of the file and provides consistent log formatting.

---

309-311: LGTM! Proper trace context propagation in build.

Same pattern as init() - capturing the context ensures the span flows through to BuildIndex and storage operations.

pkg/storage/unified/resource/server.go (2)

308-311: Correct ordering: search index initialized before watcher.

Starting the watcher after search initialization ensures the search index is ready to process events. This prevents a race condition where events could arrive before the index is ready to handle them.

---

258-264: LGTM! Eager initialization ensures server is fully ready before use.

Moving initialization to the constructor ensures the server is fully operational (storage, search index, watcher) before being returned. Returning nil on initialization failure is the correct approach to prevent callers from using a partially initialized server. All call sites properly handle the error case.

[github-actions]

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!

[github-actions]

This pull request has been automatically closed because it has not had any further activity in the last 2 weeks. Thank you for your contributions!