(OraklNode) Async fetcher #1523
Conversation
|
Warning Rate Limit Exceeded@nick-bisonai has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 40 minutes and 49 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe recent updates introduce new features and significant refactoring to enhance data handling and streaming capabilities. Key additions include a Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant App
participant Collector
participant Streamer
participant Redis
participant PostgreSQL
App->>Collector: startAllCollectors(ctx)
Collector-->>Redis: collect(ctx)
Redis-->>Collector: data
Collector-->>App: data collected
App->>Streamer: startStreamer(ctx)
Streamer-->>Redis: fetch data
Redis-->>Streamer: data
Streamer-->>PostgreSQL: stream data
Streamer-->>App: data streamed
Tip Early Access Features
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 as PR comments)
Additionally, you can add CodeRabbit Configration 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: 3
Outside diff range and nitpick comments (3)
node/pkg/fetcher/streamer.go (1)
16-37: TheRunmethod is well-implemented with proper context management and periodic task scheduling. Consider enhancing the error handling by potentially retrying or escalating the error if it persists.node/pkg/fetcher/collector.go (1)
20-42: TheRunmethod is well-implemented with proper context management and periodic task scheduling. Consider enhancing the error handling by potentially retrying or escalating the error if it persists.node/pkg/fetcher/app_test.go (1)
Line range hint
41-121: TheTestAppRunfunction adequately tests the initialization, running, and stopping of the app components. Consider adding more assertions to verify the state of the data after each operation for more comprehensive testing.
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (15)
- node/.env.example (2 hunks)
- node/.env.local (1 hunks)
- node/pkg/db/redis.go (4 hunks)
- node/pkg/db/redis_test.go (2 hunks)
- node/pkg/error/sentinel.go (1 hunks)
- node/pkg/fetcher/app.go (10 hunks)
- node/pkg/fetcher/app_test.go (3 hunks)
- node/pkg/fetcher/collector.go (1 hunks)
- node/pkg/fetcher/fetcher.go (5 hunks)
- node/pkg/fetcher/fetcher_test.go (4 hunks)
- node/pkg/fetcher/main_test.go (5 hunks)
- node/pkg/fetcher/streamer.go (1 hunks)
- node/pkg/fetcher/types.go (3 hunks)
- node/pkg/fetcher/utils.go (3 hunks)
- node/pkg/fetcher/utils_test.go (1 hunks)
Files skipped from review due to trivial changes (3)
- node/.env.example
- node/.env.local
- node/pkg/fetcher/fetcher.go
Additional Context Used
Learnings (2)
Common
User: nick-bisonai PR: Bisonai/orakl#1280 File: node/pkg/fetcher/fetcher.go:267-267 Timestamp: 2024-03-19T13:22:13.702Z Learning: Data insertion in the Orakl project is handled by `./core`, not `./node`, and references within `./api` are considered irrelevant to changes made in the PR context. This highlights the architectural separation between `./api`, `./core`, and `./node` within the project.node/pkg/fetcher/app_test.go (1)
User: nick-bisonai PR: Bisonai/orakl#1280 File: node/pkg/fetcher/fetcher.go:267-267 Timestamp: 2024-03-19T13:22:13.702Z Learning: Data insertion in the Orakl project is handled by `./core`, not `./node`, and references within `./api` are considered irrelevant to changes made in the PR context. This highlights the architectural separation between `./api`, `./core`, and `./node` within the project.
Additional comments not posted (48)
node/pkg/fetcher/streamer.go (2)
10-14: TheNewStreamerfunction is correctly initializing theStreamerstruct with the provided interval.
39-47: TheJobmethod correctly handles data fetching and copying with appropriate error handling and logging.node/pkg/fetcher/collector.go (3)
11-18: TheNewCollectorfunction correctly initializes theCollectorstruct with the provided configuration and feeds.
44-63: TheJobmethod correctly handles data collection, aggregation, and insertion with appropriate error handling and logging.
65-80: Thecollectmethod efficiently fetches and processes data for each feed with appropriate error handling and logging.node/pkg/fetcher/types.go (3)
22-24: Adding aTimestampfield to theFeedDatastruct is a good practice for tracking data retrieval times.
50-57: TheCollectorstruct is appropriately defined with necessary fields for lifecycle management and operation.
59-65: TheStreamerstruct is well-structured with necessary fields for effective lifecycle management.node/pkg/fetcher/utils.go (5)
53-59: ThesetLatestFeedDatafunction correctly sets the latest feed data in Redis with appropriate error handling.
61-72: ThegetLatestFeedDatafunction efficiently retrieves the latest feed data from Redis with appropriate error handling.
74-76: ThesetFeedDataBufferfunction correctly pushes feed data to a Redis list with appropriate error handling.
78-80: ThegetFeedDataBufferfunction effectively retrieves and clears the feed data buffer from Redis with appropriate error handling.
94-101: ThecopyFeedDatafunction correctly copies feed data to a database with appropriate error handling.node/pkg/db/redis.go (4)
61-76: TheMSetObjectfunction correctly sets multiple key-value pairs in Redis with appropriate error handling and data marshalling.
169-185: TheLRangeObjectfunction efficiently retrieves a range of objects from a Redis list and correctly unmarshals them with appropriate error handling.
196-196: TheLPushObjectfunction correctly pushes multiple objects to a Redis list with appropriate error handling and data marshalling.
235-237: ThePopAllObjectfunction effectively pops all objects from a Redis list and correctly unmarshals them with appropriate error handling.node/pkg/fetcher/main_test.go (3)
79-119: The setup function is well-structured and covers all necessary initializations for testing, including mock servers and data insertion.
Line range hint
121-232: The function effectively isolates external dependencies by using mock data sources and inserts necessary test data.
Line range hint
235-265: The cleanup function is comprehensive, ensuring that all resources are properly released and database entries cleared after tests.node/pkg/fetcher/fetcher_test.go (6)
19-57: The test function correctly initializes fetchers and verifies their running state, ensuring the fetcher's functionality is as expected.
60-103: The test function effectively tests the fetch operation and ensures that the latest feed data is correctly handled and stored.
188-225: The test function correctly tests interactions with centralized exchanges and validates the results, ensuring thecexmethod functions as expected.
Line range hint
228-266: The test function correctly tests interactions with the Uniswap V3 protocol and validates the results, ensuring theuniswapV3method functions as expected.
268-304: The test function effectively tests therequestFeedmethod, ensuring that the request operation is correctly performed and the results are validated.
317-339: The test function correctly tests thefilterProxyByLocationmethod, ensuring that proxies are accurately filtered based on their location.node/pkg/fetcher/utils_test.go (9)
20-109: The test function effectively tests theFetchSinglemethod using a mock server to ensure controlled test conditions and correct data processing.
111-134: The test function correctly tests thegetTokenPricemethod, ensuring accurate price calculation based on predefined values.
136-166: The test function effectively tests thesetLatestFeedDatamethod, ensuring that feed data is correctly stored and retrieved from the database.
168-197: The test function correctly tests thegetLatestFeedDatamethod, ensuring that feed data is accurately retrieved and validated.
199-227: The test function effectively tests thesetFeedDataBuffermethod, ensuring that feed data is correctly stored in and retrieved from the buffer.
229-257: The test function correctly tests thegetFeedDataBuffermethod, ensuring that buffer data is accurately retrieved and validated.
259-286: The test function effectively tests theinsertLocalAggregatePgsqlmethod, ensuring that local aggregates are correctly inserted into and retrieved from the PostgreSQL database.
288-316: The test function effectively tests theinsertLocalAggregateRdbmethod, ensuring that local aggregates are correctly inserted into and retrieved from the Redis database.
319-361: The test function effectively tests thecopyFeedDatamethod, ensuring that feed data is correctly copied into and retrieved from the database.node/pkg/db/redis_test.go (8)
Line range hint
1-14: The test function correctly verifies the singleton behavior of the Redis connection, ensuring that the same instance is returned on multiple calls.
Line range hint
16-36: The test function effectively tests the basicSetandGetoperations in Redis, ensuring that values are correctly set and retrieved.
Line range hint
38-66: The test function effectively tests theMGetoperation in Redis, ensuring that multiple values are correctly set and retrieved.
Line range hint
68-76: The test function correctly handles the case of empty keys in theMGetoperation, ensuring that an appropriate error is returned and no values are retrieved.
Line range hint
78-106: The test function effectively handles a mix of valid and invalid keys in theMGetoperation, ensuring that correct values are retrieved for valid keys andnilis returned for invalid keys.
Line range hint
108-136: The test function effectively tests theMSetObjectoperation in Redis, ensuring that multiple objects are correctly set, retrieved, and marshalled.
Line range hint
138-166: The test function effectively tests theSetObjectoperation in Redis, ensuring that an object is correctly set, retrieved, and marshalled.
Line range hint
168-206: The test function effectively tests theMGetObjectoperation in Redis, ensuring that multiple objects are correctly set, retrieved, and unmarshalled.node/pkg/fetcher/app.go (4)
233-244: Optimize the initialization of collectors to avoid potential performance issues.
[PERFORMANCE]
Consider using a more efficient scheduling mechanism than random sleep to manage the start time of collectors.
188-197: Ensure idempotence instartCollectormethod.Verification successful
The
startCollectormethod in theAppclass is idempotent. It checks if the collector is already running before starting it, ensuring that multiple invocations do not produce side effects.
node/pkg/fetcher/app.go, lines 188-197: The method checkscollector.isRunningand returns early if true, preventing multiple starts.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `startCollector` method is idempotent. # Test: Search for multiple calls to `startCollector`. Expect: No side effects on multiple invocations. rg --type python $'startCollector'Length of output: 64
Script:
#!/bin/bash # Description: Verify that `startCollector` method is idempotent. # Test: Search for multiple calls to `startCollector` in Go files. rg --type go 'startCollector' # Test: Retrieve the implementation of `startCollector` to check for side effects. ast-grep --lang go --pattern $'func (a *App) startCollector(ctx context.Context, collector *Collector) error { $$$ }'Length of output: 1013
33-33: Ensure proper error handling and logging forstartAllmethod.Verification successful
The
startAllmethod innode/pkg/fetcher/app.godoes include proper error handling and logging. The method checks for errors when starting fetchers, collectors, and streamers, and logs errors appropriately.
The
startAllmethod itself:func (a *App) startAll(ctx context.Context) error { err := a.startAllFetchers(ctx) if err != nil { return err } err = a.startAllCollectors(ctx) if err != nil { return err } return a.startStreamer(ctx) }Error handling and logging within the method:
err := a.startAllFetchers(ctx) if err != nil { return err } err = a.startAllCollectors(ctx) if err != nil { return err } return a.startStreamer(ctx)The method returns errors encountered during the start process, ensuring that any issues are propagated and can be handled by the caller. Additionally, the caller of
startAlllogs errors and handles them appropriately.
- Example of error handling by the caller:
err := a.startAll(ctx) if err != nil { log.Error().Err(err).Str("Player", "Fetcher").Msg("failed to start all fetchers") bus.HandleMessageError(err, msg, "failed to start all fetchers") return }This confirms that the
startAllmethod and its usage include proper error handling and logging.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify proper error handling in `startAll` method. # Test: Search for error handling patterns in `startAll` method. Expect: Proper error handling and logging. rg --type python $'startAll'Length of output: 58
Script:
#!/bin/bash # Description: Verify proper error handling in `startAll` method. # Test: Search for error handling patterns in `startAll` method. Expect: Proper error handling and logging. rg --type go 'func startAll' -A 20Length of output: 36
Script:
#!/bin/bash # Description: Verify proper error handling in `startAll` method. # Test: Search for any occurrence of `startAll` in Go files. Expect: Proper error handling and logging. rg 'startAll' --type go -A 20Length of output: 10450
348-363: Validate environment variable parsing ininitializemethod.Verification successful
The environment variable
FEED_DATA_STREAM_INTERVALis correctly parsed and handled in theinitializemethod. If parsing fails, the code falls back to a default value of 5 seconds.
node/pkg/fetcher/app.go:
- Line 356:
streamIntervalRaw := os.Getenv("FEED_DATA_STREAM_INTERVAL")- Line 358:
streamInterval, err := time.ParseDuration(streamIntervalRaw)- Line 360:
if err != nil { streamInterval = time.Second * 5 }Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Ensure that the environment variable `FEED_DATA_STREAM_INTERVAL` is correctly parsed and handled. # Test: Check for fallback mechanism if parsing fails. Expect: Default value is used if parsing fails. rg --type python $'FEED_DATA_STREAM_INTERVAL'Length of output: 75
Script:
#!/bin/bash # Description: Ensure that the environment variable `FEED_DATA_STREAM_INTERVAL` is correctly parsed and handled. # Test: Check for fallback mechanism if parsing fails. Expect: Default value is used if parsing fails. rg --type go 'FEED_DATA_STREAM_INTERVAL'Length of output: 125
node/pkg/error/sentinel.go (1)
138-139: Add new error constants for better granularity in error handling.These new constants
ErrCollectorCancelNotFoundandErrStreamerCancelNotFoundwill help in more precise error handling and debugging.
nick-bisonai
force-pushed
the
feat/async-fetcher
branch
from
d2df8e2 to
fbeb05c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (15)
- node/.env.example (2 hunks)
- node/.env.local (1 hunks)
- node/pkg/db/redis.go (4 hunks)
- node/pkg/db/redis_test.go (2 hunks)
- node/pkg/error/sentinel.go (1 hunks)
- node/pkg/fetcher/app.go (10 hunks)
- node/pkg/fetcher/app_test.go (3 hunks)
- node/pkg/fetcher/collector.go (1 hunks)
- node/pkg/fetcher/fetcher.go (5 hunks)
- node/pkg/fetcher/fetcher_test.go (4 hunks)
- node/pkg/fetcher/main_test.go (5 hunks)
- node/pkg/fetcher/streamer.go (1 hunks)
- node/pkg/fetcher/types.go (3 hunks)
- node/pkg/fetcher/utils.go (3 hunks)
- node/pkg/fetcher/utils_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (10)
- node/.env.example
- node/.env.local
- node/pkg/db/redis.go
- node/pkg/db/redis_test.go
- node/pkg/error/sentinel.go
- node/pkg/fetcher/app.go
- node/pkg/fetcher/collector.go
- node/pkg/fetcher/fetcher.go
- node/pkg/fetcher/streamer.go
- node/pkg/fetcher/utils.go
Additional Context Used
Learnings (2)
Common
User: nick-bisonai PR: Bisonai/orakl#1280 File: node/pkg/fetcher/fetcher.go:267-267 Timestamp: 2024-03-19T13:22:13.702Z Learning: Data insertion in the Orakl project is handled by `./core`, not `./node`, and references within `./api` are considered irrelevant to changes made in the PR context. This highlights the architectural separation between `./api`, `./core`, and `./node` within the project.node/pkg/fetcher/app_test.go (1)
User: nick-bisonai PR: Bisonai/orakl#1280 File: node/pkg/fetcher/fetcher.go:267-267 Timestamp: 2024-03-19T13:22:13.702Z Learning: Data insertion in the Orakl project is handled by `./core`, not `./node`, and references within `./api` are considered irrelevant to changes made in the PR context. This highlights the architectural separation between `./api`, `./core`, and `./node` within the project.
Additional comments not posted (18)
node/pkg/fetcher/types.go (3)
24-24: AddedTimestampfield toFeedDataenhances data tracking capabilities.
50-57: Introduction ofCollectorstruct organizes data collection functionalities effectively.
59-65: Introduction ofStreamerstruct clearly defines responsibilities for streaming operations.node/pkg/fetcher/app_test.go (3)
Line range hint
41-84: UpdatedTestAppRunto include checks forCollectorandStreamercomponents, ensuring new functionalities are covered by tests.
74-84: Enhanced test for stopping the application now includesCollectorandStreamercomponents, ensuring all components can be properly stopped.
100-115: Updated tests include operations related to fetching and handling of feed data buffers, ensuring comprehensive testing of the application's data handling capabilities.node/pkg/fetcher/main_test.go (2)
79-119: Enhanced setup function includes routes for new components and initializes mock data sources, improving the test environment's comprehensiveness and relevance.
Line range hint
235-265: Updated cleanup function includes shutdown operations for new components and thorough data cleanup, ensuring a clean state after tests.node/pkg/fetcher/fetcher_test.go (4)
60-103: UpdatedTestFetcherFetcherJobto include thefetcherJobfunction, ensuring that the refactored fetch operation is thoroughly tested.
Line range hint
162-186: EnhancedTestFetcherFetchProxyincludes setup and teardown for a proxy server, improving the test's relevance and coverage of proxy-based fetching scenarios.
188-225: UpdatedTestFetcherCexincludes operations related to fetching from centralized exchanges, ensuring comprehensive testing of the application's data source handling capabilities.
Line range hint
228-266: UpdatedTestFetcherUniswapV3includes operations related to fetching from Uniswap V3, ensuring comprehensive testing of DeFi-related functionalities.node/pkg/fetcher/utils_test.go (6)
136-166: UpdatedTestSetLatestFeedDatachecks the functionality of setting and retrieving the latest feed data, ensuring that the utility functions are thoroughly tested.
168-197: UpdatedTestGetLatestFeedDataensures that the latest feed data can be retrieved correctly, confirming the reliability of the utility function.
199-227: UpdatedTestSetFeedDataBufferchecks the functionality of setting and retrieving data from the feed data buffer, ensuring that the utility functions are thoroughly tested.
229-257: UpdatedTestGetFeedDataBufferensures that data can be retrieved from the feed data buffer correctly, confirming the reliability of the utility function.
259-286: UpdatedTestInsertLocalAggregatePgsqlchecks the functionality of inserting and retrieving local aggregates in PostgreSQL, ensuring that the utility functions are thoroughly tested.
288-316: UpdatedTestInsertLocalAggregateRdbchecks the functionality of inserting and retrieving local aggregates in Redis, ensuring that the utility functions are thoroughly tested.
There was a problem hiding this comment.
Actionable comments posted: 3
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- node/pkg/db/redis.go (4 hunks)
- node/pkg/db/redis_test.go (3 hunks)
- node/pkg/fetcher/fetcher.go (5 hunks)
- node/pkg/fetcher/utils.go (3 hunks)
- node/pkg/fetcher/utils_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (4)
- node/pkg/db/redis.go
- node/pkg/db/redis_test.go
- node/pkg/fetcher/fetcher.go
- node/pkg/fetcher/utils.go
Additional Context Used
Learnings (1)
Common
User: nick-bisonai PR: Bisonai/orakl#1280 File: node/pkg/fetcher/fetcher.go:267-267 Timestamp: 2024-03-19T13:22:13.702Z Learning: Data insertion in the Orakl project is handled by `./core`, not `./node`, and references within `./api` are considered irrelevant to changes made in the PR context. This highlights the architectural separation between `./api`, `./core`, and `./node` within the project.
Additional comments not posted (3)
node/pkg/fetcher/utils_test.go (3)
111-134: The testTestGetTokenPricecorrectly uses big integers and performs calculations based on the Uniswap protocol. The assertion is precise and checks for an exact value, which is good for ensuring accuracy.
199-256: The testsTestSetFeedDataBufferandTestGetFeedDataBuffereffectively test the buffer functionalities. It's good to see the use ofdeferfor cleanup. However, ensure that the buffer is tested for the correct order of data, as buffers often have order-specific behavior.Consider adding assertions to check the order of data in the buffer to ensure it's being processed correctly.
319-361: The testTestCopyFeedDatachecks the functionality of copying data from a buffer to a database. The cleanup logic is well-handled withdefer. However, consider adding more detailed assertions to verify the integrity of the data copied.
[REFACTOR_SUGGESTion]
Add assertions to check specific fields in the copied data to ensure all expected attributes are correctly transferred and stored.
There was a problem hiding this comment.
Actionable comments posted: 5
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- node/pkg/db/redis.go (4 hunks)
Additional Context Used
Learnings (1)
Common
User: nick-bisonai PR: Bisonai/orakl#1280 File: node/pkg/fetcher/fetcher.go:267-267 Timestamp: 2024-03-19T13:22:13.702Z Learning: Data insertion in the Orakl project is handled by `./core`, not `./node`, and references within `./api` are considered irrelevant to changes made in the PR context. This highlights the architectural separation between `./api`, `./core`, and `./node` within the project.
Description
Update fetcher structure based on recent update plan
Please refer to notion docs to see the whole picture
New players added to Fetcher package
Test updates
Minor update
FEED_DATA_STREAM_INTERVALwhich is default to 10sLRangeObjectType of change
Please delete options that are not relevant.
Checklist before requesting a review
Deployment