docs/metrics: canonical hash metrics docs + consistency script #49
Conversation
Summary of ChangesHello @zensgit, 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 documentation and consistency of metrics within the Highlights
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.
Pull Request Overview
This PR adds comprehensive metrics documentation and consistency verification tooling to support the previously-merged metrics endpoint functionality. The PR transforms the earlier verification report into proper project documentation and provides a maintenance script.
- Updated metrics verification report with canonical hash metrics documentation and deprecation notices
- Added monitoring and consistency verification script for operational use
- Enhanced documentation to distinguish between health checks and Prometheus metrics
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| jive-api/target/release/jive-api.d | Build dependency file updated with metrics.rs dependency |
| jive-api/src/metrics.rs | Enhanced metrics endpoint with canonical hash metrics and backward-compatible legacy metrics |
| jive-api/PR47_METRICS_VERIFICATION_REPORT.md | Comprehensive verification report documenting metrics implementation and testing results |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| buf.push_str(&format!("password_hash_argon2id_total {}\n", a2id)); | ||
| buf.push_str("# HELP password_hash_unknown_total Users with unknown hash prefix.\n"); | ||
| buf.push_str("# TYPE password_hash_unknown_total gauge\n"); | ||
| buf.push_str(&format!("password_hash_unknown_total {}\n", unknown.max(0))); |
There was a problem hiding this comment.
The unknown.max(0) call is unnecessary since unknown is calculated as a difference of non-negative values and should never be negative. This suggests potential logic issues or defensive programming that obscures the actual calculation.
| buf.push_str(&format!("password_hash_unknown_total {}\n", unknown.max(0))); | |
| buf.push_str(&format!("password_hash_unknown_total {}\n", unknown)); |
Copilot uses AI. Check for mistakes.
|
|
||
| // Legacy (deprecated) metrics for transitional dashboards | ||
| buf.push_str( | ||
| "# HELP jive_password_hash_users (DEPRECATED) Users by password hash algorithm variant.\n", |
There was a problem hiding this comment.
The deprecation notice should include a timeline or version for removal and reference to the replacement metrics to help users migrate their monitoring configurations.
| "# HELP jive_password_hash_users (DEPRECATED) Users by password hash algorithm variant.\n", | |
| "# HELP jive_password_hash_users (DEPRECATED, will be removed in v2.0.0; use password_hash_bcrypt_variant and password_hash_total_count instead) Users by password hash algorithm variant.\n", |
Copilot uses AI. Check for mistakes.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces canonical metrics for password hash distribution, deprecates the old metrics, and adds related documentation. The changes in src/metrics.rs are a good step towards better observability.
My review focuses on the new documentation and the implementation of the new metrics. I've identified some inconsistencies in the verification report (PR47_METRICS_VERIFICATION_REPORT.md) regarding port numbers and the actual metrics output shown. I've also suggested a refactoring in src/metrics.rs to improve code maintainability by reducing duplication in how metric strings are generated. Overall, the changes are positive, and addressing these points will improve the quality and clarity of the code and documentation.
| ### Service Startup Confirmed ✅ | ||
| From log analysis (bash process 2340c9): | ||
| ``` | ||
| 🚀 Starting Jive Money API Server (Complete Version)... | ||
| 📦 Features: WebSocket, Database, Redis (optional), Full API | ||
| ✅ Database connected successfully | ||
| ✅ WebSocket manager initialized | ||
| ✅ Redis connected successfully | ||
| 🌐 Server running at http://127.0.0.1:8012 | ||
| ``` |
There was a problem hiding this comment.
There's an inconsistency in the port number used throughout this report. The service startup log (line 93) shows the server running on port 8012, but all subsequent curl commands and status checks (lines 170, 192, 226) use port 8014. Please ensure the port numbers are consistent to avoid confusion.
| ## Runtime Testing Results ✅ | ||
|
|
||
| ### 1. Endpoint Accessibility ✅ | ||
| **Command**: | ||
| ```bash | ||
| curl -s http://localhost:8014/metrics | head -10 | ||
| ``` | ||
|
|
||
| **Actual Output**: | ||
| ``` | ||
| # HELP jive_password_rehash_total Total successful bcrypt to argon2id password rehashes. | ||
| # TYPE jive_password_rehash_total counter | ||
| jive_password_rehash_total 0 | ||
| # HELP jive_password_hash_users Users by password hash algorithm variant. | ||
| # TYPE jive_password_hash_users gauge | ||
| jive_password_hash_users{algo="bcrypt_2a"} 0 | ||
| jive_password_hash_users{algo="bcrypt_2b"} 0 | ||
| jive_password_hash_users{algo="bcrypt_2y"} 0 | ||
| jive_password_hash_users{algo="argon2id"} 0 | ||
| ``` | ||
|
|
||
| **Result**: ✅ PASSED - Endpoint responding with proper Prometheus format |
There was a problem hiding this comment.
The 'Runtime Testing Results' section appears to be inconsistent with the rest of the document. The 'Key Features Verified' (lines 73-81) and 'Expected Metrics' (lines 110-133) sections describe the new canonical metrics (e.g., password_hash_bcrypt_total), but the 'Actual Output' (lines 175-183) shows the old, deprecated metrics (e.g., jive_password_hash_users). To ensure the verification report is accurate and reflects the changes in this PR, the 'Actual Output' should be updated to show the new canonical metrics.
| buf.push_str("# HELP password_hash_bcrypt_total Users with any bcrypt hash (2a+2b+2y).\n"); | ||
| buf.push_str("# TYPE password_hash_bcrypt_total gauge\n"); | ||
| buf.push_str(&format!("password_hash_bcrypt_total {}\n", bcrypt_total)); | ||
| buf.push_str("# HELP password_hash_argon2id_total Users with argon2id hash.\n"); | ||
| buf.push_str("# TYPE password_hash_argon2id_total gauge\n"); | ||
| buf.push_str(&format!("password_hash_argon2id_total {}\n", a2id)); | ||
| buf.push_str("# HELP password_hash_unknown_total Users with unknown hash prefix.\n"); | ||
| buf.push_str("# TYPE password_hash_unknown_total gauge\n"); | ||
| buf.push_str(&format!("password_hash_unknown_total {}\n", unknown.max(0))); | ||
| buf.push_str("# HELP password_hash_total_count Total users with password hashes.\n"); | ||
| buf.push_str("# TYPE password_hash_total_count gauge\n"); | ||
| buf.push_str(&format!("password_hash_total_count {}\n", total)); | ||
| buf.push_str("# HELP password_hash_bcrypt_variant Users by bcrypt variant.\n"); | ||
| buf.push_str("# TYPE password_hash_bcrypt_variant gauge\n"); | ||
| buf.push_str(&format!( | ||
| "password_hash_bcrypt_variant{{variant=\"2a\"}} {}\n", | ||
| b2a | ||
| )); | ||
| buf.push_str(&format!( | ||
| "password_hash_bcrypt_variant{{variant=\"2b\"}} {}\n", | ||
| b2b | ||
| )); | ||
| buf.push_str(&format!( | ||
| "password_hash_bcrypt_variant{{variant=\"2y\"}} {}\n", | ||
| b2y | ||
| )); |
There was a problem hiding this comment.
The manual string building for each metric is quite repetitive. To improve maintainability and reduce code duplication, consider creating a helper function or macro to generate the Prometheus format for a metric. This would make the main handler logic cleaner and less prone to copy-paste errors.
For example, you could have a helper function:
fn write_gauge(buf: &mut String, name: &str, help: &str, value: impl std::fmt::Display) {
buf.push_str(&format!("# HELP {} {}\n", name, help));
buf.push_str(&format!("# TYPE {} gauge\n", name));
buf.push_str(&format!("{} {}\n", name, value));
}1 participant
Adds:\n- Updated PR47 verification report with canonical metrics & deprecation notice.\n- README monitoring section (health vs metrics, Prometheus config, quick consistency check).\n- Consistency script: scripts/check_metrics_consistency.sh.\n\nNote: Requires API running at localhost:8012 for script success.\n