feat: add traces all along the migration path #839
Conversation
WalkthroughThe changes refactor how storage drivers are initialized and managed. The legacy Changes
Sequence Diagram(s)sequenceDiagram
participant Cmd as Command
participant WS as withStorageDriver
participant FX as fx.App
participant DSF as systemStoreFactory
participant Driver as Driver
Cmd->>WS: Invoke withStorageDriver(callback)
WS->>FX: Initialize fx with modules (OpenTelemetry, Storage, etc.)
FX->>DSF: Create system store instance
DSF->>Driver: Inject system store into driver
Driver->>WS: Execute callback (upgrade/migration)
WS->>Cmd: Return result/error
sequenceDiagram
participant Caller
participant Tracer as trace.Tracer
participant TraceFunc as Trace
Caller->>TraceFunc: Call Trace with callback
TraceFunc->>Caller: Execute function (e.g., migration logic)
alt Error Occurred
TraceFunc-->>Tracer: Record error via tracer
end
TraceFunc->>Caller: Return result and error status
Possibly related PRs
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #839 +/- ##
==========================================
+ Coverage 82.17% 82.43% +0.25%
==========================================
Files 139 140 +1
Lines 7536 7589 +53
==========================================
+ Hits 6193 6256 +63
+ Misses 1027 1017 -10
Partials 316 316 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
gfyrag
force-pushed
the
feat/upgrade-traces
branch
from
2429ef8 to
b505c60
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
internal/storage/driver/driver_test.go (1)
35-35: Reduced test load from 80 to 30 ledgers.Reducing the test workload is reasonable for faster tests, but consider documenting the rationale for this change.
Consider adding a comment explaining the reason for reducing the number of ledgers in the test:
- const countLedgers = 30 + // Reduced from 80 to 30 for faster test execution while maintaining coverage + const countLedgers = 30internal/storage/system/store.go (1)
147-151: LGTM - Added WithTracer option function.This function allows setting a custom tracer during store initialization.
Consider adding a documentation comment for this exported function to clarify its purpose and usage:
+// WithTracer returns an option that configures the DefaultStore with the specified tracer. +// This allows for custom tracing implementations to be used with the store. func WithTracer(tracer trace.Tracer) Option { return func(d *DefaultStore) { d.tracer = tracer } }cmd/buckets_upgrade.go (1)
37-37: Public helper function
withStorageDriveris a neat wrapper encapsulating driver initialization via Fx. Consider adding a doc comment to clarify its usage for future maintainers.internal/storage/driver/driver.go (1)
82-84: Repeated factory calls
Throughout these lines, the driver repeatedly callsd.systemStoreFactory.Create(d.db)to retrieve the system store. While not incorrect, consider storing the result for reuse when it is tightly scoped to the same operation, which might improve clarity or performance slightly if repeated multiple times in a single method.Also applies to: 101-101, 111-111, 172-172, 176-176, 180-180, 184-184, 193-193, 245-245
internal/storage/driver/module.go (1)
57-59: Tracer for system store
Including a tracer for the system store parallels the ledger store approach. If you aim to collect metrics for system store operations as well, consider similarly adding the meter option.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sumtools/generator/go.modis excluded by!**/*.modtools/generator/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (11)
cmd/buckets_upgrade.go(3 hunks)cmd/root.go(1 hunks)internal/storage/bucket/migrations.go(1 hunks)internal/storage/driver/driver.go(9 hunks)internal/storage/driver/driver_test.go(5 hunks)internal/storage/driver/module.go(1 hunks)internal/storage/ledger/main_test.go(1 hunks)internal/storage/system/factory.go(1 hunks)internal/storage/system/store.go(3 hunks)internal/tracing/tracing.go(1 hunks)test/migrations/upgrade_test.go(2 hunks)
🧰 Additional context used
🧬 Code Definitions (7)
test/migrations/upgrade_test.go (1)
internal/storage/system/factory.go (1)
NewStoreFactory(21-23)
internal/storage/ledger/main_test.go (1)
internal/storage/system/factory.go (1)
NewStoreFactory(21-23)
cmd/root.go (1)
internal/storage/driver/driver.go (1)
Driver(24-32)
internal/storage/system/factory.go (2)
internal/storage/system/store.go (3)
Store(20-31)Option(145-145)New(133-143)internal/storage/driver/driver.go (2)
Option(292-292)New(273-290)
cmd/buckets_upgrade.go (4)
internal/storage/driver/driver.go (2)
Driver(24-32)New(273-290)internal/storage/system/store.go (1)
New(133-143)cmd/root.go (1)
Version(16-16)internal/storage/module.go (2)
NewFXModule(18-74)ModuleConfig(14-16)
internal/storage/system/store.go (4)
internal/tracing/tracing.go (1)
Trace(42-53)internal/storage/ledger/store.go (2)
Option(265-265)WithTracer(273-277)internal/controller/system/controller.go (1)
Option(157-157)internal/storage/bucket/bucket.go (1)
WithTracer(48-52)
internal/storage/driver/module.go (5)
internal/storage/ledger/factory.go (2)
Factory(9-11)NewFactory(18-23)internal/storage/driver/driver.go (3)
Option(292-292)Driver(24-32)New(273-290)internal/storage/system/store.go (3)
Option(145-145)WithTracer(147-151)New(133-143)internal/storage/ledger/store.go (3)
Option(265-265)WithTracer(273-277)New(161-248)internal/storage/system/factory.go (1)
NewStoreFactory(21-23)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Dirty
- GitHub Check: Tests
🔇 Additional comments (41)
internal/storage/ledger/main_test.go (1)
63-63: Good addition of system store factory to driver initialization.The addition of
systemstore.NewStoreFactory()as a parameter to the driver initialization aligns with the factory pattern approach being implemented throughout the codebase. This change ensures that the test setup correctly instantiates the driver with all necessary components.test/migrations/upgrade_test.go (2)
13-13: LGTM: Required import for system store factory.Correctly added the import for the system package that contains the
NewStoreFactorymethod.
73-73: Good integration of system store factory in migration tests.The addition of
system.NewStoreFactory()to the driver initialization ensures that the migration tests are using the same driver configuration as the rest of the codebase, maintaining consistency with the new factory pattern approach.internal/storage/bucket/migrations.go (1)
58-58: Good enhancement to error tracing.Adding
span.RecordError(err)improves observability by recording migration errors to the tracing span without altering the control flow of the function. This change aligns with the PR's goal of adding traces along the migration path.internal/tracing/tracing.go (1)
46-52: Well-implemented error handling in Trace function.The changes to the
Tracefunction significantly improve error handling by:
- Capturing both the return value and error from the provided function
- Recording errors to the trace context when they occur
- Properly propagating both the result and error to the caller
This enhancement provides better observability while maintaining the function's intended behavior.
internal/storage/system/factory.go (5)
7-9: The StoreFactory interface is well-designed and follows good practices.The interface is minimal with a single method, which follows the Interface Segregation Principle and makes implementation straightforward.
11-13: LGTM - Appropriate factory implementation.The
DefaultStoreFactorystruct correctly stores the options that will be passed to the created store instances.
15-17: LGTM - Clean implementation of the Create method.The implementation properly forwards the stored options to the underlying
Newfunction.
19-19: Good practice: Compile-time interface check.This line ensures that
DefaultStoreFactoryimplements theStoreFactoryinterface, catching any implementation issues at compile time.
21-23: LGTM - Factory creation function.The
NewStoreFactoryfunction provides a clean way to create and configure factory instances, which aligns with Go's functional options pattern.internal/storage/driver/driver_test.go (5)
15-15: LGTM - Adding system package import.This import is necessary for using the new
system.NewStoreFactory()function.
27-32: LGTM - Updated driver initialization with factory pattern.The test now uses the new factory pattern for system store creation, which is consistent with the architectural changes.
80-85: LGTM - Consistent driver initialization.The test correctly uses the factory pattern, maintaining consistency with other test cases.
119-124: LGTM - Consistent driver initialization.The test correctly uses the factory pattern, maintaining consistency with other test cases.
141-146: LGTM - Consistent driver initialization.The test correctly uses the factory pattern, maintaining consistency with other test cases.
cmd/root.go (1)
36-42:✅ Verification successful
LGTM - Refactored migration command with helper function.
The code has been refactored to use the
withStorageDriverhelper, which encapsulates driver creation and usage, improving modularity and readability.
🏁 Script executed:
#!/bin/bash # Check the implementation of withStorageDriver to ensure proper error handling rg -A 15 "func withStorageDriver" cmd/Length of output: 960
Code refactoring verified: Helper function usage validated.
The implementation of
withStorageDriverincmd/buckets_upgrade.goconfirms that error handling is properly integrated. The refactored code incmd/root.gouses the helper function appropriately by initializing the driver and managing errors during the bucket upgrade process, thus improving modularity and readability.internal/storage/system/store.go (6)
14-17: LGTM - Added imports for tracing.These imports are necessary for the tracing functionality added to the store.
37-40: LGTM - Added tracer field to DefaultStore.The addition of the tracer field allows the store to perform tracing operations.
121-126: LGTM - Enhanced Migrate method with tracing.The method now wraps the migration operation with tracing, which improves observability and helps with debugging.
133-143: LGTM - Updated New function with options pattern.The
Newfunction now accepts options for configuring the store, following the functional options pattern which is idiomatic in Go.
145-145: LGTM - Added Option type for configuring DefaultStore.This type definition enables the functional options pattern.
153-155: LGTM - Added default options with no-op tracer.Setting a no-op tracer by default ensures that the store has valid tracing behavior even without explicit configuration.
cmd/buckets_upgrade.go (11)
6-7: Good integration of OTLP dependencies
The newly imported OTLP and OTLP traces modules are properly referenced later to enable telemetry.
9-9: Storage module import
This import is essential for the new FX module usage below. The usage looks clean and straightforward.
12-12: Fx import usage
Bringing ingo.uber.org/fxis consistent with the new approach for lifecycle and dependency injection.
21-27: Simplified code by delegating driver creation
Replacing direct driver logic withwithStorageDriverhelps keep the command function concise. The conditionalargs[0] == "*"also cleanly covers upgrading all buckets.
39-39: Logger creation
Instantiating a default logger withservice.IsDebug(cmd)is consistent and clear for debugging.
41-41: Connection options
Fetching connection options from Cobra flags ensures consistent CLI behavior. Make sure these flags are documented in the CLI help.
43-43: Early return on error
Immediately returningerrkeeps the logic simple and prevents further processing.
46-54: Fx application construction
Defining all modules and supplying the logger ties the application dependencies together effectively. Great approach to keep the driver logic consolidated.
56-58: Proper app start error handling
Checkingapp.Startreturn value is essential to catch initialization issues. This is well-handled.
60-62: Deferred app stop
Using a deferred stop ensures that resources are properly released afterfn(d)completes.
64-64: Delegated callback
Returningfn(d)cleanly hands off the initialized driver to the callback logic for further actions.internal/storage/driver/driver.go (7)
8-8: System store import
Introducing thesystemstoreimport aligns with the new factory-based approach.
15-16: Metadata and Postgres imports
Both imports are used properly for constraints resolution and metadata handling.
18-18: System controller import
The reference tosystemcontrolleris consistent with usage in ledger creation and bucket checks.
21-21: Uptrace Bun import
Leveraging Bun for database connectivity is standard throughout this file.
28-28: Added systemStoreFactory field
Opting for asystemStoreFactoryfield helps decouple store creation from the driver implementation.
38-38: Factory usage for system store
Creating the system store viad.systemStoreFactory.Create(d.db)is consistent with the new pattern.
277-285: Extended constructor with systemStoreFactory
ReceivingsystemStoreFactoryin theNewconstructor is a solid step toward more flexible store creation. This design clarifies the driver’s dependencies.internal/storage/driver/module.go (1)
31-46: Parameterized ledgerstore factory
Using a struct with optionalTracerProviderandMeterProvideris a neat approach, keeping the design flexible. The resultingledgerstore.Factoryrespects the presence or absence of instrumentation.
gfyrag
force-pushed
the
feat/upgrade-traces
branch
from
b505c60 to
275d011
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
cmd/buckets_upgrade.go (1)
37-65: New helper function for driver lifecycle
ThewithStorageDriverfunction cleanly encapsulates driver creation, injection, and teardown—improving maintainability. Ensure error handling covers all app start/stop edge cases.You may consider logging the success of app shutdown for full traceability, e.g., after
_ = app.Stop(...).internal/storage/driver/driver.go (1)
196-232: Restructured UpgradeAllBuckets with tracing
Wrapping this logic in a trace (tracing.Trace) and using a worker pool to parallelize bucket upgrades is a sound approach. Watch for partial upgrades if any worker fails repeatedly or ifctxtimes out.Consider exposing a configurable max retry limit or reporting partial success/failure to the caller.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sumtools/generator/go.modis excluded by!**/*.modtools/generator/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (11)
cmd/buckets_upgrade.go(3 hunks)cmd/root.go(1 hunks)internal/storage/bucket/migrations.go(1 hunks)internal/storage/driver/driver.go(8 hunks)internal/storage/driver/driver_test.go(5 hunks)internal/storage/driver/module.go(1 hunks)internal/storage/ledger/main_test.go(1 hunks)internal/storage/system/factory.go(1 hunks)internal/storage/system/store.go(3 hunks)internal/tracing/tracing.go(1 hunks)test/migrations/upgrade_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- internal/storage/ledger/main_test.go
- test/migrations/upgrade_test.go
- internal/tracing/tracing.go
- cmd/root.go
- internal/storage/bucket/migrations.go
- internal/storage/system/store.go
- internal/storage/system/factory.go
🧰 Additional context used
🧬 Code Definitions (3)
internal/storage/driver/driver_test.go (5)
internal/storage/driver/driver.go (1)
New(278-295)internal/storage/ledger/store.go (1)
New(161-248)internal/storage/ledger/factory.go (1)
NewFactory(18-23)internal/storage/bucket/bucket.go (1)
NewDefaultFactory(38-44)internal/storage/system/factory.go (1)
NewStoreFactory(21-23)
cmd/buckets_upgrade.go (3)
internal/storage/driver/driver.go (2)
Driver(27-36)New(278-295)cmd/root.go (1)
Version(16-16)internal/storage/driver/module.go (1)
NewFXModule(26-74)
internal/storage/driver/module.go (5)
internal/storage/ledger/factory.go (2)
Factory(9-11)NewFactory(18-23)internal/storage/driver/driver.go (4)
Option(297-297)WithTracer(311-315)Driver(27-36)New(278-295)internal/storage/system/store.go (3)
Option(145-145)WithTracer(147-151)New(133-143)internal/storage/ledger/store.go (4)
Option(265-265)WithTracer(273-277)WithMeter(267-271)New(161-248)internal/storage/system/factory.go (1)
NewStoreFactory(21-23)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: GoReleaser
- GitHub Check: Tests
🔇 Additional comments (17)
internal/storage/driver/driver_test.go (6)
15-15: Import system package
This newly added import forsystemlooks consistent with the changes that introduce thesystem.NewStoreFactory()usage.
27-32: Constructing driver with system store
The updated constructor call (system.NewStoreFactory()) is consistent with the refactor toward factory-based creation. Ensure all call sites are tested to confirm compatibility.
35-35: Reducing ledger creation count
Reducing the count from 80 to 30 shortens test runtime, but might reduce coverage of concurrency or stress aspects of the test. Confirm that 30 remains sufficient to cover those scenarios.Do you want to verify concurrency edge cases by re-increasing it or by introducing a separate stress test?
80-85: Driver initialization with system factory
Reusing the same updated constructor logic as inTestLedgersCreateis consistent.
119-124: Driver initialization with system factory
No issues. This is a straightforward application of the new factory-based creation.
141-146: Driver usage for metadata deletion
The approach mirrors the other tests. No concerns.cmd/buckets_upgrade.go (2)
6-7: New imports for telemetry and storage
The newly introduced imports (otlp,otlptraces,storage, andfx) align with the refactored approach of using the fx framework and tracing modules.Also applies to: 9-9, 12-12
21-27: Refactored command callback
Delegating bucket upgrade logic towithStorageDriverand checking ifargs[0]is"*"or a specific bucket is straightforward and improves readability.internal/storage/driver/driver.go (7)
8-11: Updated imports for tracing and system store
These newly introduced or modified imports support the new factory-based design and trace integration. No issues noted.Also applies to: 18-19, 21-21, 24-24
31-31: Introduced systemStoreFactory in Driver struct
Storing the factory in the driver struct cleanly centralizes system store creation.
42-42: Using systemStoreFactory in driver methods
Repeated calls tod.systemStoreFactory.Create(d.db)improve consistency and modularity. This pattern is simpler than building system store logic in every method.Also applies to: 88-88, 105-105, 115-115, 176-176, 180-180, 184-184, 188-188, 250-250
234-245: New helper methodupgradeBucket
Extracting upgrade logic intoupgradeBucketclarifies responsibility and fosters reusability. The approach seems consistent with the rest of the driver.
282-283: ExtendedNewconstructor
The newsystemStoreFactoryparameter seamlessly ties into the driver initialization. The usage in test files and command code looks coherent.Also applies to: 289-289
311-316:WithTraceroption
Providing a tracer via an option keeps tracing concerns decoupled. This is consistent with the overall design.
320-320: Default tracer set to noop
Usingnoop.Tracer{}by default is a safe fallback for production builds that don't require explicit tracing.internal/storage/driver/module.go (2)
31-46: Great refactoring to support optional telemetry components!This change improves flexibility by making tracerProvider and meterProvider optional dependencies using the
optional:"true"tag. The conditional logic to check for nil providers before applying them prevents potential nil pointer dereferences, allowing the system to function properly even when telemetry components aren't available.This approach follows fx's recommended pattern for optional dependencies and leads to more maintainable code.
57-61: Good use of factory pattern and explicit tracer namingThe change from direct instantiation to using
systemstore.NewStoreFactoryis consistent with the factory pattern used elsewhere in the codebase. Explicitly naming tracers as "SystemStore" and "StorageDriver" improves observability by making it easier to identify the source of traces in your telemetry system.This approach also simplifies dependency management and aligns with the removal of the meterProvider parameter from the function signature.
gfyrag
force-pushed
the
feat/upgrade-traces
branch
from
275d011 to
e7eb9e2
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cmd/buckets_upgrade.go (1)
37-65: Well-structured helper function with proper resource managementThe
withStorageDriverfunction effectively encapsulates the lifecycle management of the storage driver using fx, which is a good practice. It properly handles initialization, execution, and cleanup.Consider logging any errors that might occur during shutdown instead of silently ignoring them:
defer func() { - _ = app.Stop(cmd.Context()) + if err := app.Stop(cmd.Context()); err != nil { + logger.Errorf("Error shutting down application: %v", err) + } }()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sumtools/generator/go.modis excluded by!**/*.modtools/generator/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (13)
cmd/buckets_upgrade.go(3 hunks)cmd/root.go(1 hunks)internal/storage/bucket/migrations.go(1 hunks)internal/storage/driver/driver.go(8 hunks)internal/storage/driver/driver_test.go(5 hunks)internal/storage/driver/module.go(1 hunks)internal/storage/ledger/main_test.go(1 hunks)internal/storage/system/factory.go(1 hunks)internal/storage/system/store.go(3 hunks)internal/tracing/tracing.go(1 hunks)internal/worker/async_block.go(5 hunks)internal/worker/fx.go(3 hunks)test/migrations/upgrade_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- internal/storage/bucket/migrations.go
- internal/storage/ledger/main_test.go
- cmd/root.go
- internal/tracing/tracing.go
- test/migrations/upgrade_test.go
- internal/storage/system/factory.go
- internal/storage/system/store.go
- internal/storage/driver/driver.go
🧰 Additional context used
🧬 Code Definitions (5)
internal/storage/driver/driver_test.go (5)
internal/storage/driver/driver.go (1)
New(288-305)internal/storage/ledger/store.go (1)
New(161-248)internal/storage/ledger/factory.go (1)
NewFactory(18-23)internal/storage/bucket/bucket.go (1)
NewDefaultFactory(38-44)internal/storage/system/factory.go (1)
NewStoreFactory(21-23)
internal/worker/fx.go (3)
internal/worker/async_block.go (2)
AsyncBlockRunner(27-33)WithTracer(131-135)internal/storage/driver/driver.go (1)
WithTracer(321-325)internal/storage/system/store.go (1)
WithTracer(147-151)
cmd/buckets_upgrade.go (3)
internal/storage/driver/driver.go (2)
Driver(28-37)New(288-305)cmd/root.go (1)
Version(16-16)internal/storage/module.go (2)
NewFXModule(18-74)ModuleConfig(14-16)
internal/storage/driver/module.go (5)
internal/storage/ledger/factory.go (2)
Factory(9-11)NewFactory(18-23)internal/storage/driver/driver.go (3)
Option(307-307)Driver(28-37)New(288-305)internal/storage/system/store.go (2)
Option(145-145)New(133-143)internal/storage/ledger/store.go (2)
Option(265-265)New(161-248)internal/storage/system/factory.go (1)
NewStoreFactory(21-23)
internal/worker/async_block.go (4)
internal/storage/driver/driver.go (2)
Option(307-307)WithTracer(321-325)internal/storage/system/store.go (2)
Option(145-145)WithTracer(147-151)internal/storage/ledger/store.go (2)
Option(265-265)WithTracer(273-277)internal/controller/ledger/controller_default.go (1)
WithTracer(522-526)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: GoReleaser
- GitHub Check: Tests
🔇 Additional comments (21)
internal/worker/fx.go (3)
8-8: Good addition of the trace import.The new import for OpenTelemetry tracing is aligned with the changes introducing tracing in the codebase.
19-23: Clean integration of tracer provider parameter.Injecting
traceProvider trace.TracerProviderhere neatly follows the fx pattern for dependency injection, promoting clarity in how tracing dependencies are provided.
33-33: Effective use of tracer injection into the AsyncBlockRunner.Passing
WithTracer(traceProvider.Tracer("AsyncBlockRunner"))ensures that logs and spans from this runner are easily distinguishable, improving observability.internal/storage/driver/driver_test.go (6)
15-15: System package import is consistent with new store factory usage.The import of
github.com/formancehq/ledger/internal/storage/systemaligns well with the subsequent calls tosystem.NewStoreFactory().
27-31: Appropriate addition of system store factory in TestLedgersCreate.Incorporating
system.NewStoreFactory()in the driver instantiation is consistent with the updated driver constructor, ensuring the system store is properly configured.
35-35: Reduced ledger count from 80 to 30.Using fewer ledgers may accelerate testing but could slightly reduce coverage of edge cases. Confirm this meets test coverage needs.
Please verify whether reducing the iterations still adequately tests concurrency and performance conditions.
80-85: Consistent driver creation in TestLedgersList.Passing
system.NewStoreFactory()here mirrors the pattern in other tests, ensuring uniform usage of the enhanced driver constructor.
119-124: Consistent driver creation in TestLedgerUpdateMetadata.Including
system.NewStoreFactory()ensures the driver is set up with the latest system store integrations for metadata updates.
141-146: Consistent driver creation in TestLedgerDeleteMetadata.Maintaining the same driver initialization pattern with
system.NewStoreFactory()ensures consistency across all ledger tests.internal/worker/async_block.go (7)
16-18: New OpenTelemetry imports.These imports support enhanced observability. Ensure the project’s module and dependency versions align with the required OpenTelemetry releases.
32-32: Introduction oftracerfield.Storing a
trace.TracerinAsyncBlockRunnerallows for improved trace instrumentation without cluttering the method signatures.
76-79: Span creation for therunmethod.Starting a trace span named "Run" effectively captures the lifecycle of the asynchronous loop. The deferred
span.End()ensures the span is closed properly.
101-105: Ledger-level tracing with attributes.Tagging the
RunForLedgerspan withattribute.String("ledger", l.Name)adds important context for debugging ledger-specific operations.
114-127: Enhanced constructor pattern.Accepting variadic
Optionparameters forNewAsyncBlockRunnerimproves extensibility. Future adjustments (e.g., different tracers) can be injected with minimal changes.
129-135: Robust tracer injection withWithTracer.This pattern simplifies the runner configuration while keeping the code highly maintainable.
137-139: Reasonable default tracer configuration.Using a
noop.Tracerby default prevents null references and offers a graceful fallback when no custom tracer is provided.cmd/buckets_upgrade.go (3)
6-7: Good addition of OpenTelemetry for tracingAdding OpenTelemetry tracing and proper dependency injection with fx will improve the observability of the application, especially during migrations which can be critical operations.
Also applies to: 9-9, 12-12
21-27: Clean refactoring of command execution logicThe refactoring to use
withStorageDriverwith a callback function significantly improves the code structure. This pattern centralizes driver initialization and cleanup while keeping the command logic focused and concise.
49-50: OpenTelemetry integration enhances observabilityAdding OpenTelemetry modules with proper service versioning will provide valuable tracing data throughout the migration process, helping with debugging and performance monitoring.
internal/storage/driver/module.go (2)
31-46: Improved dependency management with optional telemetry providersThe refactored code now properly handles optional telemetry providers using fx's dependency injection capabilities. This makes the code more flexible and robust, allowing the application to function even when telemetry providers are not available.
The conditional appending of options based on the availability of TracerProvider and MeterProvider is a good practice that respects the optional nature of these dependencies.
57-61: Well-implemented factory pattern with proper tracingThe use of
systemstore.NewStoreFactorywith a specific tracer name ("SystemStore") is a good architectural choice that:
- Consistently applies the factory pattern across the codebase
- Provides better observability with dedicated tracers for each component
- Makes the tracing data more meaningful and easier to filter/analyze
The explicit naming of tracers ("SystemStore" and "StorageDriver") will make it easier to track operations through the system.
gfyrag
force-pushed
the
feat/upgrade-traces
branch
from
e7eb9e2 to
d34a299
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cmd/buckets_upgrade.go (1)
60-62: Consider logging errors during application cleanup.The error from
app.Stop(cmd.Context())is currently ignored. Consider logging any errors that might occur during cleanup for better diagnostics.defer func() { - _ = app.Stop(cmd.Context()) + if err := app.Stop(cmd.Context()); err != nil { + logger.Errorf("Error stopping application: %v", err) + } }()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sumtools/generator/go.modis excluded by!**/*.modtools/generator/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (13)
cmd/buckets_upgrade.go(3 hunks)cmd/root.go(1 hunks)internal/storage/bucket/migrations.go(1 hunks)internal/storage/driver/driver.go(8 hunks)internal/storage/driver/driver_test.go(5 hunks)internal/storage/driver/module.go(2 hunks)internal/storage/ledger/main_test.go(1 hunks)internal/storage/system/factory.go(1 hunks)internal/storage/system/store.go(3 hunks)internal/tracing/tracing.go(1 hunks)internal/worker/async_block.go(5 hunks)internal/worker/fx.go(3 hunks)test/migrations/upgrade_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- internal/storage/ledger/main_test.go
- internal/storage/bucket/migrations.go
- internal/tracing/tracing.go
- internal/worker/fx.go
- cmd/root.go
- internal/storage/driver/driver_test.go
- internal/worker/async_block.go
- internal/storage/system/store.go
- internal/storage/system/factory.go
- test/migrations/upgrade_test.go
- internal/storage/driver/driver.go
🧰 Additional context used
🧬 Code Definitions (2)
cmd/buckets_upgrade.go (5)
internal/storage/driver/driver.go (2)
Driver(28-37)New(288-305)internal/storage/system/store.go (1)
New(133-143)internal/storage/ledger/store.go (1)
New(161-248)cmd/root.go (1)
Version(16-16)internal/storage/module.go (2)
NewFXModule(18-74)ModuleConfig(14-16)
internal/storage/driver/module.go (4)
internal/storage/ledger/factory.go (2)
Factory(9-11)NewFactory(18-23)internal/storage/driver/driver.go (4)
Option(307-307)WithTracer(321-325)Driver(28-37)New(288-305)internal/storage/system/store.go (3)
Option(145-145)WithTracer(147-151)New(133-143)internal/storage/system/factory.go (1)
NewStoreFactory(21-23)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: GoReleaser
- GitHub Check: Tests
🔇 Additional comments (9)
cmd/buckets_upgrade.go (7)
6-8: Good addition of OpenTelemetry tracing dependencies.The inclusion of OpenTelemetry modules enhances observability by adding tracing capabilities for the bucket upgrade process. This aligns with modern monitoring practices and will help with debugging and performance analysis.
12-12: Good introduction of dependency injection framework.Adding the
fximport introduces a structured dependency injection framework that will help manage component lifecycles and dependencies more efficiently.
21-27: Well-structured command execution with separation of concerns.The refactoring simplifies the command's execution logic by using the new
withStorageDriverhelper, resulting in cleaner code that clearly separates driver initialization from the bucket upgrade logic. The upgrade operation is now handled via a callback function, which is a cleaner pattern.
37-38: Good function signature for the new helper function.The
withStorageDriverfunction has a clear, focused purpose with a well-designed signature that accepts a command and a callback function. This design pattern effectively manages the lifecycle of the storage driver.
39-39: Good logger initialization based on command output.The logger is correctly initialized based on the command's output and debug flags, which is a good practice for command-line tools.
47-54: Well-structured application assembly with fx.The use of
fx.Newwith various modules creates a well-structured application with clear dependencies. The inclusion of OpenTelemetry, database connection, and storage modules provides a robust foundation for the command.
64-64: Clean callback invocation.The callback function is invoked with the driver instance after proper initialization, which is a clean way to structure the code flow and ensure that resources are properly managed.
internal/storage/driver/module.go (2)
32-47: LGTM: Good refactoring of dependency injection.The change to use a struct with
fx.Inand optional parameters improves the flexibility of the codebase. The conditional addition of options based on provider availability is a robust approach.
59-62: Good use of factory pattern with explicit tracer naming.The switch to using
NewStoreFactorywith explicitly named tracers ("SystemStore" and "StorageDriver") improves observability and follows good design practices.
gfyrag
force-pushed
the
feat/upgrade-traces
branch
from
d34a299 to
0b619c4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
cmd/buckets_upgrade.go (2)
4-4:⚠️ Potential issueRemove debugging import before merging.
The
go-spew/spewpackage is a debugging tool that should not be used in production code.- "github.com/davecgh/go-spew/spew"
48-50:⚠️ Potential issueRemove debugging code.
These debug dump statements should be removed before merging to production as they could flood logs and potentially expose sensitive information.
- spew.Dump(os.Environ()) - exporter, _ := cmd.Flags().GetString(otlptraces.OtelTracesExporterFlag) - spew.Dump(exporter)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sumtools/generator/go.modis excluded by!**/*.modtools/generator/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (13)
cmd/buckets_upgrade.go(3 hunks)cmd/root.go(1 hunks)internal/storage/bucket/migrations.go(1 hunks)internal/storage/driver/driver.go(8 hunks)internal/storage/driver/driver_test.go(5 hunks)internal/storage/driver/module.go(1 hunks)internal/storage/ledger/main_test.go(1 hunks)internal/storage/system/factory.go(1 hunks)internal/storage/system/store.go(3 hunks)internal/tracing/tracing.go(1 hunks)internal/worker/async_block.go(5 hunks)internal/worker/fx.go(3 hunks)test/migrations/upgrade_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- internal/storage/ledger/main_test.go
- test/migrations/upgrade_test.go
- cmd/root.go
- internal/worker/fx.go
- internal/tracing/tracing.go
- internal/storage/bucket/migrations.go
- internal/storage/system/factory.go
- internal/storage/system/store.go
- internal/worker/async_block.go
🧰 Additional context used
🧬 Code Definitions (4)
internal/storage/driver/driver_test.go (2)
internal/storage/driver/driver.go (1)
New(285-302)internal/storage/system/factory.go (1)
NewStoreFactory(21-23)
internal/storage/driver/driver.go (3)
internal/storage/system/factory.go (1)
StoreFactory(7-9)internal/tracing/tracing.go (1)
Trace(42-53)internal/storage/system/store.go (2)
New(133-143)Option(145-145)
cmd/buckets_upgrade.go (4)
internal/storage/driver/driver.go (2)
Driver(27-36)New(285-302)internal/storage/system/store.go (1)
New(133-143)cmd/root.go (1)
Version(16-16)internal/storage/driver/module.go (1)
NewFXModule(26-74)
internal/storage/driver/module.go (4)
internal/storage/ledger/factory.go (2)
Factory(9-11)NewFactory(18-23)internal/storage/driver/driver.go (3)
Option(304-304)Driver(27-36)New(285-302)internal/storage/system/store.go (2)
Option(145-145)New(133-143)internal/storage/system/factory.go (1)
NewStoreFactory(21-23)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: GoReleaser
- GitHub Check: Tests
🔇 Additional comments (26)
internal/storage/driver/driver_test.go (6)
15-15: Added import for system package support.The new import is necessary to support the system store factory parameter that's now required by the driver constructor.
27-32: Updated driver constructor to include system store factory.The driver constructor now correctly includes the system store factory as the fourth parameter, which aligns with the updated signature in
driver.go. This change ensures the tests properly instantiate the driver with all required dependencies.
35-35: Reduced the number of test ledgers from 80 to 30.This change reduces the test execution time while still maintaining adequate test coverage. This is a good optimization for test performance.
80-85: Updated driver constructor in TestLedgersList.The driver constructor call has been correctly updated to include the system store factory parameter, maintaining consistency across all test functions.
119-124: Updated driver constructor in TestLedgerUpdateMetadata.Consistently updated the driver constructor to include the system store factory parameter.
141-146: Updated driver constructor in TestLedgerDeleteMetadata.The final test function has also been properly updated to use the new driver constructor signature with the system store factory parameter.
cmd/buckets_upgrade.go (3)
7-14: Added dependencies for telemetry and application lifecycle management.The new imports support OpenTelemetry tracing and the fx dependency injection framework, which will help improve observability throughout the migration process.
23-29: Refactored command execution to use callback pattern.This implementation simplifies the command execution logic by using a callback pattern with the new
withStorageDriverfunction. The logic for handling both all-bucket upgrades and single bucket upgrades is now more concise and follows a consistent error handling pattern.
52-61: Fx application setup for dependency injection.This section sets up the fx application with all necessary dependencies for the storage driver. The use of
fx.Populateto extract the driver instance is a clean pattern for getting the fully configured driver from the dependency container.internal/storage/driver/driver.go (15)
7-12: Added imports for tracing and system store.The new imports support the tracing functionality and the factory pattern for system store creation. This aligns with the goal of improving observability throughout the migration process.
31-32: Updated Driver struct to use system store factory.The
Driverstruct now uses a factory pattern for creating system stores, which improves testability and separation of concerns.
42-42: Replaced direct system store creation with factory method.The
CreateLedgermethod now uses the factory method to create the system store, centralizing the system store creation logic and making it more consistent.
88-88: Updated OpenLedger to use system store factory.Consistently updated to use the factory pattern for system store creation.
105-105: Updated Initialize to use system store factory.The
Initializemethod uses the factory pattern to create a system store before migrating it.
115-115: Updated detectRollbacks to use system store factory.Consistently updated to use the factory pattern for system store creation.
176-176: Updated methods to use system store factory.All methods interacting with the system store now consistently use the factory pattern for system store creation.
Also applies to: 180-180, 184-184, 188-188
196-229: Added tracing to UpgradeAllBuckets method.The refactored
UpgradeAllBucketsmethod now uses thetracing.Tracefunction to wrap its execution with tracing information. This improves observability of the bucket upgrade process, which is critical for monitoring migration performance and troubleshooting issues.
197-197: Updated GetDistinctBuckets to use system store factory.The method continues to use the factory pattern consistently.
211-211: Extracted bucket upgrade logic to a helper method.The bucket upgrade logic is now encapsulated in the
upgradeBuckethelper method, improving code organization and reusability.
234-253: Added helper method with tracing for bucket upgrades.The new
upgradeBuckethelper method implements detailed tracing for each bucket upgrade, creating a new trace span with links to the parent context. This provides fine-grained observability into individual bucket migrations, which will be valuable for diagnosing performance issues or failures.
257-257: Updated HasReachMinimalVersion to use system store factory.Consistently updated to use the factory pattern for system store creation.
289-297: Updated Driver constructor to include system store factory.The constructor now requires a
systemStoreFactoryparameter and initializes the corresponding field in theDriverstruct. This completes the transition to the factory pattern for system store creation.
318-322: Updated WithTracer option.The
WithTraceroption now properly sets thetracerfield on theDriverstruct.
327-327: Updated default options to use noop tracer.The default options now initialize the
tracerfield with a noop tracer, ensuring that tracing is always available even if not configured. This prevents null pointer exceptions when tracing methods are called.internal/storage/driver/module.go (2)
31-46: Refactored ledger store factory provider to support optional dependencies.The provider now uses a struct parameter with the
fx.Intag, allowing for optional dependencies likeTracerProviderandMeterProvider. This improves flexibility by making these dependencies optional, and dynamically configuring the ledger store factory based on what's available.The conditional logic ensures that tracing and metrics options are only added when the corresponding providers are present, preventing potential nil pointer dereferences.
57-60: Integrated system store factory with tracing.Instead of directly creating a system store, the module now provides a system store factory with tracing enabled. This aligns with the updated
Driverconstructor and improves observability by ensuring that system store operations are properly traced.
gfyrag
force-pushed
the
feat/upgrade-traces
branch
from
0b619c4 to
de9f37b
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
cmd/root.go (1)
45-59: Migrate with storage driver setup.The
newMigrationCommandfunction leverageswithStorageDriverto initialize, then upgrade all buckets. Generally, the logic is good and properly checks for initialization errors. However, consider logging success or failure explicitly to aid troubleshooting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sumtools/generator/go.modis excluded by!**/*.modtools/generator/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (14)
cmd/buckets_upgrade.go(3 hunks)cmd/root.go(2 hunks)cmd/serve.go(1 hunks)internal/storage/bucket/migrations.go(1 hunks)internal/storage/driver/driver.go(8 hunks)internal/storage/driver/driver_test.go(5 hunks)internal/storage/driver/module.go(1 hunks)internal/storage/ledger/main_test.go(1 hunks)internal/storage/system/factory.go(1 hunks)internal/storage/system/store.go(3 hunks)internal/tracing/tracing.go(1 hunks)internal/worker/async_block.go(5 hunks)internal/worker/fx.go(3 hunks)test/migrations/upgrade_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- internal/storage/ledger/main_test.go
- internal/tracing/tracing.go
- test/migrations/upgrade_test.go
- internal/worker/fx.go
- internal/storage/system/factory.go
- internal/storage/bucket/migrations.go
- internal/storage/driver/driver_test.go
- internal/storage/system/store.go
- internal/worker/async_block.go
🧰 Additional context used
🧬 Code Definitions (4)
cmd/root.go (2)
cmd/docs.go (1)
NewDocsCommand(5-13)internal/storage/driver/driver.go (1)
Driver(27-36)
internal/storage/driver/module.go (4)
internal/storage/ledger/factory.go (2)
Factory(9-11)NewFactory(18-23)internal/storage/driver/driver.go (3)
Option(304-304)Driver(27-36)New(285-302)internal/storage/ledger/store.go (2)
Option(265-265)New(161-248)internal/storage/system/factory.go (1)
NewStoreFactory(21-23)
internal/storage/driver/driver.go (3)
internal/storage/system/factory.go (1)
StoreFactory(7-9)internal/tracing/tracing.go (1)
Trace(42-53)internal/storage/system/store.go (2)
New(133-143)Option(145-145)
cmd/buckets_upgrade.go (4)
internal/storage/driver/driver.go (2)
Driver(27-36)New(285-302)internal/storage/system/store.go (1)
New(133-143)cmd/root.go (1)
Version(18-18)internal/storage/module.go (1)
ModuleConfig(14-16)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: GoReleaser
- GitHub Check: Tests
🔇 Additional comments (22)
cmd/serve.go (1)
177-177: Add flags for OTLP telemetry integration.This addition cleanly exposes OTLP flags. Ensure that documentation is updated so end users know how to configure these flags.
cmd/root.go (1)
37-37: New migration command registration.Registering the new migration command on the root command looks good. This neatly modularizes migration logic under a dedicated subcommand.
cmd/buckets_upgrade.go (4)
4-14: Added multiple new imports, including debugging and telemetry.
spewis often used for debugging but can expose sensitive data.- OTLP imports align with the new telemetry approach.
osusage is for environment dumping, which can also be risky.
Ensure these remain only in development or are removed from production to avoid unintended data exposure.
23-29: Graceful handling of bucket name wildcard.The logic for upgrading all buckets when the argument is
*and a single bucket otherwise is clear and straightforward. Nice and concise.
62-67: Unaddressed error handling on application stop.The application shutdown error is being ignored. Consider logging or handling this error for better observability. This parallels a past review recommendation regarding debug statements and error handling.
defer func() { - _ = app.Stop(cmd.Context()) + if stopErr := app.Stop(cmd.Context()); stopErr != nil { + logger.Errorf("error stopping Fx application: %v", stopErr) + } }()
39-47: OverallwithStorageDriverstructure.Centralizing driver initialization and teardown simplifies command execution. This also helps ensure resources are released consistently. Apart from the security risk of dumping environment variables, the setup is robust.
Also applies to: 52-61, 70-71
internal/storage/driver/driver.go (14)
8-11: No issues with these new imports.
They appear necessary for tracing and system store functionality.
18-19: No concerns with these additional imports.
They seem valid for metadata, PostgreSQL, system controller, and Bun usage.Also applies to: 21-21, 24-24
31-31: Good addition of systemStoreFactory field.
Centralizing system store creation via a dedicated factory enhances maintainability.
86-86: Creating system store outside a transaction is acceptable for read operations.
105-105: System store migration call is consistent with the new factory usage.
115-115: Detecting rollbacks with a non-transactional system store is fine for read-only checks.
176-176: Updating ledger metadata directly is valid.
If needed, ensure atomic updates by running in a transaction.
180-180: Deleting ledger metadata is appropriately handled here.
184-184: Querying ledgers through systemStoreFactory is properly integrated.
188-188: Opening ledger without transaction is acceptable for reads.
196-229: Parallel upgrade logic is well-structured with label-based re-try.
The approach neatly handles repeated upgrade attempts and respects context cancellation.
234-254: upgradeBucket method is well-encapsulated.
Using Otel tracing to wrap migrations is a solid approach for monitoring.
289-289: systemStoreFactory parameter in the constructor is a good addition.
Centralizing system store creation in the driver constructor simplifies usage.Also applies to: 296-296
318-322: Tracer customizations align with best practices.
Providing a noop tracer as the default is a sensible fallback for non-instrumented environments.Also applies to: 327-327
internal/storage/driver/module.go (2)
31-45: Optional tracer and meter providers are nicely handled.
Dynamically appending options for ledgerstore creation improves modularity.
57-60: New store factories and tracer usage align with the updated driver construction.
This approach cleanly bundles systemstore creation and driver tracing into the FX module.
| @@ -41,7 +39,7 @@ func (d *Driver) CreateLedger(ctx context.Context, l *ledger.Ledger) (*ledgersto | |||
|
|
|||
| var ret *ledgerstore.Store | |||
| err := d.db.RunInTx(ctx, nil, func(ctx context.Context, tx bun.Tx) error { | |||
| systemStore := systemstore.New(tx) | |||
| systemStore := d.systemStoreFactory.Create(d.db) | |||
There was a problem hiding this comment.
Use the same transaction context for systemStore.
Here, the system store is created with d.db instead of tx. This can cause partial writes if bucket creation and ledger creation are rolled back but the system store modifications persist.
Consider the following fix:
- systemStore := d.systemStoreFactory.Create(d.db)
+ systemStore := d.systemStoreFactory.Create(tx)📝 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.
| systemStore := d.systemStoreFactory.Create(d.db) | |
| systemStore := d.systemStoreFactory.Create(tx) |
gfyrag
force-pushed
the
feat/upgrade-traces
branch
2 times, most recently
from
97c071b to
08d8759
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
cmd/root.go (1)
37-39:⚠️ Potential issueDuplicate registration of Docs command.
The
NewDocsCommand()is added twice - once on line 35 and again on line 38.root.AddCommand(newMigrationCommand()) -root.AddCommand(NewDocsCommand())
🧹 Nitpick comments (2)
cmd/buckets_upgrade.go (1)
37-65: Well-designed storage driver management with dependency injection.The new
withStorageDriverfunction:
- Centralizes the setup and teardown of the storage driver
- Uses fx framework for dependency injection
- Properly integrates OpenTelemetry tracing
- Manages application lifecycle correctly
However, error handling for app shutdown could be improved:
defer func() { - _ = app.Stop(cmd.Context()) + if stopErr := app.Stop(cmd.Context()); stopErr != nil { + logger.Errorf("Error stopping application: %v", stopErr) + } }()internal/storage/driver/module.go (1)
57-60: Good implementation of factory pattern and tracingUsing
systemstore.NewStoreFactorywith proper tracing improves modularity and observability. The addition of theWithTraceroption for the StorageDriver itself further enhances tracing capabilities across the application.Consider standardizing tracer names for consistency. You have "store" (line 29, 40), "SystemStore" (line 58), and "StorageDriver" (line 60). A consistent naming pattern (either all PascalCase or all lowercase) would improve maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sumtools/generator/go.modis excluded by!**/*.modtools/generator/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (16)
cmd/buckets_upgrade.go(3 hunks)cmd/root.go(2 hunks)cmd/serve.go(1 hunks)internal/storage/bucket/default_bucket.go(1 hunks)internal/storage/bucket/migrations.go(1 hunks)internal/storage/driver/driver.go(8 hunks)internal/storage/driver/driver_test.go(5 hunks)internal/storage/driver/module.go(1 hunks)internal/storage/ledger/main_test.go(1 hunks)internal/storage/module.go(1 hunks)internal/storage/system/factory.go(1 hunks)internal/storage/system/store.go(3 hunks)internal/tracing/tracing.go(1 hunks)internal/worker/async_block.go(5 hunks)internal/worker/fx.go(3 hunks)test/migrations/upgrade_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- internal/storage/ledger/main_test.go
- test/migrations/upgrade_test.go
- internal/storage/bucket/migrations.go
- cmd/serve.go
- internal/worker/fx.go
- internal/storage/system/factory.go
- internal/tracing/tracing.go
- internal/storage/driver/driver.go
- internal/storage/system/store.go
- internal/worker/async_block.go
- internal/storage/bucket/default_bucket.go
🧰 Additional context used
🧬 Code Definitions (5)
cmd/root.go (2)
cmd/docs.go (1)
NewDocsCommand(5-13)internal/storage/driver/driver.go (1)
Driver(27-36)
internal/storage/module.go (1)
internal/tracing/tracing.go (2)
Trace(42-53)NoResult(55-59)
internal/storage/driver/driver_test.go (5)
internal/storage/driver/driver.go (1)
New(285-302)internal/storage/system/store.go (1)
New(133-143)internal/storage/ledger/factory.go (1)
NewFactory(18-23)internal/storage/bucket/bucket.go (1)
NewDefaultFactory(38-44)internal/storage/system/factory.go (1)
NewStoreFactory(21-23)
cmd/buckets_upgrade.go (4)
internal/storage/driver/driver.go (2)
Driver(27-36)New(285-302)cmd/root.go (1)
Version(18-18)internal/storage/driver/module.go (1)
NewFXModule(26-74)internal/storage/module.go (2)
NewFXModule(20-79)ModuleConfig(16-18)
internal/storage/driver/module.go (5)
internal/storage/ledger/factory.go (2)
Factory(9-11)NewFactory(18-23)internal/storage/driver/driver.go (4)
Option(304-304)WithTracer(318-322)Driver(27-36)New(285-302)internal/storage/system/store.go (3)
Option(145-145)WithTracer(147-151)New(133-143)internal/storage/ledger/store.go (4)
Option(265-265)WithTracer(273-277)WithMeter(267-271)New(161-248)internal/storage/system/factory.go (1)
NewStoreFactory(21-23)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Tests
🔇 Additional comments (12)
internal/storage/module.go (2)
9-10: Good addition of tracing dependencies.The addition of tracing imports is appropriate for implementing observability in the health check functionality.
23-42: Well-structured tracing implementation for health check.The health check function now receives a tracer provider and uses it to create spans around the health check logic. This implementation:
- Maintains the original logic flow
- Adds proper tracing context
- Records errors appropriately within the trace
- Preserves the early return optimization
cmd/root.go (2)
5-7: Appropriate addition of OpenTelemetry dependencies.Adding imports for
otlpandotlptracesis necessary for the tracing integration in the migration command.
45-59: Well-designed migration command with tracing support.The
newMigrationCommandfunction:
- Uses the factory pattern with
bunmigrate.NewDefaultCommand- Leverages the new
withStorageDriverhelper for dependency management- Properly initializes the driver before upgrading buckets
- Adds appropriate OpenTelemetry flags
internal/storage/driver/driver_test.go (4)
15-15: Good addition of the system package import.The import is necessary for using
system.NewStoreFactory()in the driver constructor.
27-32: Updated driver constructor to include system store factory.The driver constructor now accepts the system store factory as a parameter, which matches the updated
Driverstruct that now uses a factory pattern for the system store.
35-35: Test optimization: reduced ledger count.Reducing
countLedgersfrom 80 to 30 is a good optimization that speeds up the test without compromising its effectiveness.
80-85: Consistent application of system store factory across tests.All test cases have been updated to include the system store factory, maintaining consistency across the test suite.
Also applies to: 119-124, 141-146
cmd/buckets_upgrade.go (2)
6-10: Good addition of tracing and updated storage imports.These imports are necessary for integrating OpenTelemetry tracing and using the updated storage module.
21-28: Simplified command execution with improved storage driver management.The refactored
RunEfunction now delegates to the newwithStorageDriverhelper, which:
- Centralizes storage driver lifecycle management
- Simplifies error handling
- Maintains the original upgrade logic for either all buckets or a specific bucket
internal/storage/driver/module.go (2)
31-46: Good restructuring of dependency injection with optional parametersThe changes to the ledgerstore Factory provider improve flexibility by making TracerProvider and MeterProvider optional. The conditional addition of options based on available providers is a solid pattern.
47-62: Removed MeterProvider from the Driver constructorThe MeterProvider parameter has been removed from the Driver constructor function signature, which aligns with the changes in the
Newfunction signature shown in the relevant code snippets. This simplifies the Driver initialization while still providing metrics through the ledgerStoreFactory.
gfyrag
force-pushed
the
feat/upgrade-traces
branch
2 times, most recently
from
6c13009 to
5d6d96f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
cmd/root.go (1)
37-39:⚠️ Potential issueFix duplicate command registration.
The
NewDocsCommand()is being added twice to the root command - once at line 35 and again at line 38.root.AddCommand(newMigrationCommand()) -root.AddCommand(NewDocsCommand())internal/storage/driver/driver.go (1)
42-42:⚠️ Potential issuePotential transaction inconsistency.
The
systemStoreis created withd.dbinstead oftx. This could cause partial writes if bucket creation and ledger creation are rolled back but the system store modifications persist.- systemStore := d.systemStoreFactory.Create(d.db) + systemStore := d.systemStoreFactory.Create(tx)
🧹 Nitpick comments (2)
cmd/buckets_upgrade.go (1)
61-62: Improve error handling in app shutdown.The application shutdown error is being ignored, which might hide important errors during cleanup.
defer func() { - _ = app.Stop(cmd.Context()) + if err := app.Stop(cmd.Context()); err != nil { + logger.Errorf("Error stopping application: %v", err) + } }()internal/storage/driver/module.go (1)
31-46: Improved dependency injection with optional parametersThe refactoring of the ledgerstore factory provider function to use
fx.Inwith optional tags for TracerProvider and MeterProvider adds flexibility to the dependency injection setup. This approach allows these dependencies to be conditionally included, making the system more adaptable.Consider pre-allocating the options slice with a capacity based on potential options:
- options := make([]ledgerstore.Option, 0) + options := make([]ledgerstore.Option, 0, 2) // Pre-allocate for potential tracer and meter options
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sumtools/generator/go.modis excluded by!**/*.modtools/generator/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (16)
cmd/buckets_upgrade.go(3 hunks)cmd/root.go(2 hunks)cmd/serve.go(1 hunks)internal/storage/bucket/default_bucket.go(1 hunks)internal/storage/bucket/migrations.go(1 hunks)internal/storage/driver/driver.go(8 hunks)internal/storage/driver/driver_test.go(5 hunks)internal/storage/driver/module.go(1 hunks)internal/storage/ledger/main_test.go(1 hunks)internal/storage/module.go(1 hunks)internal/storage/system/factory.go(1 hunks)internal/storage/system/store.go(3 hunks)internal/tracing/tracing.go(1 hunks)internal/worker/async_block.go(5 hunks)internal/worker/fx.go(3 hunks)test/migrations/upgrade_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- internal/storage/ledger/main_test.go
- cmd/serve.go
- test/migrations/upgrade_test.go
- internal/storage/bucket/default_bucket.go
- internal/worker/fx.go
- internal/storage/module.go
- internal/tracing/tracing.go
- internal/storage/system/factory.go
- internal/storage/system/store.go
- internal/storage/bucket/migrations.go
- internal/worker/async_block.go
🧰 Additional context used
🧬 Code Definitions (4)
cmd/root.go (2)
cmd/docs.go (1)
NewDocsCommand(5-13)internal/storage/driver/driver.go (1)
Driver(27-36)
internal/storage/driver/driver_test.go (2)
internal/storage/driver/driver.go (1)
New(285-302)internal/storage/system/factory.go (1)
NewStoreFactory(21-23)
cmd/buckets_upgrade.go (4)
internal/storage/driver/driver.go (2)
Driver(27-36)New(285-302)cmd/root.go (1)
Version(18-18)internal/storage/driver/module.go (1)
NewFXModule(26-74)internal/storage/module.go (2)
NewFXModule(20-79)ModuleConfig(16-18)
internal/storage/driver/module.go (4)
internal/storage/ledger/factory.go (2)
Factory(9-11)NewFactory(18-23)internal/storage/driver/driver.go (3)
Option(304-304)Driver(27-36)New(285-302)internal/storage/system/store.go (2)
Option(145-145)New(133-143)internal/storage/system/factory.go (1)
NewStoreFactory(21-23)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: GoReleaser
- GitHub Check: Tests
🔇 Additional comments (12)
cmd/root.go (1)
45-59: Good refactoring of migration command with proper tracing.The migration command has been well structured with:
- Using the new
withStorageDriverhelper for consistent driver management- Clear error handling and resource cleanup
- Integration with OpenTelemetry for tracing
internal/storage/driver/driver_test.go (3)
27-32: Driver instantiation updated to include system store factory.The code correctly updates the driver instantiation to match the new constructor signature that requires a system store factory.
35-35: Test performance optimization.Reducing the number of test ledgers from 80 to 30 will improve test performance while still providing adequate test coverage.
80-85: Consistent driver instantiation pattern applied across all tests.All test cases have been updated to use the same pattern for driver instantiation with the system store factory.
Also applies to: 119-124, 141-146
cmd/buckets_upgrade.go (2)
21-27: Clean refactoring using withStorageDriver helper.The migration command now uses the
withStorageDriverhelper, which simplifies the code and ensures consistent driver initialization and cleanup.
37-65: Well-structured dependency injection with fx framework.The new
withStorageDriverfunction:
- Sets up a logger
- Creates a properly configured application using the fx framework
- Manages the lifecycle of the application and its dependencies
- Ensures proper cleanup with deferred app stopping
This approach centralizes storage driver management and avoids code duplication.
internal/storage/driver/driver.go (5)
27-36: Improved Driver structure with system store factory.The Driver struct now uses a factory pattern for system store creation, which improves modularity and consistency. This allows for better testing and dependency injection.
195-232: Added tracing to bucket upgrade process.OpenTelemetry tracing has been properly integrated into the bucket upgrade process, which will provide better observability of the migration path. The code follows best practices by:
- Creating a trace span for the overall operation
- Properly handling the context and span lifecycle
- Using proper error handling and retries
234-254: Well-implemented helper method with detailed tracing.The new
upgradeBucketmethod:
- Creates a new trace span with appropriate links to the parent context
- Provides detailed logging for monitoring the upgrade process
- Properly handles errors and context propagation
- Uses proper logger context management
This is an excellent implementation of the tracing pattern.
285-302: Updated constructor with improved factory pattern.The constructor has been properly updated to accept the system store factory and apply the options. This pattern allows for better testing and dependency injection.
318-328: Tracing integration with noop default.The code adds proper tracing with sensible defaults:
- New
WithTraceroption to customize tracing- Default option using a noop tracer to ensure safety when tracing is not configured
This approach ensures the code works correctly in all environments while enabling tracing when needed.
internal/storage/driver/module.go (1)
57-61: Good use of factory pattern with appropriate tracer namingThe implementation of the system store factory with a dedicated tracer name improves observability. The use of factory pattern here provides better separation of concerns and aligns with the overall refactoring direction.
The explicit naming of tracers ("SystemStore" and "StorageDriver") will make traces more identifiable and easier to filter in observability tools.
gfyrag
force-pushed
the
feat/upgrade-traces
branch
2 times, most recently
from
c34df58 to
f4ebb55
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
internal/storage/driver/driver.go (1)
257-257:⚠️ Potential issueSame transaction issue in HasReachMinimalVersion method.
Similar to line 42, the system store is created with
d.dbrather than using a transaction context, which could lead to inconsistent state.- systemStore := d.systemStoreFactory.Create(d.db) + // Consider wrapping this in a transaction or ensuring consistent state management
🧹 Nitpick comments (1)
cmd/buckets_upgrade.go (1)
37-65: Well-implemented storage driver lifecycle management.The new
withStorageDriverfunction centralizes the setup and teardown of the storage driver using the fx framework, which simplifies command execution and ensures proper resource cleanup.However, error handling for application shutdown could be improved:
defer func() { - _ = app.Stop(cmd.Context()) + if stopErr := app.Stop(cmd.Context()); stopErr != nil { + logger.Errorf("Error stopping application: %v", stopErr) + } }()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sumtools/generator/go.modis excluded by!**/*.modtools/generator/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (19)
cmd/buckets_upgrade.go(3 hunks)cmd/root.go(2 hunks)cmd/serve.go(1 hunks)internal/api/bulking/bulker.go(2 hunks)internal/api/common/errors.go(2 hunks)internal/api/router.go(2 hunks)internal/storage/bucket/default_bucket.go(1 hunks)internal/storage/bucket/migrations.go(2 hunks)internal/storage/driver/driver.go(8 hunks)internal/storage/driver/driver_test.go(5 hunks)internal/storage/driver/module.go(1 hunks)internal/storage/ledger/main_test.go(1 hunks)internal/storage/module.go(1 hunks)internal/storage/system/factory.go(1 hunks)internal/storage/system/store.go(3 hunks)internal/tracing/tracing.go(3 hunks)internal/worker/async_block.go(5 hunks)internal/worker/fx.go(3 hunks)test/migrations/upgrade_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (15)
- internal/storage/ledger/main_test.go
- test/migrations/upgrade_test.go
- internal/api/router.go
- internal/api/common/errors.go
- internal/storage/bucket/default_bucket.go
- cmd/root.go
- internal/worker/fx.go
- cmd/serve.go
- internal/storage/bucket/migrations.go
- internal/api/bulking/bulker.go
- internal/worker/async_block.go
- internal/storage/driver/driver_test.go
- internal/storage/system/store.go
- internal/tracing/tracing.go
- internal/storage/system/factory.go
🧰 Additional context used
🧬 Code Definitions (3)
internal/storage/module.go (1)
internal/tracing/tracing.go (2)
Trace(43-54)NoResult(56-60)
internal/storage/driver/module.go (4)
internal/storage/ledger/factory.go (2)
Factory(9-11)NewFactory(18-23)internal/storage/driver/driver.go (4)
Option(304-304)WithTracer(318-322)Driver(27-36)New(285-302)internal/storage/ledger/store.go (4)
Option(265-265)WithTracer(273-277)WithMeter(267-271)New(161-248)internal/storage/system/factory.go (1)
NewStoreFactory(21-23)
cmd/buckets_upgrade.go (4)
internal/storage/driver/driver.go (2)
Driver(27-36)New(285-302)cmd/root.go (1)
Version(18-18)internal/storage/driver/module.go (1)
NewFXModule(26-74)internal/storage/module.go (2)
NewFXModule(20-79)ModuleConfig(16-18)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: GoReleaser
- GitHub Check: Tests
🔇 Additional comments (13)
internal/storage/driver/driver.go (7)
42-42: Use the same transaction context for systemStore.Here, the system store is created with
d.dbinstead oftx. This can cause partial writes if bucket creation and ledger creation are rolled back but the system store modifications persist.Consider the following fix:
- systemStore := d.systemStoreFactory.Create(d.db) + systemStore := d.systemStoreFactory.Create(tx)
8-11: Good addition of trace imports.Adding proper imports for tracing functionality aligns with the PR objective of adding traces along the migration path.
31-32: Good refactor using factory pattern for system store.Adding the
systemStoreFactoryfield and removing themeterfield improves modularity and follows a consistent factory pattern throughout the codebase.
196-231: Excellent addition of tracing to UpgradeAllBuckets.The tracing wrapper provides better observability of the bucket upgrade process, which is a critical operation. The code is well-structured and ensures proper span management.
234-254: Well-implemented upgradeBucket helper method with tracing.This new helper method properly encapsulates the bucket upgrade logic with trace spans. The use of
trace.WithNewRoot()andtrace.WithLinks()ensures proper trace context propagation while allowing independent tracking of each bucket upgrade.
289-297: Constructor properly updated for systemStoreFactory.The
Newfunction has been updated to accept the new systemStoreFactory parameter and initialize it in the Driver struct.
318-328: Good default tracer configuration.Adding a noop tracer as the default option is a good practice to ensure tracing is always available even when not explicitly configured.
cmd/buckets_upgrade.go (2)
20-28: Clean refactor of command execution logic.The RunE function has been simplified to use the new
withStorageDriverhelper, making the code more readable and focused on its core functionality.
46-55: Good use of FX framework for dependency injection.Utilizing the fx framework for setting up the storage driver and its dependencies improves modularity and testability.
internal/storage/module.go (2)
9-10: Appropriate import of tracing packages.Adding imports for tracing functionality is consistent with the PR objective of adding traces along the migration path.
23-42: Enhanced health check with proper tracing.The health check function has been improved with tracing capabilities by:
- Adding a
tracerparameter to the health check provider- Wrapping the health check logic with
tracing.Traceandtracing.NoResult- Maintaining the same core functionality while improving observability
This change aligns well with the PR objective of adding traces throughout the codebase.
internal/storage/driver/module.go (2)
31-46: Improved dependency injection with optional parameters.Converting direct parameter passing to a struct with optional fields using
fx.Inis a good practice that:
- Makes the code more flexible and maintainable
- Clearly documents which dependencies are optional
- Allows for graceful degradation when dependencies are missing
The conditional addition of options based on available providers is also well implemented.
57-60: Good implementation of system store factory with tracing.The use of
systemstore.NewStoreFactorywith proper tracer configuration enhances observability while maintaining a clean architecture.
gfyrag
force-pushed
the
feat/upgrade-traces
branch
2 times, most recently
from
fdce779 to
7cae448
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
internal/storage/driver/driver.go (1)
42-42:⚠️ Potential issueUse the same transaction context for systemStore.
Here, the system store is created with
d.dbinstead oftx. This can cause partial writes if bucket creation and ledger creation are rolled back but the system store modifications persist.Consider the following fix:
- systemStore := d.systemStoreFactory.Create(d.db) + systemStore := d.systemStoreFactory.Create(tx)
🧹 Nitpick comments (1)
internal/storage/driver/module.go (1)
57-60: Good implementation of factory pattern with tracing, but consider adding metrics too.The use of
systemstore.NewStoreFactorywith specific tracer names improves observability. However, while you've added tracing for theSystemStore, you might want to consider also adding metrics for consistency with the ledger store implementation (lines 42-44).systemstore.NewStoreFactory(systemstore.WithTracer( tracerProvider.Tracer("SystemStore"), +)), systemstore.WithMeter(meterProvider.Meter("SystemStore"))),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sumtools/generator/go.modis excluded by!**/*.modtools/generator/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (19)
cmd/buckets_upgrade.go(3 hunks)cmd/root.go(2 hunks)cmd/serve.go(1 hunks)internal/api/bulking/bulker.go(2 hunks)internal/api/common/errors.go(2 hunks)internal/api/router.go(2 hunks)internal/storage/bucket/default_bucket.go(1 hunks)internal/storage/bucket/migrations.go(2 hunks)internal/storage/driver/driver.go(8 hunks)internal/storage/driver/driver_test.go(5 hunks)internal/storage/driver/module.go(1 hunks)internal/storage/ledger/main_test.go(1 hunks)internal/storage/module.go(1 hunks)internal/storage/system/factory.go(1 hunks)internal/storage/system/store.go(3 hunks)internal/tracing/tracing.go(3 hunks)internal/worker/async_block.go(5 hunks)internal/worker/fx.go(3 hunks)test/migrations/upgrade_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (14)
- internal/storage/ledger/main_test.go
- internal/worker/fx.go
- cmd/serve.go
- internal/api/bulking/bulker.go
- test/migrations/upgrade_test.go
- cmd/root.go
- internal/api/router.go
- internal/api/common/errors.go
- internal/storage/driver/driver_test.go
- internal/storage/bucket/default_bucket.go
- internal/storage/system/store.go
- internal/worker/async_block.go
- internal/storage/system/factory.go
- internal/tracing/tracing.go
🧰 Additional context used
🧬 Code Definitions (4)
internal/storage/module.go (1)
internal/tracing/tracing.go (2)
Trace(43-54)NoResult(56-60)
internal/storage/driver/module.go (4)
internal/storage/ledger/factory.go (2)
Factory(9-11)NewFactory(18-23)internal/storage/driver/driver.go (3)
Option(304-304)Driver(27-36)New(285-302)internal/storage/system/store.go (2)
Option(145-145)New(133-143)internal/storage/system/factory.go (1)
NewStoreFactory(21-23)
cmd/buckets_upgrade.go (4)
internal/storage/driver/driver.go (2)
Driver(27-36)New(285-302)cmd/root.go (1)
Version(18-18)internal/storage/driver/module.go (1)
NewFXModule(26-74)internal/storage/module.go (2)
NewFXModule(20-79)ModuleConfig(16-18)
internal/storage/driver/driver.go (5)
internal/storage/system/factory.go (1)
StoreFactory(7-9)internal/storage/common/resource.go (1)
ColumnPaginatedQuery(405-414)internal/tracing/tracing.go (1)
Trace(43-54)internal/storage/ledger/store.go (2)
New(161-248)Option(265-265)internal/storage/system/store.go (2)
New(133-143)Option(145-145)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: GoReleaser
- GitHub Check: Tests
🔇 Additional comments (16)
internal/storage/bucket/migrations.go (2)
10-10: LGTM: Adding OpenTelemetry tracing support.Adding the import for OpenTelemetry protocol provides enhanced observability capabilities for the migration process.
59-60: Good addition of error recording for observability.Recording errors with OpenTelemetry improves the traceability of migration failures, which will make debugging easier while maintaining the same control flow logic.
internal/storage/module.go (3)
9-10: LGTM: Adding imports for tracing functionality.These imports are necessary to support the new tracing capabilities being added to the health check.
23-23: LGTM: Updated function signature to include tracer.Adding the tracer parameter to the health check provider allows for proper tracing throughout the health check function.
26-41: Improved health check with structured tracing.You've enhanced the health check with tracing capabilities, which will provide better observability into storage driver health status. The logic is now properly wrapped with tracing while maintaining the same functional behavior.
cmd/buckets_upgrade.go (3)
6-7: LGTM: Adding OpenTelemetry imports.Adding these imports is necessary for the integration of tracing capabilities in the bucket upgrade command.
21-27: Simplified command execution with new helper function.The refactoring to use
withStorageDrivermakes the command execution cleaner and more focused on its primary responsibility.
37-65: Introduced helper function for storage driver lifecycle management.The new
withStorageDriverfunction centralizes the setup and teardown of the storage driver using the fx framework, which simplifies command execution and ensures proper resource cleanup.Consider improving error handling for app shutdown:
defer func() { - _ = app.Stop(cmd.Context()) + if stopErr := app.Stop(cmd.Context()); stopErr != nil { + logger.Errorf("Error stopping application: %v", stopErr) + } }()internal/storage/driver/driver.go (7)
8-11: LGTM: Added imports for tracing and system store.The new imports support the enhanced tracing capabilities and the factory pattern for system store creation.
31-31: LGTM: Added systemStoreFactory field to the Driver struct.Adding the factory field enhances modularity and enables dependency injection for system store creation.
195-232: Enhanced UpgradeAllBuckets with tracing capabilities.The refactoring of
UpgradeAllBucketsto use tracing improves observability of the upgrade process. The code is now properly structured to record errors and track the progress of bucket migrations with spans.
234-254: Good extraction of upgradeBucket helper method with tracing.Extracting the
upgradeBucketmethod improves code organization and adds proper tracing for individual bucket upgrades, with span context linking to maintain the trace hierarchy.
289-290: LGTM: Updated constructor to include systemStoreFactory.The updated constructor signature properly includes the new systemStoreFactory dependency.
318-322: LGTM: Added WithTracer option.Adding this option allows for configurable tracing in the Driver, which enhances flexibility and testability.
327-327: LGTM: Updated default options to include a noop tracer.Setting a default noop tracer ensures that tracing is always available even when not explicitly configured.
internal/storage/driver/module.go (1)
31-46: Clean dependency injection model with optional telemetry providers.The refactored code introduces a more flexible way to provide dependencies to the
ledgerstore.Factoryby using anfx.Instruct with optional telemetry providers. This approach allows components that don't need tracing or metrics to function without them, enhancing modularity.
gfyrag
force-pushed
the
feat/upgrade-traces
branch
from
7cae448 to
44f2943
Compare
gfyrag
force-pushed
the
feat/upgrade-traces
branch
from
44f2943 to
974c78d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
internal/storage/driver/driver.go (1)
42-42:⚠️ Potential issueUse the transaction context for system store creation
Here, the system store is created with
d.dbinstead oftx, which could lead to inconsistencies if the transaction is rolled back. The system store operations would persist even if other operations within the transaction fail.Apply this fix:
- systemStore := d.systemStoreFactory.Create(d.db) + systemStore := d.systemStoreFactory.Create(tx)
🧹 Nitpick comments (1)
cmd/buckets_upgrade.go (1)
37-65: Excellent refactoring to centralize driver lifecycle management.The new
withStorageDriverfunction provides a consistent way to set up and tear down the storage driver with proper tracing integration. This will reduce code duplication and ensure proper cleanup of resources.However, there's room for improvement in the error handling during app shutdown.
Consider capturing and logging errors during application shutdown:
defer func() { - _ = app.Stop(cmd.Context()) + if err := app.Stop(cmd.Context()); err != nil { + logger.Errorf("Error stopping application: %v", err) + } }()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sumtools/generator/go.modis excluded by!**/*.modtools/generator/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (19)
cmd/buckets_upgrade.go(3 hunks)cmd/root.go(2 hunks)cmd/serve.go(1 hunks)internal/api/bulking/bulker.go(2 hunks)internal/api/common/errors.go(2 hunks)internal/api/router.go(2 hunks)internal/storage/bucket/default_bucket.go(1 hunks)internal/storage/bucket/migrations.go(2 hunks)internal/storage/driver/driver.go(8 hunks)internal/storage/driver/driver_test.go(5 hunks)internal/storage/driver/module.go(1 hunks)internal/storage/ledger/main_test.go(1 hunks)internal/storage/module.go(1 hunks)internal/storage/system/factory.go(1 hunks)internal/storage/system/store.go(3 hunks)internal/tracing/tracing.go(3 hunks)internal/worker/async_block.go(5 hunks)internal/worker/fx.go(3 hunks)test/migrations/upgrade_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (14)
- cmd/serve.go
- internal/storage/ledger/main_test.go
- internal/api/router.go
- test/migrations/upgrade_test.go
- internal/api/common/errors.go
- internal/api/bulking/bulker.go
- internal/storage/bucket/default_bucket.go
- internal/storage/system/factory.go
- internal/storage/driver/driver_test.go
- internal/storage/system/store.go
- cmd/root.go
- internal/tracing/tracing.go
- internal/worker/fx.go
- internal/worker/async_block.go
🧰 Additional context used
🧬 Code Definitions (3)
internal/storage/driver/module.go (5)
internal/storage/ledger/factory.go (2)
Factory(9-11)NewFactory(18-23)internal/storage/driver/driver.go (3)
Option(304-304)Driver(27-36)New(285-302)internal/storage/system/store.go (2)
Option(145-145)New(133-143)internal/storage/ledger/store.go (2)
Option(265-265)New(161-248)internal/storage/system/factory.go (1)
NewStoreFactory(21-23)
cmd/buckets_upgrade.go (4)
internal/storage/driver/driver.go (2)
Driver(27-36)New(285-302)cmd/root.go (1)
Version(18-18)internal/storage/driver/module.go (1)
NewFXModule(26-74)internal/storage/module.go (2)
NewFXModule(20-79)ModuleConfig(16-18)
internal/storage/driver/driver.go (4)
internal/storage/system/factory.go (1)
StoreFactory(7-9)internal/tracing/tracing.go (1)
Trace(43-54)internal/storage/ledger/store.go (2)
New(161-248)Option(265-265)internal/storage/system/store.go (2)
New(133-143)Option(145-145)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Tests
🔇 Additional comments (13)
internal/storage/bucket/migrations.go (2)
10-10: Good addition of OpenTelemetry for error tracking.The addition of the OpenTelemetry package will help with tracing errors during migrations, improving observability throughout the migration path.
59-60: Excellent error handling improvement with tracing.Adding
otlp.RecordError(ctx, err)ensures that migration errors are properly recorded in your telemetry system, while still allowing normal error propagation. This will help with debugging migration issues in production.internal/storage/driver/module.go (2)
31-46: Well-structured dependency injection with optional telemetry providers.The refactoring to use
fx.Instruct with optional tags for TracerProvider and MeterProvider is a good practice. This allows for more flexible dependency injection and makes it easier to run the system in different configurations (with or without telemetry).
57-60: Good implementation of factory pattern for system store.Using
systemstore.NewStoreFactorywith the tracer ensures that all system store operations are properly traced. This centralizes the creation of system stores and ensures consistent tracing across all instances.internal/storage/module.go (3)
9-10: Good addition of tracing imports.These imports support the new tracing functionality in the module.
23-23: Health check now properly supports tracing.Adding the tracer parameter to the health check provider allows for tracing health check operations, which is consistent with the PR's goal of adding traces throughout the system.
26-41: Well-implemented tracing for health checks.The health check logic has been wrapped with
tracing.Trace, which will properly record timing and errors for health check operations. The implementation maintains the same functionality while adding observability.cmd/buckets_upgrade.go (2)
6-7: Good addition of OpenTelemetry packages.Adding the OTLP packages enables tracing support in the command-line tools, which is essential for complete end-to-end tracing.
21-27: Simplified command execution with better error handling.The refactoring to use
withStorageDrivermakes the command implementation much cleaner and ensures consistent error handling.internal/storage/driver/driver.go (4)
195-232: Well-structured tracing implementation for batch bucket upgradesThe refactoring of the
UpgradeAllBucketsmethod provides excellent observability with the addition of tracing. This will help monitor the upgrade process and troubleshoot issues more effectively.
234-254: Good separation of concerns with proper tracingThe new
upgradeBucketmethod nicely encapsulates the bucket upgrade logic with proper tracing. The trace context is set up correctly with a new root span that links back to the parent span, which allows for better distributed tracing visualization.
318-322: Good addition of tracer configuration optionThe new
WithTraceroption function enables flexible configuration of the tracing implementation, aligning with the dependency injection pattern used throughout the codebase.
327-327: Safe default with noop tracerSetting a noop tracer as the default ensures that the code will work correctly even if no tracer is explicitly provided, which is a good defensive programming practice.
No description provided.