feat: simplify health check #750
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes span multiple parts of the codebase. A new ignore pattern has been added to ignore Pulumi YAML files, and several Earthfile imports have been upgraded from v0.19.0 to v0.19.1. In the command layer, the system store dependency has been removed and driver initialization simplified. Many internal packages now use pointer types for identifiers (IDs), and new helper methods (e.g. WithID) have been added. Numerous API and mock signatures were updated to use consistent internal types. In storage and migration modules, the database interface has been generalized via bun.IDB and SQL migration scripts were enhanced with conditional index creation. Finally, the SDK was updated with version bumps and refactored endpoint methods. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant V2 as V2 Client
participant S as HTTP Server
C->>V2: Call GetInfo(ctx, opts...)
V2->>S: Send GET request to "/_/info"
S-->>V2: Return HTTP response
V2->>C: Parse and return V2GetInfoResponse
Possibly related PRs
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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 (
|
gfyrag
force-pushed
the
hotfix/v2.2/light-healthcheck
branch
from
cb661c9 to
4296bb9
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
🧹 Nitpick comments (25)
internal/machine/vm/run.go (1)
38-43: Improved handling of different amount types in ScriptV1.ToCoreThe type switch implementation handles both string and float64 types for the "amount" field, which addresses compatibility issues between version 1 and version 2 API formats for large numbers mentioned in the PR objectives.
However, there's a potential issue with the float64 to int conversion which may lead to precision loss for very large numbers.
Consider enhancing the implementation to better handle large float values:
switch amount := v["amount"].(type) { case string: s.Script.Vars[k] = fmt.Sprintf("%s %s", v["asset"], amount) case float64: - s.Script.Vars[k] = fmt.Sprintf("%s %d", v["asset"], int(amount)) + // Use fmt.Sprintf with %g for better float representation or consider big.Int for very large numbers + s.Script.Vars[k] = fmt.Sprintf("%s %g", v["asset"], amount) }internal/machine/vm/run_test.go (2)
482-486: Simplify big integer string creationThe current approach uses a function to create a string representation of a big integer, which adds unnecessary complexity.
Consider simplifying this to:
- "amount": func() string { - ret, _ := big.NewInt(0).SetString("9999999999999999999999999999999999999999", 10) - return ret.String() - }(), + "amount": "9999999999999999999999999999999999999999",Since you're already converting it to a string, there's no need to create a big.Int first.
458-462: Consider adding expected error cases to test structThe test case structure is good, but doesn't account for potential error cases.
Consider enhancing the test case struct to include expected error conditions:
type testCase struct { name string inputVars map[string]any expected map[string]string + expectError bool }This would allow testing scenarios where conversion might fail, improving test coverage.
internal/storage/ledger/resource.go (1)
303-303: Commented debug print in paginate method.
A debug line (//fmt.Println(finalQuery.Model(&ret).String())) has been added as a comment. If this was intended solely for troubleshooting during development, consider removing it before merging to keep the codebase clean.internal/storage/system/migrations.go (1)
14-14: API change to use more flexible interface typeThe parameter type for
dbhas been changed from*bun.DBtobun.IDB. This is a great improvement that follows the Dependency Inversion Principle by depending on an interface rather than a concrete implementation.However, there's an inconsistency with the
Migratefunction on line 223, which still uses*bun.DBas its parameter type:func Migrate(ctx context.Context, db *bun.DB, options ...migrations.Option) error {For consistency, consider updating this function to also use
bun.IDB.pkg/client/v2.go (2)
35-35: Minor observation: Duplicate scope in OAuth2Scopes.The OAuth2Scopes array contains two identical "ledger:read" entries. While this doesn't affect functionality, it's redundant.
- OAuth2Scopes: []string{"ledger:read", "ledger:read"}, + OAuth2Scopes: []string{"ledger:read"},
226-226: Same duplicate scope issue as in GetInfo method.The OAuth2Scopes array contains duplicate "ledger:read" entries here as well.
- OAuth2Scopes: []string{"ledger:read", "ledger:read"}, + OAuth2Scopes: []string{"ledger:read"},internal/storage/bucket/migrations.go (2)
15-35: Encapsulate or handle migration collection errors without panicking.
Panic calls halt the entire application if migration collection fails. Consider returning an error and letting the caller decide whether to panic, log, or recover gracefully.allMigrations, err := migrations.CollectMigrations(MigrationsFS, name, collectOptions...) if err != nil { - panic(err) + // return nil or handle error as appropriate + log.Fatalf("Failed to collect migrations: %v", err) }
37-52: Validate the loop-based migration approach.
This loop continues callingmigrator.UpByOne(ctx)until migrations are up to date. In scenarios with critical or time-consuming large migrations, consider limiting how many migrations can happen in one invocation or incorporate a progress mechanism to prevent excessive lock durations.internal/storage/driver/driver.go (3)
30-37: Add field-level comments or docstrings for new properties.
Documentingdb *bun.DB,migrationRetryPeriod, andparallelBucketMigrationsclarifies their purpose, default values, and expected usage.
166-214: Check for infinite-retry edge cases in parallel migrations.
Re-trying migrations withpondplus a time-based backoff can lead to an infinite loop if an irrecoverable error persists. Consider capping retries or collecting and reporting repeated failures back to a monitoring/logging system.
232-246: Validate that db is properly closed or managed.
Although referencingdb *bun.DBis straightforward, ensure you have a mechanism to close or manage connections, especially if using connection pooling.internal/storage/bucket/migrations/11-make-stateless/up.sql (2)
562-565: Clarify backward compatibility comment.
The inline comment clarifies re-creation of older functions to set a custom search path. Consider referencing an official version or ticket ID to help future maintainers understand why this duplication is essential for older databases.
565-579: Ensure indices support the get_transaction performance.
The function filters by_id,_before, andledger. Verify appropriate indexing (e.g., on(ledger, id, timestamp)) to avoid performance bottlenecks for large transaction tables.internal/storage/bucket/bucket.go (2)
21-24: Improved interface design with explicit dependency injectionThe method signatures in the
Factoryinterface have been updated to accept abun.IDBparameter, which is a good architectural change. This shifts from implicit dependency on a stored DB instance to explicit dependency injection at the method level.This change enhances flexibility by allowing different database contexts to be used per method call, improving testability and reducing tight coupling.
38-44: Factory constructor simplified by removing stored DB dependencyThe
NewDefaultFactoryconstructor no longer requires a database parameter, supporting the shift to method-level dependency injection.This change aligns with the PR objective of simplifying health checks by making the system more flexible in how it manages database connections, likely allowing more targeted database operations.
internal/controller/ledger/store_generated_test.go (1)
46-51: Consistently updated all domain model types to use internal packageAll domain model types in method signatures (Account, Transaction, Log, AggregatedVolumes, etc.) have been updated to use the
internalpackage instead of theledgerpackage, maintaining consistency with the type reorganization across the codebase.This consistent change across all types demonstrates a clear architectural boundary between domain models (now in
internal) and controller logic (inledger), which improves code organization and maintainability.Also applies to: 60-65, 103-108, 131-138, 191-196, 234-239, 248-254, 263-270, 293-298, 321-328, 337-346, 356-361
internal/storage/ledger/legacy/adapters.go (4)
24-27: Handle potential panic with type assertions.
Currently, the code usesvalue.(string)andvalue.(int)without checking. This will panic if the type is unexpected. Consider adding a safety check or clarifying guarantees to avoid runtime errors.Example approach:
_ = query.Builder.Walk(func(_ string, _ string, value any) error { - address = value.(string) + addr, ok := value.(string) + if !ok { + return errors.New("expected string for address") + } + address = addr return nil })Also applies to: 105-108
174-176: Refine big.Int usage for clarity.
Usingnew(big.Int).Add(new(big.Int), balance)is correct for creating a new instance but is somewhat non-standard. Commonly, developers usebig.NewInt(0).Add(...)for clarity.
34-36: Avoid repeated volume/tier expansion logic.
Multiple resource adapters repeat the same slices-based checks for "volumes" and "effectiveVolumes". Extracting this snippet into a shared helper function would reduce duplication and improve maintainability.Also applies to: 45-50, 64-66, 115-117, 145-147
225-227: Unify legacy vs. new store logic.
Each method checksif !d.isFullUpToDate. If more adapters are added, this pattern can proliferate. Consider a design pattern or a single point of decision to reduce branching across multiple methods.Also applies to: 233-234, 239-241, 246-248, 253-255
internal/storage/bucket/migrations/16-create-transaction-id-index-on-moves/up.sql (1)
1-1: Consistent Conditional Concurrent Indexing (with Minor Template Styling Note).
The index creation command uses a conditional inclusion ofconcurrentlysimilar to other migration files. Note that while the functionality is correct, the template variable for the schema is written as{{ .Schema }}here whereas other files use{{.Schema}}. For consistency across migration files, consider aligning the spacing. For example:- create index {{ if not .Transactional }}concurrently{{end}} moves_transactions_id on "{{ .Schema }}".moves(transactions_id); + create index {{ if not .Transactional }}concurrently{{end}} moves_transactions_id on "{{.Schema}}".moves(transactions_id);pkg/client/docs/sdks/v2/README.md (2)
44-48: Consider fixing markdown formatting issuesThe code examples use hard tabs for indentation, which markdown linters flag as inconsistent formatting. Consider replacing tabs with spaces for better consistency across documentation.
- "github.com/formancehq/ledger/pkg/client/models/components" - "github.com/formancehq/ledger/pkg/client" - "context" - "log" + "github.com/formancehq/ledger/pkg/client/models/components" + "github.com/formancehq/ledger/pkg/client" + "context" + "log"Also applies to: 95-99
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
44-44: Hard tabs
Column: 1(MD010, no-hard-tabs)
45-45: Hard tabs
Column: 1(MD010, no-hard-tabs)
46-46: Hard tabs
Column: 1(MD010, no-hard-tabs)
47-47: Hard tabs
Column: 1(MD010, no-hard-tabs)
76-82: Add blank lines around tablesFor better readability and to conform to markdown best practices, consider adding blank lines before and after tables in the documentation.
| `opts` | [][operations.Option](../../models/operations/option.md) | :heavy_minus_sign: | The options for this request. | + ### Response **[*operations.V2GetInfoResponse](../../models/operations/v2getinforesponse.md), error** + | Error Object | Status Code | Content Type | | ------------------------- | ------------------------- | ------------------------- | | sdkerrors.V2ErrorResponse | default | application/json | | sdkerrors.SDKError | 4xx-5xx | */* |🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
80-80: Tables should be surrounded by blank lines
null(MD058, blanks-around-tables)
pkg/client/README.md (1)
112-113: Inconsistent list style in markdownThe unordered list items use asterisks (*) while other lists in the document use dashes (-).
-* [GetInfo](docs/sdks/v2/README.md#getinfo) - Show server information -* [GetMetrics](docs/sdks/v2/README.md#getmetrics) - Read in memory metrics +- [GetInfo](docs/sdks/v2/README.md#getinfo) - Show server information +- [GetMetrics](docs/sdks/v2/README.md#getmetrics) - Read in memory metrics🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
112-112: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
113-113: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
🛑 Comments failed to post (3)
internal/machine/vm/run.go (1)
38-43: 🛠️ Refactor suggestion
Missing default case in type switch
The current implementation only handles string and float64 types for the "amount" field, but doesn't have a default case to handle other possible types.
Add a default case to ensure all types are properly handled:
switch amount := v["amount"].(type) { case string: s.Script.Vars[k] = fmt.Sprintf("%s %s", v["asset"], amount) case float64: s.Script.Vars[k] = fmt.Sprintf("%s %d", v["asset"], int(amount)) +default: + // Handle other types to ensure robustness + s.Script.Vars[k] = fmt.Sprintf("%s %v", v["asset"], amount) }📝 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.switch amount := v["amount"].(type) { case string: s.Script.Vars[k] = fmt.Sprintf("%s %s", v["asset"], amount) case float64: s.Script.Vars[k] = fmt.Sprintf("%s %d", v["asset"], int(amount)) default: // Handle other types to ensure robustness s.Script.Vars[k] = fmt.Sprintf("%s %v", v["asset"], amount) }internal/controller/ledger/controller_default.go (1)
408-414:
⚠️ Potential issueFixed balance calculation for accounts that are both sources and destinations
Added logic to properly handle cases where a destination account is also a source in some posting during transaction reversion. This ensures accurate balance calculation by updating both sides of the transaction.
This is an important fix that prevents incorrect balance calculations during transaction reversals. The new code properly updates balances for accounts that act as both sources and destinations in different postings.
internal/storage/driver/driver.go (1)
92-99: 🛠️ Refactor suggestion
Consider caching the opened ledger.
There is a TODO indicating repeated ledger lookups in system store. If performance is critical, caching the ledger could reduce repeated database hits, but be mindful of potential staleness and concurrency.
No description provided.