Skip to content

feat(stats): add flag to update all charts#1577

Merged
bragov4ik merged 1 commit intomainfrom
bragov4ik/stats-batch-update-all
Dec 3, 2025
Merged

feat(stats): add flag to update all charts#1577
bragov4ik merged 1 commit intomainfrom
bragov4ik/stats-batch-update-all

Conversation

@bragov4ik
Copy link
Contributor

@bragov4ik bragov4ik commented Dec 3, 2025

Summary by CodeRabbit

  • New Features
    • Added an "update all charts" option for batch updates. When enabled, all available charts are updated regardless of the specified chart list.

✏️ Tip: You can customize this high-level summary in your review settings.

@bragov4ik bragov4ik self-assigned this Dec 3, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 3, 2025

Walkthrough

This pull request introduces a new optional update_all flag to the batch chart update functionality. The flag is added to the protocol buffer definition, OpenAPI specification, and the Rust service layer. When update_all is set to true, the system updates all available charts instead of only those specified in the chart_names field. Changes are propagated through the read service, update service, and test fixtures to support this new control path.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • stats/stats-server/src/update_service.rs: Core logic change where chart_names is mutated when update_all is true; verify that the override correctly retrieves all enabled chart keys from self.charts.charts_info
  • Protocol and API definitions: Ensure consistency between proto, Swagger, and implementation regarding field naming and optionality
  • Test coverage: Verify that test updates adequately cover both the update_all=true and update_all=false code paths

Poem

🐰 A flag to hop through every chart so bright,
No need to name them one by one in sight!
When update_all is set to true with glee,
All charts shall dance in harmony—wheee! 🎉

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'feat(stats): add flag to update all charts' clearly and concisely summarizes the main change across all modified files: adding an update_all flag to enable batch updating of all charts.
Docstring Coverage ✅ Passed Docstring coverage is 83.33% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch bragov4ik/stats-batch-update-all

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cf281a3 and 37e6486.

📒 Files selected for processing (6)
  • stats/stats-proto/proto/stats.proto (1 hunks)
  • stats/stats-proto/swagger/stats.swagger.yaml (1 hunks)
  • stats/stats-server/src/read_service.rs (1 hunks)
  • stats/stats-server/src/update_service.rs (1 hunks)
  • stats/stats-server/tests/it/common.rs (2 hunks)
  • stats/stats-server/tests/it/mock_blockscout_reindex.rs (7 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: bragov4ik
Repo: blockscout/blockscout-rs PR: 1226
File: stats/stats-server/src/update_service.rs:435-459
Timestamp: 2025-02-04T10:11:04.985Z
Learning: The chart batch update endpoint is admin-only, so resource exhaustion protections like batch size limits are not necessary.
📚 Learning: 2025-02-05T11:11:07.743Z
Learnt from: bragov4ik
Repo: blockscout/blockscout-rs PR: 1226
File: stats/stats-proto/swagger/stats.swagger.yaml:13-32
Timestamp: 2025-02-05T11:11:07.743Z
Learning: The `/api/v1/charts/batch-update` endpoint in the stats service requires API key authentication using the `x-api-key` header.

Applied to files:

  • stats/stats-proto/proto/stats.proto
  • stats/stats-server/tests/it/common.rs
  • stats/stats-proto/swagger/stats.swagger.yaml
📚 Learning: 2025-01-22T09:56:00.917Z
Learnt from: rimrakhimov
Repo: blockscout/blockscout-rs PR: 1202
File: eth-bytecode-db/eth-bytecode-db/src/search/alliance_db.rs:9-12
Timestamp: 2025-01-22T09:56:00.917Z
Learning: The chain_id parameter type change from i64 to u128 in eth-bytecode-db/eth-bytecode-db/src/search/alliance_db.rs was deferred to a future PR to maintain focus on the verifier_alliance schema v1 integration.

Applied to files:

  • stats/stats-server/tests/it/mock_blockscout_reindex.rs
📚 Learning: 2024-12-11T09:29:58.857Z
Learnt from: bragov4ik
Repo: blockscout/blockscout-rs PR: 1144
File: stats/stats/src/charts/counters/total_txns.rs:28-54
Timestamp: 2024-12-11T09:29:58.857Z
Learning: In `stats/stats/src/charts/counters/total_txns.rs`, attempting to combine the two database queries into one can result in code that doesn't compile and is more verbose. In future reviews, avoid suggesting this optimization for this context.

Applied to files:

  • stats/stats-server/tests/it/mock_blockscout_reindex.rs
🧬 Code graph analysis (1)
stats/stats-server/tests/it/mock_blockscout_reindex.rs (1)
stats/stats-server/tests/it/common.rs (1)
  • request_reupdate_from (136-157)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Unit, doc and integration tests
🔇 Additional comments (9)
stats/stats-server/tests/it/common.rs (1)

136-157: LGTM!

The test helper function correctly propagates the new update_all flag to the BatchUpdateChartsRequest payload, properly wrapping it in Some() for the optional proto field.

stats/stats-server/tests/it/mock_blockscout_reindex.rs (4)

8-8: LGTM!

The assert_ne import is appropriately added to support the new assertion at line 165.


73-74: LGTM!

Existing test calls correctly pass false for the new update_all parameter, preserving the original behavior of updating only specified charts.

Also applies to: 103-104, 133-134


162-165: LGTM!

The new test segment correctly exercises the update_all flag by passing an empty chart list and verifying that charts are still accepted. The assertions confirm that the request succeeded with no rejections and at least some charts were updated.


176-181: LGTM!

The manually constructed test requests appropriately set update_all: None when testing error paths (authentication and date validation). This is the correct default for these test scenarios.

Also applies to: 208-213

stats/stats-proto/proto/stats.proto (1)

203-210: LGTM!

The proto field addition is correctly defined with appropriate field number 4, optional modifier for backward compatibility, and a clear comment describing its behavior.

stats/stats-proto/swagger/stats.swagger.yaml (1)

237-252: LGTM!

The swagger schema correctly defines the update_all field with appropriate type (boolean) and clear description that matches the proto definition.

stats/stats-server/src/read_service.rs (1)

692-732: LGTM!

The implementation correctly threads the update_all flag from the request to the update service, using unwrap_or(false) to provide appropriate default behavior for backward compatibility.

stats/stats-server/src/update_service.rs (1)

453-487: LGTM!

The implementation correctly handles the update_all flag by replacing chart_names with all enabled chart keys when the flag is true. The logic is straightforward and appropriate for an admin-only endpoint. The documentation clearly explains the behavior.

Based on learnings, resource exhaustion protections are not required for this admin-only endpoint.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@bragov4ik bragov4ik added the feat New feature label Dec 3, 2025
@bragov4ik
Copy link
Contributor Author

@coderabbitai review

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 3, 2025

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Contributor

@EvgenKor EvgenKor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm

@bragov4ik bragov4ik merged commit 3006f81 into main Dec 3, 2025
7 checks passed
@bragov4ik bragov4ik deleted the bragov4ik/stats-batch-update-all branch December 3, 2025 16:52
Dustin4444

This comment was marked as spam.

Dustin4444

This comment was marked as spam.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feat New feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants