Automated Test: unified-storage-enhancements

[admin-coderabbit]

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

Batch created pull request.

Summary by CodeRabbit

• Bug Fixes

  • Improved server initialization robustness by moving from lazy per-call checks to early startup validation, enabling faster error detection.

• Improvements

  • Enhanced search index building efficiency with concurrent operation support.
  • Strengthened high-latency operation monitoring and visibility in search operations.

• Tests

  • Added conditional test handling for PostgreSQL environments.

[coderabbit-eval]

📝 Walkthrough

Walkthrough

These changes refactor the resource server to perform initialization once at startup rather than lazily on each operation, update tracing context propagation across multiple modules to use context-returning tracer calls, modify search index cache synchronization behavior, and add a PostgreSQL skip to a test.

Changes

Cohort / File(s)	Summary
Server Initialization Refactoring pkg/storage/unified/resource/server.go	Moved initialization from per-call checks in RPC handlers to a single startup phase in NewResourceServer. Removed Init() calls from Create, Update, Delete, Read, List, Watch, Search, History, Origin, IsHealthy, and blob operations. Reordered Init() lifecycle to initialize, start watcher, then initialize search index.
Tracing Context Updates pkg/storage/unified/resource/search.go, pkg/storage/unified/sql/backend.go	Updated tracer.Start() calls to capture and use returned context for downstream operations. Added logging for successful search index initialization and high latency warnings using instance logger.
Search Index Synchronization pkg/storage/unified/search/bleve.go	Removed pre-execution lock on BuildIndex to allow concurrent calls. Added guarded cache write after index construction. Replaced informational log with TODO comment.
Test Updates pkg/server/module_server_test.go	Added conditional skip for PostgreSQL in TestIntegrationWillRunInstrumentationServerWhenTargetHasNoHttpServer test with Drone-local note.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰 At startup we dance, not call by call,
Initializing once before standing tall,
Context flows through traces bright,
Cache unlocked for concurrent might,
A rabbit's refactor done just right! 🌙

🚥 Pre-merge checks | ✅ 1 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title check	❓ Inconclusive	The title 'Automated Test: unified-storage-enhancements' is vague and generic, using non-descriptive terms that don't clearly convey the actual changes made across multiple storage and search modules.	Replace with a specific title that describes the main changes, such as 'Move resource server initialization to startup and refactor search index lifecycle' or 'Consolidate resource server initialization to startup phase'.

✅ 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

🤖 Fix all issues with AI agents

In `@pkg/storage/unified/sql/backend.go`:
- Around line 126-127: The span name concatenation double-dot bug occurs because
tracePrefix is "sql.resource." and the code appends ".GetResourceStats"
producing "sql.resource..GetResourceStats"; update the span creation in
GetResourceStats (tracePrefix+".GetResourceStats") to remove the extra leading
dot by either appending "GetResourceStats" (tracePrefix+"GetResourceStats") or
trimming the trailing dot from tracePrefix before concatenation, and apply the
same fix to the other occurrences that use ".Read" so all span names follow the
same pattern as WriteEvent/Create/Update.

🧹 Nitpick comments (4)

pkg/server/module_server_test.go (1)

35-38: Tracking the TODO to fix this test for PostgreSQL.

With both sqlite3 and postgres skipped, this integration test now only runs against MySQL. Consider creating a tracking issue so this doesn't remain skipped indefinitely.

Would you like me to open an issue to track re-enabling this test for PostgreSQL?

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

186-192: Pre-existing data race on totalBatchesIndexed when initWorkers > 1.

This isn't introduced by this PR, but totalBatchesIndexed++ (line 189) is executed concurrently inside group.Go goroutines without synchronization. If s.initWorkers > 1, this is a data race detectable by -race. Consider using atomic.AddInt64 or moving the count outside the goroutine.

♻️ Suggested fix using atomic

-	totalBatchesIndexed := 0
+	var totalBatchesIndexed atomic.Int64
 	group := errgroup.Group{}
 	group.SetLimit(s.initWorkers)
 ...
 	for _, info := range stats {
 		group.Go(func() error {
 			s.log.Debug("initializing search index", "namespace", info.Namespace, "group", info.Group, "resource", info.Resource)
-			totalBatchesIndexed++
+			totalBatchesIndexed.Add(1)
 			_, _, err = s.build(ctx, info.NamespacedResource, info.Count, info.ResourceVersion)
 			return err
 		})
 	}
 ...
-	span.AddEvent("namespaces indexed", trace.WithAttributes(attribute.Int("namespaced_indexed", totalBatchesIndexed)))
+	span.AddEvent("namespaces indexed", trace.WithAttributes(attribute.Int64("namespaced_indexed", totalBatchesIndexed.Load())))

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

144-154: Pre-existing: TotalDocs iterates b.cache without holding cacheMu.

Now that BuildIndex writes to b.cache under cacheMu, reading it without a lock in TotalDocs is a data race if the two can overlap. Consider adding a read lock here.

🔒 Proposed fix

 func (b *bleveBackend) TotalDocs() int64 {
+	b.cacheMu.RLock()
+	defer b.cacheMu.RUnlock()
 	var totalDocs int64
 	for _, v := range b.cache {

---

99-101: Silenced the log and left a TODO — consider checking the existing index.

If bleve.New(dir, mapper) succeeds on a path that already has data, it creates a new index (overwriting). The TODO about checking last RV to detect changes is worth tracking.

Would you like me to open an issue to track the RV-based skip optimization for file-based indexes?

[coderabbit-eval]

⚠️ Potential issue | 🟡 Minor

Double dot in span name: "sql.resource..GetResourceStats"

tracePrefix is "sql.resource." (line 27), so concatenating ".GetResourceStats" produces "sql.resource..GetResourceStats". Other methods in this file (e.g., WriteEvent, Create, Update) omit the leading dot. This also applies to the pre-existing ".Read" on line 353.

🔧 Proposed fix

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

And separately (pre-existing, line 353):

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

🤖 Prompt for AI Agents

In `@pkg/storage/unified/sql/backend.go` around lines 126 - 127, The span name
concatenation double-dot bug occurs because tracePrefix is "sql.resource." and
the code appends ".GetResourceStats" producing "sql.resource..GetResourceStats";
update the span creation in GetResourceStats (tracePrefix+".GetResourceStats")
to remove the extra leading dot by either appending "GetResourceStats"
(tracePrefix+"GetResourceStats") or trimming the trailing dot from tracePrefix
before concatenation, and apply the same fix to the other occurrences that use
".Read" so all span names follow the same pattern as WriteEvent/Create/Update.