chore: canonical password hash metrics + legacy gauge compatibility #48
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 refining the Prometheus metrics for password hash algorithms. The primary goal is to introduce a set of canonical metrics that offer a clearer and more comprehensive understanding of how different password hashing schemes are utilized across the user base. Simultaneously, it ensures a smooth transition for existing monitoring dashboards by retaining and explicitly deprecating the older metric formats, allowing time for updates without immediate disruption. 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 introduces canonical password hash metrics while maintaining backward compatibility with legacy dashboard metrics. The changes refactor the metrics handler to provide standardized metric names alongside deprecated legacy metrics.
- Added canonical password hash metrics with clear naming conventions
- Maintained legacy jive_password_hash_users metrics marked as deprecated
- Enhanced SQL query to include total count and unknown hash detection
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
jive-api/src/metrics.rs
Outdated
| let b2a = row.try_get::<i64,_>("b2a").unwrap_or(0); | ||
| let b2b = row.try_get::<i64,_>("b2b").unwrap_or(0); | ||
| let b2y = row.try_get::<i64,_>("b2y").unwrap_or(0); | ||
| let a2id = row.try_get::<i64,_>("a2id").unwrap_or(0); | ||
| let total = row.try_get::<i64,_>("total").unwrap_or(0); |
There was a problem hiding this comment.
Missing spaces after commas in generic type parameters. Should be <i64, _> instead of <i64,_> for consistency with Rust formatting conventions.
| let b2a = row.try_get::<i64,_>("b2a").unwrap_or(0); | |
| let b2b = row.try_get::<i64,_>("b2b").unwrap_or(0); | |
| let b2y = row.try_get::<i64,_>("b2y").unwrap_or(0); | |
| let a2id = row.try_get::<i64,_>("a2id").unwrap_or(0); | |
| let total = row.try_get::<i64,_>("total").unwrap_or(0); | |
| let b2a = row.try_get::<i64, _>("b2a").unwrap_or(0); | |
| let b2b = row.try_get::<i64, _>("b2b").unwrap_or(0); | |
| let b2y = row.try_get::<i64, _>("b2y").unwrap_or(0); | |
| let a2id = row.try_get::<i64, _>("a2id").unwrap_or(0); | |
| let total = row.try_get::<i64, _>("total").unwrap_or(0); |
Copilot uses AI. Check for mistakes.
jive-api/src/metrics.rs
Outdated
| (b2a,b2b,b2y,a2id,total,unknown) | ||
| } else { (0,0,0,0,0,0) }; |
There was a problem hiding this comment.
Missing spaces after commas in tuple expressions. Should be (b2a, b2b, b2y, a2id, total, unknown) and (0, 0, 0, 0, 0, 0) for consistency with Rust formatting conventions.
| (b2a,b2b,b2y,a2id,total,unknown) | |
| } else { (0,0,0,0,0,0) }; | |
| (b2a, b2b, b2y, a2id, total, unknown) | |
| } else { (0, 0, 0, 0, 0, 0) }; |
Copilot uses AI. Check for mistakes.
| 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) operation suggests that unknown could be negative, but this should not be possible since it's calculated as total - (sum of known hashes). If this guard is necessary, it indicates a potential logic issue that should be investigated rather than silently corrected.
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 new canonical metrics for password hash distribution while maintaining backward compatibility with legacy metrics for a smooth transition. The changes are well-contained within jive-api/src/metrics.rs. The implementation correctly and efficiently fetches hash counts from the database and formats them for Prometheus. I have one suggestion to refactor the data fetching logic to improve long-term readability and maintainability.
jive-api/src/metrics.rs
Outdated
| let (b2a, b2b, b2y, a2id, total, unknown) = if let Ok(row) = sqlx::query( | ||
| "SELECT \ | ||
| COUNT(*) FILTER (WHERE password_hash LIKE '$2a$%') AS b2a,\ | ||
| COUNT(*) FILTER (WHERE password_hash LIKE '$2b$%') AS b2b,\ | ||
| COUNT(*) FILTER (WHERE password_hash LIKE '$2y$%') AS b2y,\ | ||
| COUNT(*) FILTER (WHERE password_hash LIKE '$argon2id$%') AS a2id\ | ||
| FROM users", | ||
| ) | ||
| .fetch_one(pool) | ||
| .await | ||
| { | ||
| COUNT(*) FILTER (WHERE password_hash LIKE '$argon2id$%') AS a2id,\ | ||
| COUNT(*) AS total\ | ||
| FROM users" | ||
| ).fetch_one(pool).await { | ||
| use sqlx::Row; | ||
| ( | ||
| row.try_get::<i64, _>("b2a").unwrap_or(0), | ||
| row.try_get::<i64, _>("b2b").unwrap_or(0), | ||
| row.try_get::<i64, _>("b2y").unwrap_or(0), | ||
| row.try_get::<i64, _>("a2id").unwrap_or(0), | ||
| ) | ||
| } else { | ||
| (0, 0, 0, 0) | ||
| }; | ||
| let b2a = row.try_get::<i64,_>("b2a").unwrap_or(0); | ||
| let b2b = row.try_get::<i64,_>("b2b").unwrap_or(0); | ||
| let b2y = row.try_get::<i64,_>("b2y").unwrap_or(0); | ||
| let a2id = row.try_get::<i64,_>("a2id").unwrap_or(0); | ||
| let total = row.try_get::<i64,_>("total").unwrap_or(0); | ||
| let unknown = total - (b2a + b2b + b2y + a2id); | ||
| (b2a,b2b,b2y,a2id,total,unknown) | ||
| } else { (0,0,0,0,0,0) }; |
There was a problem hiding this comment.
To improve readability and maintainability, consider introducing a struct to hold the password hash counts. This approach makes the code more explicit by using named fields instead of a large, 6-element tuple, which can be error-prone to destructure and harder to extend. This change encapsulates the data-fetching logic and makes it easier to manage if more hash types are added in the future.
let (b2a, b2b, b2y, a2id, total, unknown) = {
#[derive(Default)]
struct HashCounts {
b2a: i64,
b2b: i64,
b2y: i64,
a2id: i64,
total: i64,
}
let counts = if let Ok(row) = sqlx::query(
"SELECT \
COUNT(*) FILTER (WHERE password_hash LIKE '$2a$%') AS b2a,\
COUNT(*) FILTER (WHERE password_hash LIKE '$2b$%') AS b2b,\
COUNT(*) FILTER (WHERE password_hash LIKE '$2y$%') AS b2y,\
COUNT(*) FILTER (WHERE password_hash LIKE '$argon2id$%') AS a2id,\
COUNT(*) AS total\
FROM users"
).fetch_one(pool).await {
use sqlx::Row;
HashCounts {
b2a: row.try_get("b2a").unwrap_or(0),
b2b: row.try_get("b2b").unwrap_or(0),
b2y: row.try_get("b2y").unwrap_or(0),
a2id: row.try_get("a2id").unwrap_or(0),
total: row.try_get("total").unwrap_or(0),
}
} else {
HashCounts::default()
};
let unknown = counts.total - (counts.b2a + counts.b2b + counts.b2y + counts.a2id);
(counts.b2a, counts.b2b, counts.b2y, counts.a2id, counts.total, unknown)
};1 participant
Changes:\n- Add canonical metrics: \n * password_hash_bcrypt_total\n * password_hash_argon2id_total\n * password_hash_unknown_total\n * password_hash_total_count\n * password_hash_bcrypt_variant{variant=2a|2b|2y}\n- Keep legacy (DEPRECATED) jive_password_hash_users{algo=...} for dashboard transition.\n- Keep jive_password_rehash_total counter unchanged.\n\nAll tests pass (24/24).