[model-gateway] Improve logging in policies module#15496
Conversation
- Replace eprintln! with proper tracing::error! in bucket.rs - Change info! to debug! for eviction diagnostics in tree.rs
Summary of ChangesHello @slin1237, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on enhancing the logging practices within the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request improves logging in the policies module by replacing eprintln! with tracing::error! and lowering the log level of diagnostic messages from info to debug. The changes are beneficial for log consistency and clarity. I've added a few suggestions to further improve logging by adopting a more structured format, which will enhance observability and make logs easier to parse.
| error!( | ||
| "Failed to acquire write lock for bucket {}: {}", | ||
| model_id, e | ||
| ); |
There was a problem hiding this comment.
For better structured logging, it's recommended to use named fields for variables in tracing macros. This makes logs easier to parse and query in log management systems. Using ?e will also provide more detailed debug information about the error.
error!(
model_id = %model_id,
error = ?e,
"Failed to acquire write lock for bucket"
);| debug!("Before eviction - Used size per tenant:"); | ||
| for entry in self.tenant_char_count.iter() { | ||
| info!("Tenant: {}, Size: {}", entry.key(), entry.value()); | ||
| debug!("Tenant: {}, Size: {}", entry.key(), entry.value()); | ||
| } |
There was a problem hiding this comment.
Instead of logging each tenant's size in a loop, which can create a large number of log entries, it's more efficient to collect the sizes and log them as a single structured message. This is less noisy and easier to parse. Using tracing::enabled! avoids the collection overhead when debug logging is disabled.
| debug!("Before eviction - Used size per tenant:"); | |
| for entry in self.tenant_char_count.iter() { | |
| info!("Tenant: {}, Size: {}", entry.key(), entry.value()); | |
| debug!("Tenant: {}, Size: {}", entry.key(), entry.value()); | |
| } | |
| if tracing::enabled!(tracing::Level::DEBUG) { | |
| let tenant_sizes: std::collections::HashMap<TenantId, usize> = | |
| self.tenant_char_count.iter().map(|e| (e.key().clone(), *e.value())).collect(); | |
| debug!(tenant_sizes = ?tenant_sizes, "Before eviction - Used size per tenant"); | |
| } |
| debug!("After eviction - Used size per tenant:"); | ||
| for entry in self.tenant_char_count.iter() { | ||
| info!("Tenant: {}, Size: {}", entry.key(), entry.value()); | ||
| debug!("Tenant: {}, Size: {}", entry.key(), entry.value()); | ||
| } |
There was a problem hiding this comment.
Similar to the pre-eviction logging, it's better to log all tenant sizes in a single structured message to reduce log noise and improve parsability. The tracing::enabled! guard prevents performance impact when debug logs are off.
if tracing::enabled!(tracing::Level::DEBUG) {
let tenant_sizes: std::collections::HashMap<TenantId, usize> =
self.tenant_char_count.iter().map(|e| (e.key().clone(), *e.value())).collect();
debug!(tenant_sizes = ?tenant_sizes, "After eviction - Used size per tenant");
}
Checklist