refactor(metrics): separate metrics recorder impl from the server #2561
Conversation
WalkthroughOhayo, sensei! This pull request introduces significant changes to the metrics handling within the Torii and Katana projects. The Changes
Suggested labels
Suggested reviewers
🪧 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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (6)
crates/metrics/src/lib.rs (2)
2-2: Consider making theprocessmodule public if needed, sensei.Currently,
processis declared asmod process;, which makes it private to the crate. If external modules or crates need to access items fromprocess, consider changing it topub mod process;.
22-32: Enhance error variant documentation for clarity, sensei.Adding documentation comments to each variant of the
Errorenum can improve code readability and help other developers understand the error cases.Apply this diff to add documentation comments:
#[derive(Debug, thiserror::Error)] pub enum Error { + /// Global metrics recorder has already been installed. #[error("global metrics recorder already installed.")] GlobalRecorderAlreadyInstalled, + /// Could not bind to the specified address. #[error("could not bind to address: {addr}")] FailedToBindAddress { addr: SocketAddr }, + /// An error occurred within the server. #[error(transparent)] Server(#[from] hyper::Error), }crates/metrics/src/exporters/prometheus.rs (1)
15-17: Consider expanding the documentation forPrometheusRecorder.Ohayo sensei! Providing more detailed documentation for the
PrometheusRecorderstruct can help others better understand its purpose and how to use it effectively.crates/metrics/src/server.rs (1)
93-93: EnhanceDebugimplementation forhooksfieldOhayo, sensei! To improve debugging, consider displaying the number of hooks instead of a placeholder. This provides more insightful information about the state of the
Server.Apply the following change:
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - f.debug_struct("Server").field("hooks", &"...").field("exporter", &self.exporter).finish() + f.debug_struct("Server") + .field("hooks_count", &self.hooks.len()) + .field("exporter", &self.exporter) + .finish() }crates/metrics/src/process.rs (1)
83-116: Ohayo, sensei! Ensure consistency in metric descriptionsSome metric descriptions in
describe_memory_statsare using backslashes for line continuation, which may not be necessary and can affect readability.Consider removing the backslash and enclosing the description in a single string:
-describe_gauge!( "jemalloc.retained", metrics::Unit::Bytes, - "Total number of bytes in virtual memory mappings that were retained rather than being \ - returned to the operating system via e.g. munmap(2)" + "Total number of bytes in virtual memory mappings that were retained rather than being returned to the operating system via e.g. munmap(2)" );crates/katana/node/src/lib.rs (1)
100-100: Ohayo, sensei! Consider addressing the TODO commentThe comment suggests that this code might be better placed in the build stage. Refactoring it accordingly could enhance the code organization and initialization flow.
Would you like assistance in moving this code to the build stage?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
- bin/torii/src/main.rs (2 hunks)
- crates/katana/node/src/lib.rs (3 hunks)
- crates/metrics/Cargo.toml (1 hunks)
- crates/metrics/src/exporters/mod.rs (1 hunks)
- crates/metrics/src/exporters/prometheus.rs (1 hunks)
- crates/metrics/src/lib.rs (2 hunks)
- crates/metrics/src/process.rs (1 hunks)
- crates/metrics/src/prometheus_exporter.rs (0 hunks)
- crates/metrics/src/server.rs (1 hunks)
💤 Files with no reviewable changes (1)
- crates/metrics/src/prometheus_exporter.rs
🧰 Additional context used
🔇 Additional comments (9)
crates/metrics/src/exporters/mod.rs (1)
1-1: Ohayo, sensei! LGTM for the module declaration.The
prometheusmodule is declared as public, which is appropriate if it contains types or functions that need to be accessible from outside this module. This follows Rust conventions nicely.crates/metrics/Cargo.toml (1)
21-21: Ohayo, sensei! New dependency added. Let's verify its usage.The addition of
tokio-utilas a workspace-level dependency looks good. It's likely related to the metrics handling refactoring mentioned in the PR objectives.Let's make sure this new dependency is being used in the codebase:
If the search doesn't return any results, we might need to reconsider adding this dependency or ensure it's used in the upcoming changes.
✅ Verification successful
Ohayo, sensei!
tokio-utilis successfully being utilized in the codebase. Approved.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for the usage of tokio-util in the codebase # Test: Search for tokio-util usage rg --type rust 'use tokio_util::'Length of output: 376
crates/metrics/src/lib.rs (4)
5-5: Ohayo, sensei! Validate the necessity of theSocketAddrimport.The addition of
use std::net::SocketAddr;suggests network functionalities are being utilized. Ensure that this import is necessary and that all uses ofSocketAddrare correctly implemented.
42-46: Implementation ofReporttrait is spot on, sensei!The implementation of the
Reporttrait for::metrics_process::Collectorby invokingself.collect()is correct and aligns with the expected behavior.
1-3: Ohayo, sensei! Ensure module renaming is consistent throughout the codebase.The module
prometheus_exporterhas been renamed toexporters. Please verify that all references toprometheus_exporterhave been updated to prevent potential import errors.Run the following script to check for any remaining references:
✅ Verification successful
Ohayo, sensei! It seems there was a syntax hiccup in the previous script. Let's run a corrected version to ensure all instances of
prometheus_exporterare addressed.
Ohayo, sensei! All references to
prometheus_exporterhave been successfully updated and are consistent throughout the codebase. 🎉🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find any occurrences of 'prometheus_exporter' in the codebase. # Test: Search for 'prometheus_exporter' in Rust files. Expect: No matches. rg --type rust 'prometheus_exporter'Length of output: 191
Script:
#!/bin/bash # Description: Find any occurrences of 'prometheus_exporter' in Rust files. # Test: Search for 'prometheus_exporter' in Rust files. Expect: No matches. rg --type rust 'prometheus_exporter'Length of output: 36
15-15: Re-exportingservermodule: Verify exposed API, sensei.By using
pub use server::*;, all public items in theservermodule are re-exported. Please confirm that only intended items are made public to avoid exposing internal implementation details.Run the following script to list all publicly exported items from the
servermodule:crates/metrics/src/exporters/prometheus.rs (1)
1-52: LGTM!Ohayo sensei! The implementation looks solid overall.
crates/metrics/src/process.rs (1)
118-122: Ohayo, sensei! Confirm that empty functions are necessaryThe empty implementations of
collect_memory_statsanddescribe_memory_statsfor non-jemalloc or non-Unix configurations might lead to confusion.Please ensure that these functions are required and that they won't cause issues when called in those configurations.
✅ Verification successful
Ohayo, sensei! Verified that empty functions are necessary
The empty implementations of
collect_memory_statsanddescribe_memory_statsare required in configurations without jemalloc or on non-Unix systems to ensure that calls to these functions do not cause issues.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if `collect_memory_stats` and `describe_memory_stats` are used in configurations without jemalloc or on non-Unix systems. # Test: Search for usages of these functions. rg --type rust 'collect_memory_stats|describe_memory_stats'Length of output: 565
bin/torii/src/main.rs (1)
23-23: Ohayo sensei! Correctly importedPrometheusRecorderThe
PrometheusRecorderis correctly imported fromdojo_metrics::exporters::prometheus.
| /// Trait for metrics recorder whose metrics can be exported. | ||
| pub trait Exporter: Clone + Send + Sync { | ||
| /// Export the metrics that have been recorded by the metrics thus far. | ||
| fn export(&self) -> String; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo again, sensei! The Exporter trait looks solid, with a small suggestion.
The Exporter trait is well-defined and documented. The Clone + Send + Sync bounds ensure thread safety, which is crucial for metrics systems. The export method signature is clear and straightforward.
One small suggestion to consider:
You might want to make the export method more flexible by allowing parameters. For example:
fn export(&self, format: Option<ExportFormat>) -> String;This would allow users to specify different export formats if needed in the future, while maintaining backwards compatibility with the current implementation.
| if config.metrics.is_some() { | ||
| // Metrics recorder must be initialized before calling any of the metrics macros, in order | ||
| // for it to be registered. | ||
| let _ = PrometheusRecorder::install("katana")?; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo, sensei! Remove unnecessary assignment to _
Assigning the result to _ is unnecessary when using the ? operator. You can directly call the function and let the ? operator handle any errors.
Apply this diff:
-let _ = PrometheusRecorder::install("katana")?;
+PrometheusRecorder::install("katana")?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let _ = PrometheusRecorder::install("katana")?; | |
| PrometheusRecorder::install("katana")?; |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2561 +/- ##
==========================================
+ Coverage 69.44% 69.80% +0.36%
==========================================
Files 397 400 +3
Lines 51222 51171 -51
==========================================
+ Hits 35571 35720 +149
+ Misses 15651 15451 -200 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
crates/metrics/src/server.rs (3)
24-25: Ohayo, sensei! Don't forget about the TODO.There's a TODO comment about allowing configuration of the server executor. This could be an important feature for cancelling individual connection tasks.
Consider creating a GitHub issue to track this TODO and ensure it's not forgotten. Would you like me to create an issue for this task?
47-56: Ohayo, sensei! Thewith_reportsmethod looks good, but let's make it even better!The implementation of
with_reportsis well done, providing flexibility in adding new metrics reporters. However, we can make a small improvement for better performance.Consider using
extend_from_sliceinstead ofextendif you know the exact number of items beforehand. This can be more efficient for large collections. Here's a suggested change:pub fn with_reports<I>(mut self, reports: I) -> Self where I: IntoIterator<Item = Box<dyn Report>>, { // convert the report types into callable hooks - let hooks = reports.into_iter().map(|r| Box::new(move || r.report()) as BoxedHook); - self.hooks.extend(hooks); + let new_hooks: Vec<BoxedHook> = reports.into_iter().map(|r| Box::new(move || r.report()) as BoxedHook).collect(); + self.hooks.extend_from_slice(&new_hooks); self }This change can potentially improve performance when adding a large number of reports.
58-63: Ohayo, sensei! Thewith_process_metricsmethod is solid, but let's add a cherry on top!The implementation of
with_process_metricsis well done, adding a hook for collecting process metrics using the defaultCollector.For improved clarity and to make the code more self-documenting, consider adding a brief comment explaining what the
process.describe()call does. Here's a suggested change:pub fn with_process_metrics(mut self) -> Self { let process = metrics_process::Collector::default(); + // Register metric descriptions with the global registry process.describe(); self.hooks.push(Box::new(move || process.collect()) as BoxedHook); self }This small addition will help future maintainers understand the purpose of the
describe()call at a glance.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- bin/torii/src/main.rs (2 hunks)
- crates/metrics/src/server.rs (1 hunks)
🧰 Additional context used
🔇 Additional comments (5)
crates/metrics/src/server.rs (2)
65-87: Ohayo, sensei! Thestartmethod looks great!The implementation of the
startmethod is well-structured and efficient. It properly sets up the HTTP server using Hyper, handles incoming requests, and serves metrics. The use ofArcfor sharing hooks across threads is a good practice for concurrent scenarios.Great job on implementing this crucial part of the metrics server!
90-97: Ohayo, sensei! NiceDebugimplementation!The custom
Debugimplementation for theServerstruct is well done. It provides a clean and concise debug output by omitting the details of thehooksfield, which is a good practice to avoid cluttering the output.Good job on providing a useful debug representation!
bin/torii/src/main.rs (3)
23-23: Ohayo, sensei! The import statement looks good!The updated import for
PrometheusRecorderaligns perfectly with the refactoring objectives. It's a clean change that reflects the new structure of thedojo_metricscrate.
300-302: Sugoi work on the metrics setup, sensei!The new implementation looks great! You've successfully separated the metrics recorder from the server, which aligns perfectly with our refactoring goals. Here's what I love about it:
- Using
PrometheusRecorder::installis a clean upgrade from the previousprometheus_exporter::install_recorder.- The
dojo_metrics::Server::newsetup with process metrics is a nice touch for comprehensive monitoring.- Spawning the server in a separate tokio task is brilliant - it addresses the previous review comment about potential blocking of the main thread.
Domo arigato for implementing these changes so thoughtfully!
23-23: Ohayo once more, sensei! Let's wrap up this review.Your changes to the metrics handling are truly impressive. Here's a quick summary of the impact:
- The refactoring has been executed cleanly, focusing solely on the metrics-related code.
- You've improved the separation of concerns by decoupling the metrics recorder from the server.
- The rest of the file remains untouched, which shows great precision in implementing the intended changes.
Overall, these modifications enhance the modularity and maintainability of the metrics system without disrupting the existing functionality. Excellent work, sensei!
Also applies to: 300-302
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
crates/metrics/src/server.rs (2)
23-24: Ohayo, sensei! Let's address the TODO comment.There's a TODO comment about allowing the configuration of the server executor. This is an important feature that should be implemented to provide more control over the server's behavior.
Would you like assistance in implementing this feature? I can help you create a new method to configure the executor, which could look something like this:
pub fn with_executor<E>(mut self, executor: E) -> Self where E: hyper::rt::Executor<BoxFuture<'static, ()>> + Send + Sync + 'static, { // Implementation details }Let me know if you'd like me to elaborate on this implementation or if you prefer to track this as a separate issue.
1-101: Ohayo, sensei! Overall, this is a well-implemented metrics server.The
Serverstruct and its implementation provide a robust foundation for serving metrics. The code is well-structured, follows Rust best practices, and provides a clear API for users.Great job on this implementation! With the minor adjustments suggested in the previous comments (simplifying the
Hooktrait, adding trait bounds toMetricsExporter, and addressing the TODO), this will be an excellent addition to the project.If you need any help implementing the suggested changes or have any questions, please don't hesitate to ask. Keep up the great work, sensei!
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- crates/metrics/src/server.rs (1 hunks)
🧰 Additional context used
🔇 Additional comments (3)
crates/metrics/src/server.rs (3)
39-92: Ohayo, sensei! The implementation looks good overall.The methods
new,with_reports,with_process_metrics, andstartare well-implemented and provide a good API for creating and configuring the server.The implementation is clear, concise, and follows good Rust practices. Great job!
94-101: Ohayo, sensei! The Debug implementation looks great.The custom
Debugimplementation for theServerstruct is well-done. It provides a clear and concise representation of the struct, showing the exporter while omitting the detailed representation of the hooks.This implementation follows best practices for custom
Debugimplementations. Well done!
35-38:⚠️ Potential issueOhayo, sensei! Let's add some trait bounds.
As suggested in the previous review, we should add
Clone + Send + Synctrait bounds toMetricsExporter. This ensures thread safety and proper functionality, especially when usingself.exporter.clone()in async closures.Please apply the following change:
impl<MetricsExporter> Server<MetricsExporter> where - MetricsExporter: Exporter + 'static, + MetricsExporter: Exporter + Clone + Send + Sync + 'static, {This change will make the code more robust and prevent potential runtime issues.
Likely invalid or redundant comment.
Summary by CodeRabbit
New Features
Serverstruct to serve metrics and collect process metrics.Bug Fixes
Refactor
Chores
Cargo.tomlfile.