feat(stats): reset charts #1226
Conversation
WalkthroughThe pull request introduces a series of enhancements to the stats service, including a new HTTP rule for batch updates of charts via the Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (1)
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: 2
🧹 Nitpick comments (25)
stats/stats-server/src/runtime_setup.rs (2)
64-69: LGTM! Consider adding documentation.The implementation is clean and efficient. Consider adding documentation to describe the method's purpose and return value.
impl EnabledChartEntry { + /// Returns a vector of ChartKey instances for all resolutions in this chart entry. + /// Each ChartKey combines the resolution entry's name with its corresponding resolution key. pub fn get_keys(&self) -> Vec<ChartKey> { self.resolutions .iter() .map(|(res, entry)| ChartKey::new(entry.name.clone(), *res)) .collect() }
38-70: Consider caching the keys for better performance.Since the resolutions are immutable after construction, consider caching the keys in a
Vecfield to avoid recomputing them on each call toget_keys().#[derive(Debug)] pub struct EnabledChartEntry { pub settings: EnabledChartSettings, /// Static information presented as dynamic object pub resolutions: HashMap<ResolutionKind, EnabledResolutionEntry>, + /// Cached vector of chart keys for all resolutions + cached_keys: Option<Vec<ChartKey>>, } impl EnabledChartEntry { pub fn get_keys(&self) -> Vec<ChartKey> { + if let Some(keys) = &self.cached_keys { + return keys.clone(); + } self.resolutions .iter() .map(|(res, entry)| ChartKey::new(entry.name.clone(), *res)) .collect() } }stats/stats-server/src/update_service.rs (5)
29-33: Consider avoiding aMutexaround the channel.
Storing both the sender and receiver inMutexcan be cumbersome if only one task consumes messages. Typically, a single dedicated receiver doesn’t require locking. If multiple tasks need coordinated consumption, you might clarify how concurrency conflicts are resolved.
62-62: Channel buffer size is set at 128.
You may consider making it configurable or allocating performance-suited constants if the request volume is sometimes higher.
110-111: Error handling panics may be too abrupt.
Although typical for critical background tasks, consider a more graceful shutdown or an attempt to restart the service if applicable. Aborting may not always be best in production.Also applies to: 113-115, 117-119, 120-121
169-227: On-demand executor loop is well-structured, but watch out for concurrency overhead.
Relying on a locked channel receiver in a loop is functional, though unusual. If throughput grows, consider removing theMutexor controlling concurrency differently. Overall, the logic to repeatedly pick requests, handle them, and break gracefully upon channel closure is coherent.
347-358: Infinite loop for cron-based recurring updates.
Consider a graceful shutdown approach or a signal-based termination mechanism. Otherwise, the design is clear for continuous operation.stats/stats-server/tests/it/mock_blockscout_simple/stats_no_arbitrum.rs (1)
16-30: Consider replacing sleep with a more reliable wait mechanism.The 8-second sleep could make the test flaky. Consider implementing a retry mechanism or health check to wait for server initialization instead of using a fixed sleep duration.
Example implementation:
- // Sleep until server will start and calculate all values - sleep(Duration::from_secs(8)).await; + // Wait for server to be ready with retries + let max_retries = 10; + let retry_delay = Duration::from_secs(1); + for _ in 0..max_retries { + if let Ok(_) = reqwest::get(&format!("{}/health", base)).await { + break; + } + sleep(retry_delay).await; + }stats/stats-server/tests/it/mock_blockscout_simple/mod.rs (1)
17-34: Consider extracting the hardcoded date into a constant.The hardcoded date "2023-03-01" should be extracted into a constant or configuration value for better maintainability.
Example implementation:
+const TEST_MOCK_DATA_DATE: &str = "2023-03-01"; + #[tokio::test] #[ignore = "needs database"] async fn tests_with_mock_blockscout() { let test_name = "tests_with_mock_blockscout"; let blockscout_db = init_db_blockscout(test_name).await; - fill_mock_blockscout_data(&blockscout_db, NaiveDate::from_str("2023-03-01").unwrap()).await; + fill_mock_blockscout_data(&blockscout_db, NaiveDate::from_str(TEST_MOCK_DATA_DATE).unwrap()).await;stats/stats-server/src/auth.rs (2)
34-38: Enhance error message with more details.The error message "invalid api key" could be more descriptive to help users understand the issue better.
Example implementation:
- Status::unauthenticated("invalid api key") + Status::unauthenticated("Request not authorized: Invalid or missing API key in x-api-key header")
30-32: Optimize key lookup performance.The
is_key_authorizedmethod performs a linear search through values. Consider using the HashMap's key-based lookup instead.Example implementation:
- pub fn is_key_authorized(&self, api_key: &str) -> bool { - self.keys.values().find(|key| key.key.eq(api_key)).is_some() + pub fn is_key_authorized(&self, api_key: &str) -> bool { + self.keys.contains_key(api_key)stats/stats/src/data_source/kinds/data_manipulation/map/mod.rs (1)
62-68: Add documentation for the new method.The new method lacks documentation explaining its purpose and behavior. Consider adding documentation to maintain consistency with other methods in the trait.
Add documentation like this:
+ /// Sets the next update date for this data source. + /// + /// This is an adapter implementation that delegates the operation to its dependencies. + /// + /// # Arguments + /// * `db` - The database connection + /// * `update_from` - The date from which to set the next update async fn set_next_update_from_itself( _db: &DatabaseConnection, _update_from: chrono::NaiveDate, ) -> Result<(), ChartError> { // just an adapter; inner is handled recursively Ok(()) }stats/stats/src/data_source/kinds/auxiliary/cumulative.rs (1)
51-57: Add documentation for the new method.The new method should have documentation explaining its purpose and behavior, consistent with the well-documented style of the rest of the file.
Add documentation like this:
+ /// Sets the next update date for this cumulative data source. + /// + /// This is an adapter implementation that delegates the operation to its dependencies. + /// + /// # Arguments + /// * `db` - The database connection + /// * `update_from` - The date from which to set the next update async fn set_next_update_from_itself( _db: &DatabaseConnection, _update_from: chrono::NaiveDate, ) -> Result<(), ChartError> { // just an adapter; inner is handled recursively Ok(()) }stats/stats/src/data_source/kinds/data_manipulation/sum_point.rs (1)
57-63: Add documentation for the new method.The new method should have documentation explaining its purpose and behavior, maintaining consistency with the well-documented style of the file.
Add documentation like this:
+ /// Sets the next update date for this sum data source. + /// + /// This is an adapter implementation that delegates the operation to its dependencies. + /// + /// # Arguments + /// * `db` - The database connection + /// * `update_from` - The date from which to set the next update async fn set_next_update_from_itself( _db: &DatabaseConnection, _update_from: chrono::NaiveDate, ) -> Result<(), ChartError> { // just an adapter; inner is handled recursively Ok(()) }stats/stats-server/tests/it/mock_blockscout_simple/stats_not_updated.rs (1)
76-76: Remove trailing comma in vector literal.The trailing comma in the vector literal is unnecessary and inconsistent with other vector declarations.
- vec!["totalAddresses", "totalBlocks", "totalTxns",] + vec!["totalAddresses", "totalBlocks", "totalTxns"]stats/stats-server/tests/it/mock_blockscout_reindex.rs (1)
37-37: Consider making sleep durations configurable.The hardcoded sleep durations could be made configurable through settings or constants to make the tests more maintainable and adaptable to different environments.
+const BASE_SLEEP_DURATION: u64 = 8; +const REUPDATE_SLEEP_DURATION: u64 = 1; +const LONG_REUPDATE_SLEEP_DURATION: u64 = 2; - sleep(Duration::from_secs(8 * wait_multiplier)).await; + sleep(Duration::from_secs(BASE_SLEEP_DURATION * wait_multiplier)).await; - sleep(Duration::from_secs(1 * wait_multiplier)).await; + sleep(Duration::from_secs(REUPDATE_SLEEP_DURATION * wait_multiplier)).await; - sleep(Duration::from_secs(1 * wait_multiplier)).await; + sleep(Duration::from_secs(REUPDATE_SLEEP_DURATION * wait_multiplier)).await; - sleep(Duration::from_secs(2 * wait_multiplier)).await; + sleep(Duration::from_secs(LONG_REUPDATE_SLEEP_DURATION * wait_multiplier)).await;Also applies to: 68-68, 97-97, 126-126
stats/stats-server/tests/common.rs (2)
16-18: Consider making the test key name configurable.The hardcoded key name "test_key" could be made configurable or passed as a parameter for better flexibility in tests.
-pub fn setup_single_key(settings: &mut Settings, key: ApiKey) { - settings.api_keys = HashMap::from([("test_key".to_string(), key)]); +pub fn setup_single_key(settings: &mut Settings, key: ApiKey, key_name: &str) { + settings.api_keys = HashMap::from([(key_name.to_string(), key)]); }
148-151: Consider using standard library methods.The
sorted_vecfunction could be replaced with standard library methods.-pub fn sorted_vec<T: Ord>(mut v: Vec<T>) -> Vec<T> { - v.sort(); - v -} +pub fn sorted_vec<T: Ord>(v: Vec<T>) -> Vec<T> { + let mut sorted = v; + sorted.sort(); + sorted +}Alternatively, you could use:
pub fn sorted_vec<T: Ord>(v: Vec<T>) -> Vec<T> { let mut sorted = v.clone(); sorted.sort(); sorted }stats/stats-proto/proto/stats.proto (1)
113-118: BatchUpdateChartsRequest Message
The request message is well-defined withrepeated string chart_namesand optional fields forfrom(default being today) andupdate_later. Consider adding inline comments or documentation to further clarify default behaviors.stats/stats-proto/swagger/stats.swagger.yaml (1)
201-212: Swagger Request Schema: v1BatchUpdateChartsRequest
The request schema is properly structured with an array ofchart_names, along with optionalfrom(labeled with "Default is today") andupdate_laterfields. Consider clarifying the title forfromto make its purpose unambiguous.stats/stats/src/data_source/source.rs (1)
170-202: Add documentation for the new methods.The new methods
set_next_update_from_recursivelyandset_next_update_from_itselfwould benefit from documentation explaining:
- Their purpose and when they should be called
- The relationship between the recursive and non-recursive versions
- The meaning and impact of the
update_fromparameterstats/stats-server/src/read_service.rs (2)
43-44: Add documentation for the new fields.Consider adding documentation to explain:
- The purpose of the
authorizationandupdate_servicefields- Their lifecycle and thread-safety implications (why
Arcis used)
530-556: Enhance error handling for date parsing.Consider these improvements:
- Make the date format error message more specific by including the expected format
- Add validation for the date range to ensure it's reasonable (e.g., not too far in the past or future)
Apply this diff to improve the error message:
- Status::invalid_argument(format!("`from` should be a valid date: {}", e)) + Status::invalid_argument(format!( + "`from` should be a valid date in YYYY-MM-DD format: {}", + e + ))stats/stats/src/data_source/kinds/local_db/mod.rs (1)
267-291: Consider a more robust design for update tracking.The comment "make a proper separate table/column and use it if this approach brings some problems" suggests a potential design concern. Consider:
- Creating a dedicated table for update tracking now to avoid future migration complexity
- Adding columns to track update history, enabling rollback if needed
This would provide:
- Better separation of concerns
- More robust update tracking
- Easier maintenance and debugging
stats/stats/src/charts/db_interaction/read.rs (1)
517-518: Address the TODO comment about counter handling.The comment suggests reconsidering the handling of counters. Consider:
- Using dynamic dispatch to handle different chart types uniformly
- Adding a generic parameter to specialize behavior for counters
Would you like me to propose a design that addresses this concern?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
stats/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (46)
stats/stats-proto/proto/api_config_http.yaml(1 hunks)stats/stats-proto/proto/stats.proto(2 hunks)stats/stats-proto/swagger/stats.swagger.yaml(2 hunks)stats/stats-server/Cargo.toml(1 hunks)stats/stats-server/src/auth.rs(1 hunks)stats/stats-server/src/lib.rs(1 hunks)stats/stats-server/src/read_service.rs(4 hunks)stats/stats-server/src/runtime_setup.rs(1 hunks)stats/stats-server/src/server.rs(4 hunks)stats/stats-server/src/settings.rs(4 hunks)stats/stats-server/src/update_service.rs(8 hunks)stats/stats-server/tests/common.rs(3 hunks)stats/stats-server/tests/it/chart_endpoints/contracts_page.rs(0 hunks)stats/stats-server/tests/it/chart_endpoints/counters.rs(0 hunks)stats/stats-server/tests/it/chart_endpoints/lines.rs(0 hunks)stats/stats-server/tests/it/chart_endpoints/main_page.rs(0 hunks)stats/stats-server/tests/it/chart_endpoints/mod.rs(0 hunks)stats/stats-server/tests/it/chart_endpoints/transactions_page.rs(0 hunks)stats/stats-server/tests/it/main.rs(0 hunks)stats/stats-server/tests/it/mock_blockscout_reindex.rs(1 hunks)stats/stats-server/tests/it/mock_blockscout_simple/common_tests.rs(1 hunks)stats/stats-server/tests/it/mock_blockscout_simple/mod.rs(1 hunks)stats/stats-server/tests/it/mock_blockscout_simple/stats_full.rs(1 hunks)stats/stats-server/tests/it/mock_blockscout_simple/stats_no_arbitrum.rs(1 hunks)stats/stats-server/tests/it/mock_blockscout_simple/stats_not_indexed.rs(1 hunks)stats/stats-server/tests/it/mock_blockscout_simple/stats_not_updated.rs(3 hunks)stats/stats-server/tests/it/mod.rs(1 hunks)stats/stats-server/tests/it/swagger.rs(0 hunks)stats/stats-server/tests/main.rs(1 hunks)stats/stats/src/charts/chart.rs(1 hunks)stats/stats/src/charts/db_interaction/read.rs(3 hunks)stats/stats/src/data_source/kinds/auxiliary/cumulative.rs(1 hunks)stats/stats/src/data_source/kinds/data_manipulation/delta.rs(1 hunks)stats/stats/src/data_source/kinds/data_manipulation/filter_deducible.rs(3 hunks)stats/stats/src/data_source/kinds/data_manipulation/last_point.rs(1 hunks)stats/stats/src/data_source/kinds/data_manipulation/map/mod.rs(1 hunks)stats/stats/src/data_source/kinds/data_manipulation/resolutions/average.rs(1 hunks)stats/stats/src/data_source/kinds/data_manipulation/resolutions/last_value.rs(1 hunks)stats/stats/src/data_source/kinds/data_manipulation/resolutions/sum.rs(1 hunks)stats/stats/src/data_source/kinds/data_manipulation/sum_point.rs(1 hunks)stats/stats/src/data_source/kinds/local_db/mod.rs(7 hunks)stats/stats/src/data_source/kinds/remote_db/mod.rs(1 hunks)stats/stats/src/data_source/source.rs(3 hunks)stats/stats/src/data_source/tests.rs(2 hunks)stats/stats/src/tests/init_db.rs(1 hunks)stats/stats/src/update_group.rs(6 hunks)
💤 Files with no reviewable changes (8)
- stats/stats-server/tests/it/chart_endpoints/mod.rs
- stats/stats-server/tests/it/main.rs
- stats/stats-server/tests/it/chart_endpoints/transactions_page.rs
- stats/stats-server/tests/it/chart_endpoints/contracts_page.rs
- stats/stats-server/tests/it/chart_endpoints/counters.rs
- stats/stats-server/tests/it/swagger.rs
- stats/stats-server/tests/it/chart_endpoints/lines.rs
- stats/stats-server/tests/it/chart_endpoints/main_page.rs
✅ Files skipped from review due to trivial changes (2)
- stats/stats-server/tests/main.rs
- stats/stats-server/tests/it/mod.rs
🔇 Additional comments (70)
stats/stats-server/src/update_service.rs (20)
1-9: All new imports look aligned with usage.
No immediate concerns; the dependencies (cron, futures, itertools, etc.) appropriately match the functionality introduced.
14-17: Imports for BlockscoutMigrations, UpdateParameters, and ChartKey are coherent.
They are used consistently throughout, and there are no redundancy indications.Also applies to: 19-22
45-53: Helper function to retrieve group schedule is straightforward.
This is a simple and clean approach to fall back on the default schedule.
68-69: Duplicate concurrency design comment.
Applies the same rationale as the earlier mention about wrapping channels withMutex.
91-91: Retrieving the group’s schedule.
No issues detected; usage of.clone()is valid.
105-105: Initiating recurrent updates per group.
Implementation appears logically consistent with the group’s scheduling model.
109-109: Spawning the on-demand executor.
No issues; the approach is consistent with running a parallel processing task.
158-158: Force-full logic for initial update on start.
Implementation is aligned with the method signature and no hidden pitfalls are apparent.
229-283: Batch reupdate strategy looks solid.
It chooses the best matching group, optionally adjusts the from-date, and either defers or immediately runs an update. Code structure is clear and methodical.
285-310: Group selection by max intersecting members is straightforward.
Implementation adequately handles picking a suitable group.
312-335: Adjusting next update date for a chosen group.
Logic to override the reupdate date is consistent and validated with trace logs upon error.
337-341: Parameterization ofrun_recurrent_update.
No concerns about the new signature or usage.
360-365: Extended update function signature for partial chart updates.
Matches the flexible design of on-demand chart reupdates.
387-395: Switch toupdate_charts_syncfor concurrency.
Renaming is consistent with changes in theUpdateGroup.
406-435: Handling the incoming update request.
The logic for partitioning requests into an accepted subset and enqueuing them is straightforward.
437-462:split_update_request_inputeffectively distinguishes valid vs. invalid chart names.
Cleanly partitions the input into accepted and rejected.
465-470:OnDemandReupdateRequeststruct introduction.
No issues; naming is self-descriptive.
472-478:OnDemandReupdateErrorharnessesthiserror::Error.
Nice and concise error enumeration.
480-501:OnDemandReupdateAcceptedstruct is well-defined.
Converts to proto result cleanly.
503-506:Rejectionstruct is simple and clear.
Provides straightforward feedback for invalid chart names.stats/stats/src/update_group.rs (6)
38-38: AddedNaiveDateimport for reupdate logic.
No concerns here.
105-114: Newset_next_update_fromtrait method definition.
Extends the interface to reset chart data from a specified date. Implementation approach is consistent.
537-553: Renamed approach for enumerating enabled charts and dependencies.
Enhances clarity by returning both enabled members and the resolved dependency set.
555-570:lock_enabled_and_dependencieshelps avoid partial locking.
Acquires all required locks for the subset of charts in a single place, preventing partial updates from happening concurrently.
610-630:set_next_update_from_syncaddition.
Synchronizes the reupdate-from-date operation with other group updates. Good extension of concurrency control.
632-650:ChartsMutexGuardsstruct formalizes lock ownership.
Neatly contains the relevant locks plus the set of locked charts. Design is coherent.stats/stats-server/src/lib.rs (1)
1-1: Exposing theauthmodule publicly.
Logical addition, complements the new authorization logic introduced elsewhere.stats/stats/src/tests/init_db.rs (2)
3-7: LGTM! Good refactoring.The change improves code modularity by extracting the blockscout database initialization into a separate function.
13-15: LGTM! Clean implementation.The new function follows the same pattern as
init_dband maintains consistency in database initialization.stats/stats-server/tests/it/mock_blockscout_simple/stats_full.rs (1)
31-40: LGTM! Well-structured concurrent test execution.The use of
JoinSetfor concurrent test execution is a good approach. The tests cover essential functionality including lines, counters, main page, transactions, and contracts.stats/stats/src/data_source/kinds/data_manipulation/last_point.rs (1)
50-56: LGTM! Implementation follows the adapter pattern.The implementation correctly follows the adapter pattern, delegating the actual update logic to recursive handling. This maintains consistency with other data source implementations.
stats/stats/src/data_source/kinds/remote_db/mod.rs (1)
83-88: LGTM! Implementation aligns with stateless nature.The empty implementation returning
Ok(())is appropriate sinceRemoteDatabaseSourceis a stateless adapter for remote DB queries, as documented in the module comments.stats/stats-server/tests/it/mock_blockscout_simple/stats_not_updated.rs (1)
27-27: Verify reduced sleep duration.The sleep duration has been reduced from 2s to 1s. While the comment suggests this is due to "No update so no need to wait too long", please verify that 1 second is sufficient for the test to be reliable.
stats/stats/src/data_source/kinds/data_manipulation/delta.rs (1)
60-66: LGTM! Implementation follows the adapter pattern.The implementation correctly follows the adapter pattern, delegating the actual update logic to recursive handling. This maintains consistency with other data source implementations.
stats/stats/src/data_source/kinds/data_manipulation/resolutions/last_value.rs (1)
53-59: LGTM!The implementation of
set_next_update_from_itselfis consistent with the adapter pattern used throughout the struct, correctly delegating the responsibility to the inner data source.stats/stats-server/tests/it/mock_blockscout_reindex.rs (1)
144-153: LGTM!The
get_new_txnshelper function is well-implemented with clear filtering of zero values.stats/stats-server/tests/common.rs (1)
84-112: LGTM!The
send_request_with_keyfunction has robust error handling and clear error messages.stats/stats/src/data_source/kinds/data_manipulation/resolutions/sum.rs (1)
54-60: LGTM!The implementation of
set_next_update_from_itselfis consistent with the adapter pattern used throughout the struct, correctly delegating the responsibility to the inner data source.stats/stats-server/src/server.rs (4)
4-4: LGTM!The import of
AuthorizationProvideris correctly added to support the new authorization functionality.
113-113: LGTM!The chart creation method is correctly updated to use the synchronous version, aligning with the changes in other files.
134-137: LGTM!The update service is correctly cloned before being used in the async task to ensure proper ownership.
150-162: LGTM!The initialization of
ReadServiceis correctly updated to include the new dependencies:
update_servicefor handling chart updatesauthorizationfor API key validationstats/stats-server/tests/it/mock_blockscout_simple/stats_not_indexed.rs (4)
34-70: LGTM!The test setup is well-structured:
- Initializes test databases
- Sets up mock Blockscout API
- Configures API key authorization
- Runs multiple test cases concurrently
72-141: LGTM!The test correctly verifies:
- Health check status
- Empty chart data when Blockscout is not indexed
- Available counters match the expected list
143-157: LGTM!The test correctly verifies that the Swagger documentation matches the expected content.
159-182: LGTM!The test correctly verifies that unauthorized requests are rejected:
- Request without API key returns 401
- Request with incorrect API key returns 401
stats/stats/src/data_source/kinds/data_manipulation/filter_deducible.rs (3)
9-9: LGTM!The import is correctly updated to use
DbErrdirectly.
43-43: LGTM!The error type is correctly updated to use
DbErrdirectly, improving consistency.
53-59: LGTM!The new method is correctly implemented as a no-op adapter, maintaining the pattern used in other adapter methods.
stats/stats-server/tests/it/mock_blockscout_simple/common_tests.rs (5)
15-120: LGTM!The test thoroughly validates line charts:
- Verifies section IDs
- Checks enabled resolutions
- Validates chart data and info
- Handles optional resolution parameter
122-162: LGTM!The test thoroughly validates counters:
- Verifies non-empty descriptions and titles
- Checks counter order matches layout
164-211: LGTM!The test thoroughly validates main page stats:
- Handles optional Arbitrum support
- Verifies counter availability and content
- Validates window line charts
213-232: LGTM!The test thoroughly validates transaction page stats:
- Verifies all required counters are available
- Checks counter content
234-253: LGTM!The test thoroughly validates contract page stats:
- Verifies all required counters are available
- Checks counter content
stats/stats/src/data_source/tests.rs (2)
295-295: LGTM! Method name updated for consistency.The change from
create_charts_with_mutexestocreate_charts_syncaligns with the broader update to use more descriptive method names.
305-305: LGTM! Method name updated for consistency.The change from
update_charts_with_mutexestoupdate_charts_syncaligns with the broader update to use more descriptive method names.stats/stats/src/charts/chart.rs (1)
102-106: LGTM! Display trait implementation added.The implementation is clean and follows Rust's standard pattern by delegating to the existing
to_stringmethod.stats/stats-server/src/settings.rs (3)
15-17: LGTM! Dependencies updated for API key support.The imports are correctly updated to include HashMap and ApiKey for the new API key functionality.
Also applies to: 24-24
62-62: LGTM! API keys field added to Settings.The new field
api_keysis correctly typed asHashMap<String, ApiKey>for storing API key configurations.
85-85: LGTM! Default implementation updated.The default implementation correctly initializes the new
api_keysfield usingDefault::default().stats/stats/src/data_source/kinds/data_manipulation/resolutions/average.rs (1)
64-70: LGTM! Added adapter method for future functionality.The implementation correctly follows the pattern of other adapter methods in the file, with clear documentation about its purpose.
stats/stats-proto/proto/api_config_http.yaml (1)
18-20: New BatchUpdateCharts Endpoint Definition
The new HTTP rule mappingBatchUpdateChartsto a POST request at/api/v1/charts/batch-updatelooks correctly added with the request body set to"*". Ensure that this endpoint is documented consistently in your Swagger configuration.stats/stats-server/Cargo.toml (1)
21-21: New Dependency Added: thiserror
The addition ofthiserror = "1.0"supports improved error handling in the server implementation. Please verify that its usage is consistent with the rest of the error management strategy in the project.stats/stats-proto/proto/stats.proto (3)
14-15: New RPC Method: BatchUpdateCharts
The introduction of theBatchUpdateChartsRPC method is clear and follows standard proto conventions. Please ensure that its implementation in the server is updated accordingly.
120-123: BatchUpdateChartRejection Message
The rejection message is straightforward and includes the necessarynameandreasonfields.
125-130: BatchUpdateChartsResult Message Structure
The result message neatly provides total counts, accepted chart names, and detailed rejections. Ensure that the serialized data types and field formats match client expectations.stats/stats-proto/swagger/stats.swagger.yaml (3)
13-32: New Swagger Endpoint for BatchUpdateCharts
The new POST endpoint/api/v1/charts/batch-updateis well-documented with a clearoperationId, proper response definitions, and request body parameters referencing the correct schema. Confirm that these details remain consistent with the proto definitions.
194-200: Swagger Definition for BatchUpdateChartRejection
Thev1BatchUpdateChartRejectionobject is accurately defined withnameandreasonproperties.
213-230: Swagger Response Schema: v1BatchUpdateChartsResult
The response schema is comprehensive, detailing total counts, accepted chart names, and a rejection array that referencesv1BatchUpdateChartRejection. The use ofint64for totals is appropriate for handling large counts; please verify client compatibility.stats/stats/src/data_source/source.rs (1)
273-286: LGTM!The implementation for the unit type correctly provides the base case for recursion and follows the same pattern as other methods in the trait.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (11)
stats/stats-server/tests/common.rs (4)
16-18: Consider making the key name configurable.The function is well-structured, but consider making the key name a parameter instead of hardcoding "test_key" to improve reusability in tests.
-pub fn setup_single_key(settings: &mut Settings, key: ApiKey) { - settings.api_keys = HashMap::from([("test_key".to_string(), key)]); +pub fn setup_single_key(settings: &mut Settings, key: ApiKey, key_name: &str) { + settings.api_keys = HashMap::from([(key_name.to_string(), key)]); }
63-82: Consider adding support for delayed updates.The function is well-implemented but could be more flexible by allowing the caller to specify the
update_laterparameter.pub async fn request_reupdate_from( base: &Url, key: &ApiKey, from: &str, charts: Vec<&str>, + update_later: Option<bool>, ) -> proto_v1::BatchUpdateChartsResult { let chart_names = charts.into_iter().map(|s| s.to_string()).collect(); send_request_with_key( base, "/api/v1/charts/batch-update", reqwest::Method::POST, Some(&proto_v1::BatchUpdateChartsRequest { chart_names, from: Some(from.into()), - update_later: None, + update_later, }), key, ) .await }
84-112: Consider adding request timeout configuration.The function handles errors well but could benefit from configurable timeouts to prevent tests from hanging indefinitely.
pub async fn send_request_with_key<Response: for<'a> serde::Deserialize<'a>>( url: &Url, route: &str, method: reqwest::Method, payload: Option<&impl serde::Serialize>, key: &ApiKey, + timeout: Option<std::time::Duration>, ) -> Response { - let mut request = reqwest::Client::new().request(method, url.join(route).unwrap()); + let client = reqwest::Client::builder() + .timeout(timeout.unwrap_or_else(|| std::time::Duration::from_secs(30))) + .build() + .expect("Failed to build HTTP client"); + let mut request = client.request(method, url.join(route).unwrap()); if let Some(p) = payload { request = request.json(p); };
148-151: Add documentation for the utility function.The function is well-implemented but would benefit from documentation explaining its purpose and behavior.
+/// Returns a new vector containing the same elements in sorted order. +/// +/// # Arguments +/// * `v` - A vector of elements that implement the `Ord` trait +/// +/// # Returns +/// A new vector with the same elements sorted in ascending order pub fn sorted_vec<T: Ord>(mut v: Vec<T>) -> Vec<T> { v.sort(); v }stats/stats-server/tests/it/mock_blockscout_reindex.rs (4)
29-29: Consider using a test constant for the API key.Replace the hardcoded API key with a test constant to improve maintainability and make it clear this is a test-only value.
- let api_key = ApiKey::from_str_infallible("123"); + const TEST_API_KEY: &str = "123"; + let api_key = ApiKey::from_str_infallible(TEST_API_KEY);
37-37: Extract sleep durations into named constants.The sleep durations are magic numbers. Consider extracting them into named constants to improve readability and maintainability.
+ const INITIAL_WAIT_DURATION: u64 = 8; + const REUPDATE_WAIT_DURATION: u64 = 1; + const LONG_REUPDATE_WAIT_DURATION: u64 = 2; - sleep(Duration::from_secs(8 * wait_multiplier)).await; + sleep(Duration::from_secs(INITIAL_WAIT_DURATION * wait_multiplier)).await; - sleep(Duration::from_secs(1 * wait_multiplier)).await; + sleep(Duration::from_secs(REUPDATE_WAIT_DURATION * wait_multiplier)).await; - sleep(Duration::from_secs(1 * wait_multiplier)).await; + sleep(Duration::from_secs(REUPDATE_WAIT_DURATION * wait_multiplier)).await; - sleep(Duration::from_secs(2 * wait_multiplier)).await; + sleep(Duration::from_secs(LONG_REUPDATE_WAIT_DURATION * wait_multiplier)).await;Also applies to: 69-69, 99-99, 128-128
17-144: Consider splitting test scenarios into separate test functions.The test function contains multiple scenarios testing different aspects of the reupdate functionality. Consider splitting these into separate test functions for better maintainability and clarity:
- Test initial data load
- Test reupdate from 2023-01-01 for txnsGrowth
- Test reupdate from 2022-11-11 for newTxns
- Test reupdate from 2000-01-01 for newTxns
This would improve test isolation and make it easier to identify which scenario failed.
Also, enhance the comments to better describe what each test scenario is verifying:
- // should reindex newTxns transitively + // Test that updating txnsGrowth from 2023-01-01 transitively updates newTxns let reupdate_response = request_reupdate_from(&base, &api_key, "2023-01-01", vec!["txnsGrowth"]).await; - // wait to reupdate + // Allow time for the reupdate operation to complete sleep(Duration::from_secs(1 * wait_multiplier)).await;
146-155: Enhance helper function reusability and error handling.Consider the following improvements:
- Make
line_nameandresolutionparameters to improve reusability- Add error handling for the API call
-async fn get_new_txns(base: &Url) -> Vec<proto_v1::Point> { - let line_name = "newTxns"; - let resolution = "DAY"; +async fn get_new_txns( + base: &Url, + line_name: &str, + resolution: &str, +) -> Result<Vec<proto_v1::Point>, Box<dyn std::error::Error>> { let chart: proto_v1::LineChart = send_get_request( base, &format!("/api/v1/lines/{line_name}?resolution={resolution}"), ) - .await; - chart.chart.into_iter().filter(|p| p.value != "0").collect() + .await?; + Ok(chart.chart.into_iter().filter(|p| p.value != "0").collect()) }Update the call sites:
- let data = get_new_txns(&base).await; + let data = get_new_txns(&base, "newTxns", "DAY") + .await + .expect("Failed to get new transactions");stats/stats-server/src/update_service.rs (3)
31-32: Consider configurable channel capacity.The hardcoded channel capacity of 128 might be a bottleneck in high-load scenarios. Consider making it configurable through the service configuration.
pub struct UpdateService { db: Arc<DatabaseConnection>, blockscout_db: Arc<DatabaseConnection>, charts: Arc<RuntimeSetup>, status_listener: Option<IndexingStatusListener>, + on_demand_channel_capacity: usize, on_demand_sender: Mutex<mpsc::Sender<OnDemandReupdateRequest>>, on_demand_receiver: Mutex<mpsc::Receiver<OnDemandReupdateRequest>>, } impl UpdateService { pub async fn new( db: Arc<DatabaseConnection>, blockscout_db: Arc<DatabaseConnection>, charts: Arc<RuntimeSetup>, status_listener: Option<IndexingStatusListener>, + on_demand_channel_capacity: Option<usize>, ) -> Result<Self, DbErr> { - let on_demand = mpsc::channel(128); + let capacity = on_demand_channel_capacity.unwrap_or(128); + let on_demand = mpsc::channel(capacity); Ok(Self { db, blockscout_db, charts, status_listener, + on_demand_channel_capacity: capacity, on_demand_sender: Mutex::new(on_demand.0), on_demand_receiver: Mutex::new(on_demand.1), }) }
469-475: Enhance error handling with more context.The error enum could be more descriptive to aid in debugging and monitoring. Consider adding more specific error variants and context.
#[derive(Error, Debug)] pub enum OnDemandReupdateError { #[error("All provided chart names were not found")] AllChartsNotFound, + #[error("Chart update failed: {reason}")] + UpdateFailed { reason: String }, + #[error("Rate limit exceeded: {current}/{limit} updates per {window_secs}s")] + RateLimitExceeded { + current: u32, + limit: u32, + window_secs: u32, + }, #[error("internal error")] Internal, }
483-498: Add metrics for batch update results.Consider adding metrics to track the success rate and performance of batch updates for better observability.
pub fn into_update_result(self) -> proto_v1::BatchUpdateChartsResult { + // Record metrics for monitoring + metrics::gauge!("batch_update_total_charts", self.accepted.len() as f64 + self.rejected.len() as f64); + metrics::gauge!("batch_update_rejected_charts", self.rejected.len() as f64); + metrics::gauge!("batch_update_success_rate", + (self.accepted.len() as f64 / (self.accepted.len() + self.rejected.len()) as f64) * 100.0 + ); + proto_v1::BatchUpdateChartsResult { total: (self.accepted.len() + self.rejected.len()) as u32, total_rejected: self.rejected.len() as u32,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
stats/stats-server/src/auth.rs(1 hunks)stats/stats-server/src/update_service.rs(8 hunks)stats/stats-server/tests/common.rs(3 hunks)stats/stats-server/tests/it/mock_blockscout_reindex.rs(1 hunks)stats/stats-server/tests/it/mock_blockscout_simple/stats_not_indexed.rs(1 hunks)stats/stats/src/update_group.rs(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- stats/stats-server/src/auth.rs
- stats/stats-server/tests/it/mock_blockscout_simple/stats_not_indexed.rs
- stats/stats/src/update_group.rs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Unit, doc and integration tests
🔇 Additional comments (2)
stats/stats-server/tests/common.rs (1)
7-11: LGTM!The new imports are correctly organized and necessary for the added functionality.
stats/stats-server/tests/it/mock_blockscout_reindex.rs (1)
1-16: LGTM!The imports are well-organized and appropriately scoped for the test requirements.
bragov4ik
force-pushed
the
bragov4ik/stats-reset-charts
branch
from
1a91564 to
05cdfec
Compare
to avoid repeated slow initializations + organize it w/o contradictions
bragov4ik
force-pushed
the
bragov4ik/stats-reset-charts
branch
from
05cdfec to
6a9ba4a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
stats/stats-server/tests/it/mock_blockscout_simple/common_tests.rs (2)
15-120: Consider improving error messages and modularity in test_lines_ok.While the test logic is sound, consider these improvements:
- Extract the list of line names into a constant or configuration.
- Make error messages more descriptive by including actual vs expected values.
Example improvement for error messages:
-assert!( - !chart_data.is_empty(), - "chart data for '{line_name}' '{resolution}' is empty" -); +assert!( + !chart_data.is_empty(), + "chart data for '{line_name}' with resolution '{resolution}' is empty. Expected non-empty data but got empty array." +);
122-163: Consider extracting counter validation logic in test_counters_ok.The counter validation logic could be extracted into a helper function for better reusability across test functions.
Example refactor:
+fn validate_counter(counter: &Counter, name: &str) { + assert!(!counter.description.is_empty(), "Counter '{}' has empty description", name); + assert!(!counter.title.is_empty(), "Counter '{}' has empty title", name); +} pub async fn test_counters_ok(base: Url) { let counters: Counters = send_get_request(&base, "/api/v1/counters").await; for counter in counters.counters.iter() { - assert!(!counter.description.is_empty()); - assert!(!counter.title.is_empty()); + validate_counter(counter, &counter.id); }stats/stats-server/src/update_service.rs (2)
166-224: Consider adding metrics for on-demand updates.The on-demand executor would benefit from metrics tracking:
- Number of requests processed
- Processing time
- Success/failure rates
Would you like me to suggest a metrics implementation using the existing metrics system?
469-476: Consider adding more specific error variants.The error enum could be expanded to provide more detailed error information:
- Invalid date range
- Rate limiting
- Service unavailable
Would you like me to suggest additional error variants?
stats/stats-server/src/read_service.rs (1)
536-562: Enhance error handling and date validation.The implementation looks good but could benefit from additional validation and error handling:
- Consider validating that the
chart_namesarray is not empty to avoid unnecessary processing.- Consider adding an upper bound validation for the
fromdate to prevent querying too far in the past.Apply this diff to add the suggested validations:
let request = request.into_inner(); + if request.chart_names.is_empty() { + return Err(Status::invalid_argument("chart_names cannot be empty")); + } let from = request .from .map(|s| NaiveDate::from_str(&s)) .transpose() .map_err(|e| { Status::invalid_argument(format!("`from` should be a valid date: {}", e)) })?; + if let Some(from_date) = from { + let max_days = 365; // Adjust this value based on your requirements + let today = Utc::now().date_naive(); + if (today - from_date).num_days() > max_days { + return Err(Status::invalid_argument( + format!("from date cannot be more than {} days in the past", max_days) + )); + } + }stats/stats-proto/proto/stats.proto (1)
114-131: Add more descriptive field documentation.While the message definitions are correct, consider adding more descriptive comments for the following fields:
chart_names: Explain the expected format and any limitations.update_later: Document the behavior when this flag is set.total_rejected: Clarify what causes a chart update to be rejected.Apply this diff to add the suggested documentation:
message BatchUpdateChartsRequest { + // List of chart names to update. Must be valid chart identifiers. repeated string chart_names = 1; // Default is today optional string from = 2; + // When true, updates will be queued and processed asynchronously. + // When false, updates will be processed immediately. optional bool update_later = 3; } message BatchUpdateChartRejection { string name = 1; + // Detailed explanation of why the chart update was rejected. + // Common reasons: chart not found, invalid date range, etc. string reason = 2; } message BatchUpdateChartsResult { uint32 total = 1; + // Number of charts that could not be updated. + // Increases when charts are not found or when updates fail. uint32 total_rejected = 2; repeated string accepted = 3; repeated BatchUpdateChartRejection rejected = 4; }stats/stats-proto/swagger/stats.swagger.yaml (1)
194-230: Add examples to schema definitions.Consider adding examples to help API consumers understand the expected request/response format.
Add examples to the schema definitions:
v1BatchUpdateChartsRequest: type: object + example: + chart_names: ["new_txns_24h", "total_blocks"] + from: "2025-01-01" + update_later: false properties: chart_names: type: array items: type: string v1BatchUpdateChartsResult: type: object + example: + total: 2 + total_rejected: 1 + accepted: ["new_txns_24h"] + rejected: + - name: "total_blocks" + reason: "chart not found" properties:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
stats/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (45)
stats/stats-proto/proto/api_config_http.yaml(1 hunks)stats/stats-proto/proto/stats.proto(2 hunks)stats/stats-proto/swagger/stats.swagger.yaml(2 hunks)stats/stats-server/Cargo.toml(1 hunks)stats/stats-server/src/auth.rs(1 hunks)stats/stats-server/src/blockscout_waiter.rs(1 hunks)stats/stats-server/src/lib.rs(1 hunks)stats/stats-server/src/read_service.rs(4 hunks)stats/stats-server/src/runtime_setup.rs(1 hunks)stats/stats-server/src/server.rs(4 hunks)stats/stats-server/src/settings.rs(4 hunks)stats/stats-server/src/update_service.rs(8 hunks)stats/stats-server/tests/common.rs(3 hunks)stats/stats-server/tests/it/chart_endpoints/contracts_page.rs(0 hunks)stats/stats-server/tests/it/chart_endpoints/lines.rs(0 hunks)stats/stats-server/tests/it/chart_endpoints/main_page.rs(0 hunks)stats/stats-server/tests/it/chart_endpoints/mod.rs(5 hunks)stats/stats-server/tests/it/main.rs(0 hunks)stats/stats-server/tests/it/mock_blockscout_reindex.rs(1 hunks)stats/stats-server/tests/it/mock_blockscout_simple/common_tests.rs(1 hunks)stats/stats-server/tests/it/mock_blockscout_simple/mod.rs(1 hunks)stats/stats-server/tests/it/mock_blockscout_simple/stats_full.rs(1 hunks)stats/stats-server/tests/it/mock_blockscout_simple/stats_no_arbitrum.rs(1 hunks)stats/stats-server/tests/it/mock_blockscout_simple/stats_not_indexed.rs(1 hunks)stats/stats-server/tests/it/mock_blockscout_simple/stats_not_updated.rs(3 hunks)stats/stats-server/tests/it/mod.rs(1 hunks)stats/stats-server/tests/it/swagger.rs(0 hunks)stats/stats-server/tests/main.rs(1 hunks)stats/stats/src/charts/chart.rs(1 hunks)stats/stats/src/charts/db_interaction/read.rs(3 hunks)stats/stats/src/data_source/kinds/auxiliary/cumulative.rs(1 hunks)stats/stats/src/data_source/kinds/data_manipulation/delta.rs(1 hunks)stats/stats/src/data_source/kinds/data_manipulation/filter_deducible.rs(3 hunks)stats/stats/src/data_source/kinds/data_manipulation/last_point.rs(1 hunks)stats/stats/src/data_source/kinds/data_manipulation/map/mod.rs(1 hunks)stats/stats/src/data_source/kinds/data_manipulation/resolutions/average.rs(1 hunks)stats/stats/src/data_source/kinds/data_manipulation/resolutions/last_value.rs(1 hunks)stats/stats/src/data_source/kinds/data_manipulation/resolutions/sum.rs(1 hunks)stats/stats/src/data_source/kinds/data_manipulation/sum_point.rs(1 hunks)stats/stats/src/data_source/kinds/local_db/mod.rs(7 hunks)stats/stats/src/data_source/kinds/remote_db/mod.rs(1 hunks)stats/stats/src/data_source/source.rs(3 hunks)stats/stats/src/data_source/tests.rs(2 hunks)stats/stats/src/tests/init_db.rs(1 hunks)stats/stats/src/update_group.rs(6 hunks)
💤 Files with no reviewable changes (5)
- stats/stats-server/tests/it/main.rs
- stats/stats-server/tests/it/swagger.rs
- stats/stats-server/tests/it/chart_endpoints/contracts_page.rs
- stats/stats-server/tests/it/chart_endpoints/main_page.rs
- stats/stats-server/tests/it/chart_endpoints/lines.rs
🚧 Files skipped from review as they are similar to previous changes (28)
- stats/stats-server/src/lib.rs
- stats/stats/src/data_source/kinds/data_manipulation/map/mod.rs
- stats/stats-server/tests/it/mock_blockscout_simple/mod.rs
- stats/stats/src/data_source/kinds/data_manipulation/sum_point.rs
- stats/stats/src/data_source/kinds/auxiliary/cumulative.rs
- stats/stats/src/data_source/kinds/data_manipulation/resolutions/average.rs
- stats/stats/src/data_source/kinds/data_manipulation/resolutions/sum.rs
- stats/stats-server/tests/it/mock_blockscout_reindex.rs
- stats/stats-server/tests/it/mod.rs
- stats/stats-server/src/server.rs
- stats/stats-server/tests/it/mock_blockscout_simple/stats_not_indexed.rs
- stats/stats/src/data_source/kinds/data_manipulation/last_point.rs
- stats/stats-server/tests/common.rs
- stats/stats-server/src/auth.rs
- stats/stats-server/Cargo.toml
- stats/stats-server/tests/main.rs
- stats/stats/src/charts/chart.rs
- stats/stats-proto/proto/api_config_http.yaml
- stats/stats-server/src/runtime_setup.rs
- stats/stats/src/data_source/tests.rs
- stats/stats/src/data_source/kinds/remote_db/mod.rs
- stats/stats/src/tests/init_db.rs
- stats/stats/src/charts/db_interaction/read.rs
- stats/stats/src/update_group.rs
- stats/stats/src/data_source/kinds/local_db/mod.rs
- stats/stats/src/data_source/source.rs
- stats/stats/src/data_source/kinds/data_manipulation/delta.rs
- stats/stats-server/tests/it/mock_blockscout_simple/stats_full.rs
🧰 Additional context used
📓 Learnings (4)
stats/stats-server/src/update_service.rs (1)
Learnt from: bragov4ik
PR: blockscout/blockscout-rs#1226
File: stats/stats-server/src/update_service.rs:110-119
Timestamp: 2025-02-04T10:10:46.118Z
Learning: Graceful shutdown mechanisms for services in blockscout-rs are being implemented in dedicated PRs (#1128, #1129) using TaskTracker, JoinSet, and CancellationToken, rather than being handled individually in each service.
stats/stats-server/tests/it/mock_blockscout_simple/stats_no_arbitrum.rs (1)
Learnt from: bragov4ik
PR: blockscout/blockscout-rs#1226
File: stats/stats-server/tests/it/mock_blockscout_simple/stats_full.rs:0-0
Timestamp: 2025-02-04T10:08:27.299Z
Learning: In stats-server tests, waiting for server initialization requires not just checking server health but also ensuring all chart values have been calculated and initialized.
stats/stats-server/tests/it/mock_blockscout_simple/stats_not_updated.rs (1)
Learnt from: bragov4ik
PR: blockscout/blockscout-rs#1226
File: stats/stats-server/tests/it/mock_blockscout_simple/stats_full.rs:0-0
Timestamp: 2025-02-04T10:08:27.299Z
Learning: In stats-server tests, waiting for server initialization requires not just checking server health but also ensuring all chart values have been calculated and initialized.
stats/stats-server/tests/it/chart_endpoints/mod.rs (1)
Learnt from: bragov4ik
PR: blockscout/blockscout-rs#1226
File: stats/stats-server/tests/it/mock_blockscout_simple/stats_full.rs:0-0
Timestamp: 2025-02-04T10:08:27.299Z
Learning: In stats-server tests, waiting for server initialization requires not just checking server health but also ensuring all chart values have been calculated and initialized.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Unit, doc and integration tests
🔇 Additional comments (15)
stats/stats-server/tests/it/mock_blockscout_simple/stats_no_arbitrum.rs (2)
1-16: LGTM! Clear documentation and appropriate imports.The file documentation clearly states the test conditions, and all imports are properly utilized in the implementation.
17-26: LGTM! Well-structured test implementation.The test function follows good practices:
- Uses proper test isolation with unique test name
- Runs tests concurrently for efficiency
- Properly cleans up resources
Also applies to: 29-37
stats/stats-server/tests/it/mock_blockscout_simple/stats_not_updated.rs (2)
1-30: LGTM! Well-structured test setup.The test setup is clear and follows best practices. The function
run_tests_with_charts_not_updatedcorrectly initializes the test environment and disables updates on startup.
32-78: LGTM! Comprehensive test coverage.The test function
test_lines_counters_not_updated_okthoroughly verifies:
- Server health check response
- Empty chart data during update period
- Counters with fallback query logic
stats/stats/src/data_source/kinds/data_manipulation/resolutions/last_value.rs (1)
53-59: LGTM! Consistent adapter implementation.The implementation of
set_next_update_from_itselffollows the same pattern as other adapter methods in the file, correctly passing through to inner implementations.stats/stats/src/data_source/kinds/data_manipulation/filter_deducible.rs (1)
53-59: LGTM! Consistent adapter implementation.The implementation of
set_next_update_from_itselffollows the same pattern as other adapter methods in the file, correctly passing through to inner implementations.stats/stats-server/tests/it/mock_blockscout_simple/common_tests.rs (1)
165-212: LGTM! Well-structured page statistics tests.The test functions for page statistics (
test_main_page_ok,test_transactions_page_ok,test_contracts_page_ok) are well-structured and provide good coverage.Also applies to: 214-240, 242-261
stats/stats-server/tests/it/chart_endpoints/mod.rs (2)
50-63: LGTM! Well-structured test consolidation.The test consolidation using
JoinSetimproves test efficiency by running multiple tests concurrently while maintaining clear test boundaries.
144-238: Verify test coverage for edge cases.The test verifies basic reupdate functionality but could benefit from additional edge cases:
- Invalid API keys
- Concurrent reupdate requests
- Reupdate with invalid dates
Would you like me to suggest additional test cases to improve coverage?
stats/stats-server/src/settings.rs (1)
62-62: LGTM! Clean API key integration.The
api_keysfield is appropriately typed asHashMap<String, ApiKey>for efficient key lookup.stats/stats-server/src/blockscout_waiter.rs (1)
358-359: LGTM! Improved test determinism.Setting a specific timeout of 500ms instead of None makes the test more deterministic and helps catch performance regressions.
stats/stats-server/src/update_service.rs (2)
31-32: LGTM! Thread-safe channel implementation.The use of
Mutexfor the channel endpoints ensures thread-safe communication for on-demand updates.
403-432: LGTM! Robust error handling for update requests.The error handling is comprehensive, properly categorizing and propagating errors while maintaining type safety.
stats/stats-server/src/read_service.rs (1)
44-45: LGTM! Thread-safe field additions.The new fields are correctly wrapped in
Arcfor thread-safe sharing and properly initialized in the constructor.Also applies to: 54-55, 62-63
stats/stats-proto/proto/stats.proto (1)
14-14: LGTM! RPC method definition.The RPC method is correctly defined following proto3 syntax and matches the implementation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
stats/justfile (1)
14-15: Add documentation for the new target.The target implementation looks good, but would benefit from a comment explaining its purpose and usage examples.
Add documentation above the target:
+# Builds Docker image with custom arguments +# Usage: just docker-build --target release docker-build *args: docker build --build-context proto=../proto . {{args}}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/stats.yml(1 hunks)stats/Dockerfile(1 hunks)stats/justfile(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/stats.yml
92-92: input "build-contexts" is not defined in "./.github/workflows/_docker-build-push.yml" reusable workflow. defined inputs are "docker-context-path", "dockerfile-path", "organization", "registry", "service-name"
(workflow-call)
🔇 Additional comments (3)
stats/Dockerfile (1)
13-14: Ensure Build Context for Proto Definitions
The addition of the comment and theCOPY --from=proto . /protocommand introduces a dependency on a build stage namedproto. Please ensure that the build process supplies this stage (for example, by passing the appropriate build context or multi-stage target) so that the proto definitions are available during the build..github/workflows/stats.yml (1)
92-93:⚠️ Potential issueMismatch in Reusable Workflow Input for
build-contextsThe new key
build-contextsis added here to pass additional build context (i.e.,proto=./proto), but static analysis indicates that this input is not defined in the reusable workflow (./.github/workflows/_docker-build-push.yml). To avoid potential build failures, please update the reusable workflow to declare and handle thebuild-contextsinput or modify the calling workflow to use one of the inputs that is already defined (for example,docker-context-pathif that serves the same purpose).If the intention is to support multiple build contexts, you might update the reusable workflow's input definitions as follows:
+inputs: + build-contexts: + description: "Comma-separated or newline-separated build contexts (e.g., proto=./proto)." + required: false✅ Verification successful
🏁 Script executed:
#!/bin/bash echo "Searching for 'build-contexts' definitions in the reusable workflow (_docker-build-push.yml)..." rg "build-contexts:" -n .github/workflows/_docker-build-push.yml || echo "No 'build-contexts' input defined in _docker-build-push.yml"Length of output: 302
Reusable workflow already defines the
build-contextsinput.The
_docker-build-push.ymlfile contains a declaration forbuild-contexts(evidenced by the matching line at 96), so the calling workflow instats.ymlis correctly providing an input that the reusable workflow now accepts.🧰 Tools
🪛 actionlint (1.7.4)
92-92: input "build-contexts" is not defined in "./.github/workflows/_docker-build-push.yml" reusable workflow. defined inputs are "docker-context-path", "dockerfile-path", "organization", "registry", "service-name"
(workflow-call)
stats/justfile (1)
14-15: Verify proto directory exists before building.The build context path assumes the proto directory exists at
../proto. Consider adding a validation step.Add directory validation:
docker-build *args: + #!/bin/bash + if [ ! -d "../proto" ]; then + echo "Error: Proto directory not found at ../proto" + exit 1 + fi docker build --build-context proto=../proto . {{args}}✅ Verification successful
🏁 Script executed:
#!/bin/bash # Check if the 'proto' directory exists in the repository root, # which is expected to be located at ../proto relative to stats/justfile. if [ -d "proto" ]; then echo "Success: 'proto' directory exists in the repository root." else echo "Error: 'proto' directory does not exist in the repository root." fiLength of output: 149
Directory Check Verified
The shell script confirms that theprotodirectory exists in the repository root, which aligns with the enforced build context. While the directory exists, incorporating an explicit existence check (as suggested) can provide additional robustness for the build process.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
stats/stats-server/src/update_service.rs (1)
60-60: Consider making the channel size configurable.The hardcoded channel size of 128 might need adjustment based on load patterns. Consider making it configurable through environment variables or configuration files.
- let on_demand = mpsc::channel(128); + let channel_size = std::env::var("ON_DEMAND_CHANNEL_SIZE") + .map(|s| s.parse().unwrap_or(128)) + .unwrap_or(128); + let on_demand = mpsc::channel(channel_size);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
stats/stats-server/src/auth.rs(1 hunks)stats/stats-server/src/runtime_setup.rs(1 hunks)stats/stats-server/src/update_service.rs(8 hunks)stats/stats-server/tests/it/chart_endpoints/counters.rs(0 hunks)stats/stats-server/tests/it/chart_endpoints/mod.rs(0 hunks)stats/stats-server/tests/it/chart_endpoints/transactions_page.rs(0 hunks)stats/stats-server/tests/it/mock_blockscout_simple/stats_not_indexed.rs(1 hunks)stats/stats/src/charts/db_interaction/read.rs(3 hunks)stats/stats/src/data_source/kinds/local_db/mod.rs(8 hunks)stats/stats/src/data_source/source.rs(3 hunks)
💤 Files with no reviewable changes (3)
- stats/stats-server/tests/it/chart_endpoints/transactions_page.rs
- stats/stats-server/tests/it/chart_endpoints/mod.rs
- stats/stats-server/tests/it/chart_endpoints/counters.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- stats/stats-server/src/runtime_setup.rs
- stats/stats-server/tests/it/mock_blockscout_simple/stats_not_indexed.rs
- stats/stats/src/charts/db_interaction/read.rs
- stats/stats-server/src/auth.rs
- stats/stats/src/data_source/kinds/local_db/mod.rs
- stats/stats/src/data_source/source.rs
🧰 Additional context used
🧠 Learnings (1)
stats/stats-server/src/update_service.rs (1)
Learnt from: bragov4ik
PR: blockscout/blockscout-rs#1226
File: stats/stats-server/src/update_service.rs:110-119
Timestamp: 2025-02-04T10:10:46.118Z
Learning: Graceful shutdown mechanisms for services in blockscout-rs are being implemented in dedicated PRs (#1128, #1129) using TaskTracker, JoinSet, and CancellationToken, rather than being handled individually in each service.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Linting / lint
- GitHub Check: Unit, doc and integration tests
🔇 Additional comments (4)
stats/stats-server/src/update_service.rs (4)
110-119: LGTM! Temporary error handling until graceful shutdown is implemented.The panic is intentional as a temporary solution. Graceful shutdown is being implemented in PRs #1128 and #1129 using TaskTracker, JoinSet, and CancellationToken.
167-225: LGTM! Robust implementation with proper error handling.The implementation includes:
- Comprehensive error handling and logging
- Proper safeguards against infinite loops
- Correct handling of empty update lists as bugs
404-433: LGTM! Secure and robust request handling.The implementation includes proper error handling for both invalid chart names and channel closure. Since this is an admin-only endpoint, the implementation correctly focuses on functionality without unnecessary restrictions.
435-504: LGTM! Well-structured types with proper error handling.The implementation includes:
- Clear and documented error types
- Comprehensive helper structs
- Correct proto conversion implementation
..maybe also add possibility to do the reupdate with env var if api keys are too much of a hassle?
Summary by CodeRabbit
New Features
Bug Fixes
Documentation