feat(stats): The API of the Backend charts migration #1167
Conversation
|
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 WalkthroughThis pull request introduces significant enhancements to the stats service by expanding its API capabilities and restructuring chart and endpoint configurations. The changes primarily focus on adding new endpoints for main page, transactions page, and contracts page statistics. The modifications include updates to configuration files, proto definitions, server-side logic, and test suites. Key changes involve introducing new RPC methods, HTTP rules, and message types to support more comprehensive statistics retrieval across different blockchain metrics. The implementation ensures that various counters and line charts can be fetched with improved clarity and structure. Possibly related PRs
Poem
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 (
|
bragov4ik
force-pushed
the
bragov4ik/stats-migrate-backend-charts-8-api
branch
from
277ea50 to
68b7449
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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
🧹 Nitpick comments (18)
stats/stats-proto/proto/stats.proto (3)
36-37: Deprecated message name caution.
GetLineChartsRequestremains empty; if planning to remove it, consider deprecating or removing it outright to avoid confusion.
38-40: Clarify in-source TODO references.
The TODO comment references a PR for enum usage. Consider creating a tracking issue or ensuring the comment remains up to date.
79-79: Empty request for main page stats.
Having an empty request message is acceptable, but consider if future expansions might add filtering fields here.stats/stats-server/src/read_service.rs (6)
27-27: Add doc forTimespanusage.
Might be beneficial to add code-level documentation clarifying the difference betweenTimespanand simpler date ranges.
29-29: Edge case handling in error definitions.
ChartErrorenumerates typical error scenarios. Consider verifying if all custom error variants are exposed or processed.
32-32: Parallel operations.
The use oftokio::joinin your code to gather counters is efficient. Confirm that concurrency doesn't cause DB load spikes in a highly loaded environment.
230-230: Potential naming refinement.
id: namemight benefit from consistent naming across code to reduce confusion.
367-367: Collect counters asynchronously.
Line 367 sets up future iteration. Confirm memory usage if the set grows large.
389-405: Inclusive date range usage in line chart retrieval.
Building the request_range fromfromandtois flexible. Consider validating user inputs if the API eventually allows custom date queries.stats/stats-server/tests/it/chart_endpoints/counters.rs (1)
7-12: Validate counter IDs as well.Currently, the test checks that each counter's description and title are non-empty. It might be beneficial to include an assertion that
counter.idis not empty as well, ensuring full coverage in this test case.stats/stats-server/tests/it/chart_endpoints/main_page.rs (1)
11-40: Expand test coverage for main page fields.The assertions effectively verify that each returned field is present and has a non-empty description and title. As an extra measure, consider verifying numeric fields (e.g.,
total_blocks,yesterday_txns) for expected ranges or values (e.g., non-negative). This can help catch data anomalies earlier.stats/stats-server/src/config/read/layout.rs (1)
51-54: Potential diagnostic logging for excluded items.Consider logging a diagnostic message for items whose keys are not found in
layout. This can help troubleshoot configuration mismatches without complicating the result set.stats/stats-server/tests/it/swagger.rs (1)
34-34: Move from fixed sleep to readiness check.Increasing the sleep duration from 5 to 7 seconds may still be insufficient under heavy load or slower environments. Consider implementing a readiness check (e.g., polling an endpoint until the server is ready) to reduce test flakiness and speed up the suite where possible.
stats/stats-server/tests/it/chart_endpoints/mod.rs (2)
1-7: Add clarity to the top-level documentation.The doc comments are helpful in explaining why all chart tests run in a single test function. However, consider adding a short paragraph about how this structure impacts test isolation and runtime to provide further clarity for future maintainers.
66-78: Include verbose error outputs for debugging.When a test fails, you log the error and set
some_failed = true. Consider adding more contextual data in the fail message (for example, which test specifically failed) to facilitate quicker debugging.- println!("test for chart endpoint ... fail\n{}", e); + println!("test for chart endpoint failed: {e:?}");stats/stats-server/tests/it/chart_endpoints/lines.rs (1)
Line range hint
36-54: Ensure consistent coverage for commented-out charts.Many charts are commented out (e.g.,
activeRecurringAccounts60Days). If these charts are expected to be tested eventually, consider adding a TODO comment or referencing a future issue. Otherwise, remove them if they're no longer relevant.stats/stats-server/src/config/types.rs (1)
203-203: Confirm new call usage forplaced_items_according_to_layout.Please verify that the arguments and return type align with the updated logic. If the newly placed items should preserve the previous sorting guarantees, consider adding a test to confirm correct item placement for typical and edge-case scenarios.
Would you like me to propose a test snippet to ensure correct behavior?
stats/config/charts.json (1)
142-142: Maintain consistent description format while preserving clarity.While the new description provides better clarity about the time window, it deviates from the concise format used in other chart descriptions.
Consider this format to maintain consistency:
- "description": "The chart displays daily transactions for the past 30 days." + "description": "Daily transactions over past 30 days"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
stats/config/charts.json(1 hunks)stats/config/layout.json(0 hunks)stats/stats-proto/proto/api_config_http.yaml(1 hunks)stats/stats-proto/proto/stats.proto(3 hunks)stats/stats-proto/swagger/stats.swagger.yaml(4 hunks)stats/stats-server/src/config/read/layout.rs(2 hunks)stats/stats-server/src/config/types.rs(2 hunks)stats/stats-server/src/read_service.rs(9 hunks)stats/stats-server/src/runtime_setup.rs(4 hunks)stats/stats-server/tests/it/chart_endpoints/contracts_page.rs(1 hunks)stats/stats-server/tests/it/chart_endpoints/counters.rs(1 hunks)stats/stats-server/tests/it/chart_endpoints/lines.rs(1 hunks)stats/stats-server/tests/it/chart_endpoints/main_page.rs(1 hunks)stats/stats-server/tests/it/chart_endpoints/mod.rs(2 hunks)stats/stats-server/tests/it/chart_endpoints/transactions_page.rs(1 hunks)stats/stats-server/tests/it/common.rs(2 hunks)stats/stats-server/tests/it/indexing_status.rs(2 hunks)stats/stats-server/tests/it/main.rs(1 hunks)stats/stats-server/tests/it/swagger.rs(1 hunks)stats/stats/src/charts/lines/mod.rs(1 hunks)
💤 Files with no reviewable changes (1)
- stats/config/layout.json
✅ Files skipped from review due to trivial changes (1)
- stats/stats-server/tests/it/main.rs
🔇 Additional comments (52)
stats/stats-proto/proto/stats.proto (7)
11-13: Good addition of RPC methods for specialized stats retrieval.
These new RPCs (GetMainPageStats, GetTransactionsPageStats, and GetContractsPageStats) enhance clarity by offering targeted endpoints for distinct statistics. Ensure you handle edge cases (e.g. empty datasets) in downstream logic.
41-47: Comprehensive chart metadata structure.
The LineChartInfo message is well-designed. Validation logic to ensure id, title, and resolutions are never empty might prevent error-prone usage.
49-55: Sensible layered chart structure.
These nested messages (LineChartSection and LineCharts) provide a clear approach for grouping charts into sections. This facilitates more organized endpoints and UI rendering.
81-86: Counter-based fields for main-page stats.
Using optional Counter is suitable for partial data availability. Double-check that front-end usage gracefully handles these fields being absent.
88-88: Include potential chart expansions.
Adding the transactions chart to MainPageStats is a nice extension.
91-97: Refined stats for transaction pages.
This structure cleanly captures relevant transaction metrics. Ensure to confirm that all counters are effectively updated.
100-107: Encapsulation of contract page data.
Excellent approach for grouping contract metrics. Future expansions (e.g. contract success rate) would fit well here.
stats/stats-server/src/read_service.rs (18)
5-5: Import usage verification.
layout::placed_items_according_to_layout is newly used. Confirm it fully replaces the previous function to avoid partial usage or naming confusion.
18-22: Centralized counters enumerations.
These newly imported counters cover a wide range of blockchain metrics. Ensure that the database queries for each are well-optimized and proceed without timeouts.
24-24: Line window usage.
NewTxnsWindow references NEW_TXNS_WINDOW_RANGE. Validate correctness of the default range in different chain conditions.
84-91: Inclusive date-range to query-range conversion.
This approach ensures an inclusive start date. Validate that the transition to an exclusive to date is correct, especially around midnight boundaries and DST shifts.
115-121: Line chart query handle logic.
Ensuring the presence of an enabled resolution is prudent. If additional resolution types become available in the future, the code structure can easily accommodate them.
123-137: Counter handle retrieval and fallback.
Gracefully skipping counters without a day resolution is wise. Log coverage ensures visibility into partial or missing configurations.
139-159: Main page chart selection.
These enumerated chart references keep the code in sync with the proto definitions. Any changes to MainPageStats should be reflected here.
161-176: Contracts page chart references.
Properly enumerated. This helps ensure the page stats remain consistent with the proto schema.
178-193: Transactions page chart references.
Continuing the same pattern as main/contract pages fosters consistency. Overall, the approach is good.
255-283: Querying counters with error handling.
Providing error logs while returning None ensures partial data doesn't break the endpoints. Great balance between resilience and visibility.
289-314: Refined line chart retrieval.
Comprehensive coverage of missing or found chart entries and resolution checks. This is a robust approach.
316-353: query_new_txns_window logic.
It’s a neat solution to handle approximate data. Double-check off-by-one potential when reversing and slicing the data for the last NEW_TXNS_WINDOW_RANGE points.
370-370: Filter map usage.
Using .filter_map(...) is succinct and clean. Good job.
374-374: Ordered layout application.
placed_items_according_to_layout ensures final sorting is consistent. Confirm fallback behavior if any ID is missing from layout.
419-419: Method placeholders.
Line 419 is a blank line preceding the new methods. No issues.
420-444: GetMainPageStats implementation.
All relevant counters (average block time, addresses, blocks, txns, etc.) plus the chart are fetched together. This approach is straightforward and efficient.
446-463: GetTransactionsPageStats.
Similar pattern as the main page. Good use of join for parallel requests.
465-487: GetContractsPageStats.
Consistent approach with robust parallel queries providing a uniform API experience.
stats/stats-server/src/runtime_setup.rs (4)
17-21: Extended usage from config modules.
Import references for new chart and layout data structures. Keep an eye on compile times if your config modules expand further.
103-103: New charts_info field.
Holds a mapping of chart names to EnabledChartEntry. Great approach for a single source of truth.
143-143: Bridging layout with loaded chart data.
check_all_enabled_charts_have_endpoints ensures no orphaned charts. Good defensive check.
257-296: Warn about endpointless charts.
Logging a warning if certain charts are updated but not returned by any endpoint is a strong approach to catch misconfigurations. This is a forward-thinking safety measure.
stats/stats-server/tests/it/common.rs (2)
1-2: Import HashMap for chart resolution lookups.
This helps unify your test data when validating chart properties. No issues here.
18-28: enabled_resolutions function.
Aggregating chart IDs with their allowed resolutions is an excellent way to validate the lines. If the UI or proto changes, these checks will keep your tests in sync.
stats/stats-server/tests/it/chart_endpoints/contracts_page.rs (1)
1-25: New test for contracts page stats.
Validates presence of all essential counters. The explicit checks for description and title help ensure the metadata is never empty. Great coverage for a newly introduced endpoint.
stats/stats-server/src/config/read/layout.rs (1)
34-37: Confirm desired behavior for excluded items.
The documentation clarifies that items not in layout are now excluded. Ensure this is the intended behavior. If unexpected items should appear at the end of the list, alter your logic accordingly. Otherwise, logging or warning about excluded items could improve transparency and debugging.
stats/stats-server/tests/it/chart_endpoints/mod.rs (2)
Line range hint 36-49: Validate environment variables for test configurations.
When setting STATS__CONFIG to ./tests/config/test.toml, ensure that the file always exists in the testing environment. Missing or misnamed config files can cause unexpected runtime errors.
57-62: Leverage concurrency with caution.
Running tests concurrently is efficient, but be mindful if any shared state is manipulated by the tested endpoints. Confirm that the endpoints do not interfere with each other or cause unpredictable race conditions.
stats/stats-server/tests/it/chart_endpoints/lines.rs (2)
6-6: Validate function usage from the common module.
The enabled_resolutions function call from crate::common is well-integrated. Double-check that this logic is not duplicated elsewhere and stays synchronized if refactoring is required in other test modules.
Line range hint 8-35: Good approach for verifying sections in line charts.
The assertion for section IDs is a solid way to detect unexpected changes in the API. This helps ensure that new or removed sections are caught early.
stats/stats-server/tests/it/indexing_status.rs (3)
17-17: Maintain import organization.
The import use crate::common::{enabled_resolutions, send_arbitrary_request}; is properly grouped. Confirm that this aligns with any internal guidelines for ordering or grouping imports in test files.
24-24: Rename test database initializer accordingly.
The name test_not_indexed_ok is now consistent with the test function and clarifies its purpose. This avoids confusion with other tests that might handle health checks differently.
49-49: Confirm reduced stall time is sufficient.
Reducing the sleep duration to 2 seconds speeds up tests but may cause flakiness if indexing or other background tasks take longer than expected on certain environments.
stats/stats/src/charts/lines/mod.rs (1)
32-32: Use statement for NEW_TXNS_WINDOW_RANGE is clear.
Exposing WINDOW under a more descriptive alias (NEW_TXNS_WINDOW_RANGE) enhances readability. Confirm that the naming is well documented so its usage across the codebase remains consistent.
stats/stats-server/src/config/types.rs (1)
13-13: Switched to placed_items_according_to_layout.
This refactor changes the function call from sorted_items_according_to_layout to placed_items_according_to_layout. Ensure that the new function's logic aligns with the intended ordering/placement behavior and that all code references are consistent with this update.
stats/stats-proto/proto/api_config_http.yaml (1)
12-17: New endpoints for main, transactions, and contracts pages.
The newly introduced GET endpoints seem consistent with the existing naming conventions for your stats API. Make sure the corresponding RPCs (GetMainPageStats, GetTransactionsPageStats, GetContractsPageStats) and data models are thoroughly tested and documented.
stats/stats-server/tests/it/chart_endpoints/transactions_page.rs (2)
1-5: Imports and module references look good.
The send_get_request utility is properly imported, as are the necessary data types (TransactionsPageStats and Url). This setup appears consistent with other integration tests. No issues detected here.
6-25: Comprehensive test for /api/v1/pages/transactions.
- The test checks multiple counters (
pending_txns,txns_fee_24h,average_txn_fee_24h,total_txns) to ensure they exist and have valid metadata. - Assertions confirm that each counter’s description and title is non-empty, preventing silent or incomplete metric definitions.
- The usage of
expectwith a descriptive error message is good for debugging test failures.
Overall, this test is well-structured and effectively validates the endpoint’s essentials.
stats/stats-proto/swagger/stats.swagger.yaml (6)
81-94: New /api/v1/pages/contracts endpoint definition.
The endpoint StatsService_GetContractsPageStats is clearly documented as returning v1ContractsPageStats. Ensure the path and operation ID match the RPC method and YAML config for a consistent developer experience.
95-108: New /api/v1/pages/main endpoint definition.
The endpoint StatsService_GetMainPageStats is introduced with a clearly defined successful response referencing v1MainPageStats. Double-check that any required query parameters are handled properly if specified in the proto/RPC definition.
109-122: New /api/v1/pages/transactions endpoint definition.
The endpoint StatsService_GetTransactionsPageStats references v1TransactionsPageStats. This addition aligns with your test coverage verifying the endpoint’s counters. The naming is consistent with the existing pattern.
174-184: v1ContractsPageStats object definition.
The newly declared properties provide a clear and organized structural layout for contract-related counters. Verify no existing properties need renaming or re-use for standardization across your stats schema.
256-270: v1MainPageStats object definition.
Provides a comprehensive set of fields for main-page metrics (block time, addresses, blocks, etc.), plus a transactions line chart reference. Ensure that the line chart data is appended in a consistent format with other chart endpoints.
292-302: v1TransactionsPageStats object definition.
This data model clearly distinguishes pending transactions, fees, and totals. The property names follow consistent naming conventions. Ensure that the introduction of these fields doesn’t conflict with existing metrics or require additional migrations in your codebase.
stats/config/charts.json (1)
142-142: Verify the time window change implementation.
The description change from "previous day" to "past 30 days" suggests a significant modification in the data window.
Let's verify the corresponding implementation changes:
✅ Verification successful
Let me gather more information about the time window implementation.
Description accurately reflects the implementation
The code implementation in stats/stats/src/charts/lines/new_txns_window.rs confirms that the chart is indeed designed to show transactions for the past 30 days:
- The window size is explicitly defined as
pub const WINDOW: u64 = 30 - The implementation correctly handles this 30-day window in the query logic
- The module's documentation explicitly states: "New transactions for the last N days (usually 30)"
Therefore, the description change in the configuration accurately reflects the actual implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for related changes in the implementation
echo "Searching for new_txns_window related code..."
rg -l "new_txns_window" | while read -r file; do
if [[ "$file" != "stats/config/charts.json" ]]; then
echo "=== $file ==="
rg -A 5 "new_txns_window" "$file"
fi
done
# Search for potential API endpoint changes
echo -e "\nSearching for related API endpoints..."
ast-grep --pattern 'fn $_($$) -> $$ {
$$$
new_txns_window
$$$
}'
Length of output: 4507
Script:
#!/bin/bash
# Check the actual implementation of new_txns_window
echo "=== Checking new_txns_window implementation ==="
cat stats/stats/src/charts/lines/new_txns_window.rs
# Check if there are any recent changes to this file
echo -e "\n=== Checking recent changes ==="
git log -n 1 --pretty=format:"%H %s" -- stats/stats/src/charts/lines/new_txns_window.rs
# Check for any constants or configuration related to the window size
echo -e "\n=== Checking for window size configuration ==="
rg -A 2 "WINDOW|window.*30|RANGE" stats/stats/src/charts/lines/new_txns_window.rs
Length of output: 7161
| pending_txns, | ||
| txns_fee_24h, | ||
| average_txn_fee_24h, | ||
| total_txns, |
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores