Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: track client app in metrics #1056

Merged
merged 3 commits into from
Jun 10, 2024
Merged

feat: track client app in metrics #1056

merged 3 commits into from
Jun 10, 2024

Conversation

dinhani-cw
Copy link
Contributor

No description provided.

@dinhani-cw dinhani-cw requested a review from a team as a code owner June 10, 2024 19:30
Copy link

PR Reviewer Guide 🔍

⏱️ Estimated effort to review [1-5]

4

🧪 Relevant tests

No

🔒 Security concerns

No

⚡ Key issues to review

Logging and Monitoring:
The PR includes changes to logging and monitoring by incorporating the RpcClientApp into the metrics. Ensure that these changes are thoroughly tested, especially the new metric labels (app) that have been added.

Async Task Handling:
The PR modifies how RPC requests are handled and logged. It's crucial to ensure that these operations are non-blocking and appropriately asynchronous as per the project's guidelines.

Code Refactoring:
The PR includes refactoring of method names and the extraction of functions (extract_call_function and extract_transaction_function). Review these changes for correctness and ensure they align with the intended functionality.

Copy link

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Score
Performance
Use references instead of cloning for method and function in metrics calls

Replace the cloning of method and function in the metrics logging with references to avoid
unnecessary data duplication, which can impact performance.

src/eth/rpc/rpc_middleware.rs [74-75]

-metrics::set_rpc_requests_active(active, &app, method, function.clone());
-metrics::inc_rpc_requests_started(&app, method, function.clone());
+metrics::set_rpc_requests_active(active, &app, &method, &function);
+metrics::inc_rpc_requests_started(&app, &method, &function);
 
Suggestion importance[1-10]: 8

Why: This suggestion addresses a performance concern by reducing unnecessary data duplication, which can be beneficial in high-frequency logging scenarios.

8
Enhancement
Replace direct string conversions with a more robust display handling

Consider implementing Display for MetricLabelValue to avoid calling to_string() on the
enum variants directly, which can be inefficient and error-prone if the display logic
changes.

src/eth/rpc/rpc_client_app.rs [31-32]

-RpcClientApp::Identified(name) => Self::Some(name.to_string()),
-RpcClientApp::Unknown => Self::Some("unknown".to_string()),
+RpcClientApp::Identified(name) => Self::Some(name),
+RpcClientApp::Unknown => Self::Some("unknown"),
 
Suggestion importance[1-10]: 7

Why: The suggestion improves code robustness and maintainability by centralizing the display logic, but it is not critical for functionality or performance.

7
Enhance metrics labels for better data analysis capabilities

Add labels for better granularity and filtering in metrics, such as adding client_type to
distinguish between different types of clients.

src/infra/metrics/metrics_definitions.rs [8-14]

-gauge              rpc_requests_active{app, method, function} [],
-counter            rpc_requests_started{app, method, function} [],
-histogram_duration rpc_requests_finished{app, method, function, result, success} [],
+gauge              rpc_requests_active{app, method, function, client_type} [],
+counter            rpc_requests_started{app, method, function, client_type} [],
+histogram_duration rpc_requests_finished{app, method, function, result, success, client_type} [],
 
Suggestion importance[1-10]: 5

Why: While adding more labels can enhance data analysis, it is not directly related to the core functionality or performance of the code.

5
Maintainability
Simplify assignment of indexes with direct cloning

Use direct assignment for static_slot_indexes and mapping_slot_indexes instead of
clone_from to simplify the code and potentially improve performance by avoiding an
unnecessary method call.

src/eth/storage/inmemory/inmemory_temporary.rs [208-211]

-account.info.static_slot_indexes.clone_from(indexes);
-account.info.mapping_slot_indexes.clone_from(indexes);
+account.info.static_slot_indexes = indexes.clone();
+account.info.mapping_slot_indexes = indexes.clone();
 
Suggestion importance[1-10]: 6

Why: The suggestion simplifies the code and may offer a minor performance improvement, but the impact is likely minimal.

6

@dinhani-cw dinhani-cw enabled auto-merge (squash) June 10, 2024 19:47
@dinhani-cw dinhani-cw merged commit f955f1a into main Jun 10, 2024
32 checks passed
@dinhani-cw dinhani-cw deleted the rpc-middleware branch June 10, 2024 19:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant