Remove traces downloader #639
Conversation
janezpodhostnik
requested review from
peterargue,
m-Peter and
zhangchiqing
as code owners
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes in this pull request involve the removal of trace-related functionality across multiple files, including the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
cmd/run/cmd.go (1)
Trace-related code still exists in the codebase
The trace functionality hasn't been completely removed. There are still several trace-related components that need attention:
bootstrap/bootstrap.go: Contains trace indexer and storage initializationapi/debug.go: Has trace configuration and API endpointsmetrics/: Contains trace download metricsREADME.md: Still documents trace-related configuration options🔗 Analysis chain
Line range hint
1-359: Verify complete removal of trace-related configurations.The trace-related flags have been removed as traces will now be generated locally. Let's verify that all trace-related code has been completely removed.
Let me gather more information about the trace-related code to ensure we have a complete picture.
Let me do one final check to ensure we haven't missed any trace-related code, particularly in the bootstrap package since it handles initialization.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify complete removal of trace-related code # Test 1: Search for any remaining trace-related flags or configurations echo "Searching for trace-related flags..." rg -i "trace.*flag|trace.*config" # Test 2: Search for any remaining trace-related function calls or variables echo "Searching for trace-related functions or variables..." rg -i "trace.*backfill|trace.*bucket|trace.*download" # Test 3: Search for any TODO comments about traces echo "Searching for trace-related TODOs..." rg -i "todo.*trace"Length of output: 1903
Script:
#!/bin/bash # Search for trace-related code in configuration files echo "Searching for trace configuration in config package..." rg -i "trace" config/ # Search for trace-related imports echo "Searching for trace-related imports..." rg -i "trace.*\"$" # Search for trace-related struct fields and types echo "Searching for trace-related structs and types..." ast-grep --pattern 'type $_ struct { $$$ Trace$_ $_ $$$ }' # Check for any trace-related command flags in other command files echo "Searching for trace-related flags in cmd directory..." rg -i "trace.*flag" cmd/Length of output: 715
Script:
#!/bin/bash # Search for trace-related code in bootstrap package echo "Searching for trace-related code in bootstrap..." rg -i "trace" bootstrap/ # Search for any remaining trace-related configuration or initialization echo "Searching for trace initialization..." ast-grep --pattern 'func $_($$_) { $$$ trace$_ $$$ }' # Check for any trace-related environment variables echo "Searching for trace-related environment variables..." rg -i "trace.*env|env.*trace"Length of output: 663
bootstrap/bootstrap.go (2)
Line range hint
486-505: Improve error handling in storage initializationThe storage initialization could benefit from better error handling to ensure proper cleanup in case of partial initialization failures.
Consider implementing a cleanup mechanism:
func setupStorage( config *config.Config, client *requester.CrossSporkClient, logger zerolog.Logger, ) (*Storages, error) { // create pebble storage from the provided database root directory store, err := pebble.New(config.DatabaseDir, logger) if err != nil { return nil, err } + // Ensure cleanup on error + var storages *Storages + defer func() { + if err != nil && store != nil { + if cerr := store.Close(); cerr != nil { + logger.Error().Err(cerr).Msg("failed to cleanup storage on error") + } + } + }() blocks := pebble.NewBlocks(store, config.FlowNetworkID) // ... rest of the initialization ... return &Storages{ Storage: store, Blocks: blocks, Transactions: pebble.NewTransactions(store), Receipts: pebble.NewReceipts(store), Accounts: pebble.NewAccounts(store), Traces: pebble.NewTraces(store), }, nil }
Line range hint
507-547: Enhance error handling in Run functionThe Run function could benefit from better cleanup handling when services fail to start. Currently, if a service fails to start, previously started services aren't properly stopped.
Consider implementing a more robust error handling approach:
func Run(ctx context.Context, cfg *config.Config, ready chan struct{}) error { boot, err := New(cfg) if err != nil { return err } + // Ensure cleanup on error + var startupError error + defer func() { + if startupError != nil { + boot.StopEventIngestion() + boot.StopMetricsServer() + boot.StopAPIServer() + boot.StopProfilerServer() + } + }() if err := boot.StartEventIngestion(ctx); err != nil { - return fmt.Errorf("failed to start event ingestion engine: %w", err) + startupError = fmt.Errorf("failed to start event ingestion engine: %w", err) + return startupError } if err := boot.StartAPIServer(ctx); err != nil { - return fmt.Errorf("failed to start API server: %w", err) + startupError = fmt.Errorf("failed to start API server: %w", err) + return startupError } if err := boot.StartMetricsServer(ctx); err != nil { - return fmt.Errorf("failed to start metrics server: %w", err) + startupError = fmt.Errorf("failed to start metrics server: %w", err) + return startupError } if err := boot.StartProfilerServer(ctx); err != nil { - return fmt.Errorf("failed to start profiler server: %w", err) + startupError = fmt.Errorf("failed to start profiler server: %w", err) + return startupError } // mark ready close(ready) // if context is canceled start shutdown <-ctx.Done() boot.logger.Warn().Msg("bootstrap received context cancellation, stopping services") boot.StopEventIngestion() boot.StopMetricsServer() boot.StopAPIServer() + boot.StopProfilerServer() return nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
Makefile(0 hunks)bootstrap/bootstrap.go(1 hunks)cmd/run/cmd.go(1 hunks)config/config.go(0 hunks)go.mod(2 hunks)services/traces/downloader.go(0 hunks)services/traces/engine.go(0 hunks)services/traces/engine_test.go(0 hunks)services/traces/mocks/Downloader.go(0 hunks)
💤 Files with no reviewable changes (6)
- Makefile
- config/config.go
- services/traces/downloader.go
- services/traces/engine.go
- services/traces/engine_test.go
- services/traces/mocks/Downloader.go
🔇 Additional comments (4)
go.mod (2)
34-34: LGTM: Correctly moved cloud.google.com/go/storage to indirect dependencies.
The change aligns with the removal of traces downloader functionality, as the storage SDK is no longer directly used but remains as a transitive dependency.
202-202: LGTM: Correctly moved google.golang.org/api to indirect dependencies.
The change is consistent with removing direct GCP API interactions while maintaining the dependency for transitive usage by other packages.
cmd/run/cmd.go (1)
75-77: LGTM: Simplified gas price error handling.
The error handling for invalid gas price has been simplified by directly returning an error when parsing fails. This change improves code readability while maintaining the same functionality.
bootstrap/bootstrap.go (1)
245-245: Verify complete removal of trace functionality
The code appears to have incomplete removal of trace-related components. While the PR objective and AI summary indicate that traces will be generated locally and the traces downloader should be removed, several trace-related components still exist:
- The
Tracesfield in theStoragesstruct is still present - The
NewTraces(store)initialization insetupStorageis still active - The
debugAPIis still being initialized with trace storage
This could lead to:
- Orphaned code that's no longer used
- Potential runtime issues if trace functionality is partially removed
Let's verify the trace-related code that might need cleanup:
Also applies to: 500-500
| if b.config.TracesEnabled { | ||
| debugAPI = api.NewDebugAPI(b.storages.Traces, b.storages.Blocks, b.logger, b.collector) | ||
| } | ||
| var debugAPI = api.NewDebugAPI(b.storages.Traces, b.storages.Blocks, b.logger, b.collector) |
There was a problem hiding this comment.
Review debugAPI initialization after trace changes
The debugAPI initialization is now unconditional and still depends on trace storage. If traces are to be generated locally as mentioned in the PR objective, the debugAPI might need to be updated to work with the new local trace generation mechanism.
Consider updating the debugAPI initialization to:
- Use the new local trace generation mechanism
- Handle cases where trace storage might not be available
-var debugAPI = api.NewDebugAPI(b.storages.Traces, b.storages.Blocks, b.logger, b.collector)
+// TODO: Update to use local trace generation
+var debugAPI = api.NewDebugAPI(b.storages.Blocks, b.logger, b.collector)Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
LGTM! 👏
We can also remove the relevant CLI flags for traces on https://github.com/onflow/flow-evm-gateway/blob/main/README.md#configuration-flags.
|
I think we should only do this, when we have a local solution, so maybe the PR should target the feature branch. |
@ramtinms Good point, I updated the base branch, to the feature branch we use. |
janezpodhostnik
force-pushed
the
janez/remove-traces
branch
from
29db5e7 to
597a429
Compare
| @@ -222,14 +222,10 @@ The application can be configured using the following flags at runtime: | |||
| | `stream-limit` | `10` | Rate-limit for client events sent per second | | |||
| | `rate-limit` | `50` | Requests per second limit for clients over any protocol (ws/http) | | |||
| | `address-header` | `""` | Header for client IP when server is behind a proxy | | |||
| | `heartbeat-interval` | `100` | Interval for AN event subscription heartbeats | | |||
There was a problem hiding this comment.
I forgot to remove this earlier
|
@m-Peter Thanks for changing the base, this should definitely only go into the feature branch. We can hold off on merging this if it causes you any conflicts. |
@janezpodhostnik I would say let's merge this, since I also need the changes for enabled the |
janezpodhostnik
merged commit 8472396
into
feature/local-tx-reexecution
ref: #638
Description
Traces will get generated locally.
For contributor use:
masterbranchFiles changedin the Github PR explorerSummary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor
Chores
Tests