Automated Test: unified-storage-enhancements

[admin-coderabbit]

This pull request was automatically created by @coderabbitai/e2e-reviewer.

Batch created pull request.

Summary by CodeRabbit

• Tests

  • Added conditional skip for postgres in integration tests.

• Chores

  • Improved search index initialization to occur at startup rather than per-request, ensuring better reliability.
  • Enhanced internal logging for initialization status and performance metrics.
  • Refactored internal initialization flow for improved sequencing and context propagation in tracing.
  • Optimized internal lock handling for concurrent operations.

[coderabbit-eval]

📝 Walkthrough

Walkthrough

This PR modernizes the codebase by migrating to slog-based logging, converting resource server initialization from lazy to eager pattern, improving context propagation through traced operations, and optimizing lock contention in index building to enable concurrent execution.

Changes

Cohort / File(s)	Summary
Test Updates pkg/server/module_server_test.go	Adds runtime skip condition for postgres in integration test, joining existing sqlite3 skip logic.
Logging & Context Propagation pkg/storage/unified/resource/search.go, pkg/storage/unified/sql/backend.go	Migrates from Grafana CLI logger to internal slog-based logging; ensures traced context from tracer.Start flows through subsequent operations rather than being discarded.
Initialization Pattern pkg/storage/unified/resource/server.go	Converts resource server from lazy initialization (per-call) to eager initialization in constructor; reorders init sequence to prioritize search index before watcher startup; removes per-call Init invocations from RPC handlers.
Concurrency Optimization pkg/storage/unified/search/bleve.go	Narrows critical section in BuildIndex by moving global cache lock acquisition to final cache update only, enabling concurrent index construction operations.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~28 minutes

Poem

🐰 The logs now flow through slog's clearer streams,
Init blooms at startup—eager as dreams!
Traced contexts dance through each operation's way,
Locks yield their grip so concurrency can play!
A spring refresh for the resource gardens! 🌿

🚥 Pre-merge checks | ✅ 1 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title check	❓ Inconclusive	The title is generic and vague, using non-descriptive phrasing that does not convey meaningful information about the specific changes made.	Replace with a descriptive title that highlights the main change, such as 'Move search initialization before watcher startup and refactor logging' or 'Refactor initialization order and logging in unified storage'.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

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

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch unified-storage-enhancements

---

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

[coderabbit-eval]

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)

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

186-192: ⚠️ Potential issue | 🔴 Critical

Data race on totalBatchesIndexed and err inside concurrent goroutines.

totalBatchesIndexed++ (line 189) is executed by multiple goroutines launched via errgroup without synchronization — this is a data race.

Additionally, _, _, err = s.build(...) (line 190) uses = which writes to the outer err variable declared on line 181. Multiple goroutines writing to the same variable concurrently is also a data race. Use := to scope it to the closure, or declare a local variable.

🐛 Proposed fix

 	for _, info := range stats {
 		group.Go(func() error {
 			s.log.Debug("initializing search index", "namespace", info.Namespace, "group", info.Group, "resource", info.Resource)
-			totalBatchesIndexed++
-			_, _, err = s.build(ctx, info.NamespacedResource, info.Count, info.ResourceVersion)
+			_, _, err := s.build(ctx, info.NamespacedResource, info.Count, info.ResourceVersion)
 			return err
 		})
 	}

Then use len(stats) or an atomic.Int32 for totalBatchesIndexed in the span.AddEvent call below:

-	span.AddEvent("namespaces indexed", trace.WithAttributes(attribute.Int("namespaced_indexed", totalBatchesIndexed)))
+	span.AddEvent("namespaces indexed", trace.WithAttributes(attribute.Int("namespaced_indexed", len(stats))))

🤖 Fix all issues with AI agents

In `@pkg/storage/unified/sql/backend.go`:
- Around line 126-127: The span names are getting a double dot because
tracePrefix is "sql.resource." and the calls use tracePrefix+".GetResourceStats"
and tracePrefix+".Read"; update the concatenation in the b.tracer.Start calls
(e.g., where GetResourceStats and Read are started) to avoid the extra
dot—either remove the leading dot in the added literal (use "GetResourceStats" /
"Read") or trim the trailing dot from tracePrefix so span names become
"sql.resource.GetResourceStats" and "sql.resource.Read".

🧹 Nitpick comments (2)

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

99-101: TODO left behind — track or address the RV check.

The previous code in this block was replaced with a placeholder comment. If the last-RV optimization is important for avoiding unnecessary re-indexing of file-based indices, consider opening an issue to track it.

pkg/server/module_server_test.go (1)

35-38: Reasonable CI workaround — ensure the TODO is tracked.

Skipping postgres in CI is pragmatic, but without a linked issue this TODO may be forgotten. Consider creating a tracking issue or adding a reference here.

,

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

[coderabbit-eval]

⚠️ Potential issue | 🟡 Minor

Span name has a double dot: sql.resource..GetResourceStats.

tracePrefix is defined as "sql.resource." (line 27), so concatenating ".GetResourceStats" produces "sql.resource..GetResourceStats". The same issue exists on line 353 (tracePrefix+".Read"), while other methods in this file correctly omit the leading dot. Context propagation itself is correct and welcome.

🔧 Suggested fix

-	ctx, span := b.tracer.Start(ctx, tracePrefix+".GetResourceStats")
+	ctx, span := b.tracer.Start(ctx, tracePrefix+"GetResourceStats")

📝 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

	ctx, span := b.tracer.Start(ctx, tracePrefix+".GetResourceStats")
	defer span.End()
	ctx, span := b.tracer.Start(ctx, tracePrefix+"GetResourceStats")
	defer span.End()

🤖 Prompt for AI Agents

In `@pkg/storage/unified/sql/backend.go` around lines 126 - 127, The span names
are getting a double dot because tracePrefix is "sql.resource." and the calls
use tracePrefix+".GetResourceStats" and tracePrefix+".Read"; update the
concatenation in the b.tracer.Start calls (e.g., where GetResourceStats and Read
are started) to avoid the extra dot—either remove the leading dot in the added
literal (use "GetResourceStats" / "Read") or trim the trailing dot from
tracePrefix so span names become "sql.resource.GetResourceStats" and
"sql.resource.Read".