refactor(secrets): pair Vault credential failure logs and metrics#3747
Conversation
|
@coderabbitai full_review, thanks! |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
Summary by CodeRabbit
WalkthroughVault failure observability now uses specialized credential events and metric-only recorders for service-account and certificate failures. Operation handlers consume returned status codes where needed, and regression coverage validates metrics, logs, credential keys, and error context. ChangesVault failure observability
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
🐇🔍 ✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 11 minutes. |
|
@coderabbitai full_review, thanks! |
|
🐇🔍 ✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/secrets/src/forge_vault.rs (2)
275-314: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing
credential_keycontext on write/delete failure events.
VaultCredentialsSetFailedandVaultCredentialsDeleteFailedcarry onlyerror, unlikeVaultCredentialsNotFound/VaultCredentialsGetFailed, which also capturecredential_key. BothSetCredentialsHelperandDeleteCredentialsHelperalready holdself.key(used to build the Vault path at Lines 711/725/765), so this context is available but discarded — weakening diagnosability for write/delete failures relative to reads. See the paired comment on the recording functions (Lines 671-689) for the full fix, including struct field additions here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/secrets/src/forge_vault.rs` around lines 275 - 314, The VaultCredentialsSetFailed and VaultCredentialsDeleteFailed event structs are missing credential_key context. Add a credential_key field matching the existing field definition used by VaultCredentialsNotFound and VaultCredentialsGetFailed, then ensure the SetCredentialsHelper and DeleteCredentialsHelper recording functions populate it from self.key when emitting these events.
671-689: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
credential_keytorecord_vault_credentials_set_error/delete_errorfor parity with the get-path.These two functions emit
VaultCredentialsSetFailed/VaultCredentialsDeleteFailedwithout acredential_key, unlikerecord_vault_credentials_get_errorabove, which threadscredential_keythrough for the read path. Both call sites already haveself.keyin scope (Lines 738-739 inSetCredentialsHelper::execute, Lines 775-776 inDeleteCredentialsHelper::execute), so this is a straightforward, low-risk addition that closes an observability gap for troubleshooting failed writes/deletes.♻️ Proposed fix threading credential_key through set/delete recorders
struct VaultCredentialsSetFailed { #[label] request_type: VaultRequestType, #[label] http_response_status_code: VaultFailureStatusCode, + #[context] + credential_key: String, #[context] error: String, }struct VaultCredentialsDeleteFailed { #[label] request_type: VaultRequestType, #[label] http_response_status_code: VaultFailureStatusCode, + #[context] + credential_key: String, #[context] error: String, }-fn record_vault_credentials_set_error(err: &ClientError) -> Option<u16> { +fn record_vault_credentials_set_error(err: &ClientError, credential_key: &CredentialKey) -> Option<u16> { let status_code = vault_client_error_status(err); emit(VaultCredentialsSetFailed { request_type: VaultRequestType::SetCredentials, http_response_status_code: VaultFailureStatusCode(status_code), + credential_key: credential_key.to_key_str().into_owned(), error: format!("{err:?}"), }); status_code } -fn record_vault_credentials_delete_error(err: &ClientError) -> Option<u16> { +fn record_vault_credentials_delete_error(err: &ClientError, credential_key: &CredentialKey) -> Option<u16> { let status_code = vault_client_error_status(err); emit(VaultCredentialsDeleteFailed { request_type: VaultRequestType::DeleteCredentials, http_response_status_code: VaultFailureStatusCode(status_code), + credential_key: credential_key.to_key_str().into_owned(), error: format!("{err:?}"), }); status_code }And update the call sites accordingly:
- let _secret_version_metadata = vault_response.inspect_err(|err| { - record_vault_credentials_set_error(err); - })?; + let _secret_version_metadata = vault_response.inspect_err(|err| { + record_vault_credentials_set_error(err, self.key); + })?;- let _secret_version_metadata = vault_response.inspect_err(|err| { - record_vault_credentials_delete_error(err); - })?; + let _secret_version_metadata = vault_response.inspect_err(|err| { + record_vault_credentials_delete_error(err, self.key); + })?;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/secrets/src/forge_vault.rs` around lines 671 - 689, Update record_vault_credentials_set_error and record_vault_credentials_delete_error to accept a credential_key argument and include it in their emitted VaultCredentialsSetFailed and VaultCredentialsDeleteFailed events, matching record_vault_credentials_get_error. Pass self.key from SetCredentialsHelper::execute and DeleteCredentialsHelper::execute at their existing error-recording call sites.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/secrets/src/forge_vault.rs`:
- Around line 275-314: The VaultCredentialsSetFailed and
VaultCredentialsDeleteFailed event structs are missing credential_key context.
Add a credential_key field matching the existing field definition used by
VaultCredentialsNotFound and VaultCredentialsGetFailed, then ensure the
SetCredentialsHelper and DeleteCredentialsHelper recording functions populate it
from self.key when emitting these events.
- Around line 671-689: Update record_vault_credentials_set_error and
record_vault_credentials_delete_error to accept a credential_key argument and
include it in their emitted VaultCredentialsSetFailed and
VaultCredentialsDeleteFailed events, matching
record_vault_credentials_get_error. Pass self.key from
SetCredentialsHelper::execute and DeleteCredentialsHelper::execute at their
existing error-recording call sites.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ec6c45b6-511e-4e14-adb0-42b5f5444beb
📒 Files selected for processing (1)
crates/secrets/src/forge_vault.rs
chet
changed the title
|
Keeping |
|
@coderabbitai full_review, thanks! |
|
✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 17 minutes. |
|
Hey @coderabbitai can you do a full_review when you get a chance? CICD isn't letting me continue because the CodeRabbit check "failed". Thanks! |
|
🐇🔍 ✅ Action performedFull review finished. |
chet
changed the title
Vault credential failures incremented `carbide_api_vault_requests_failed_total` and then wrote a separate diagnostic for the same request outcome. Four credential-specific sibling `Event`s now own those paired signals while preserving each historical log level, message, and context shape. The existing metric-only `VaultRequestFailed` continues to cover service-account and certificate failures, and the frozen request/status labels remain unchanged. A table-driven test covers all six failure shapes without adding irrelevant empty context to any operation. This supports NVIDIA#3735 Signed-off-by: Chet Nichols III <chetn@nvidia.com>
|
@coderabbitai full_review, thanks! |
|
✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 20 minutes. |
|
Hey @coderabbitai can you do a full_review when you get a chance? CICD isn't letting me continue because the CodeRabbit check "failed". Thanks! |
|
🐇🔍 ✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 12 minutes. |
|
@coderabbitai full_review, thanks! |
|
🐇🔍 ✅ Action performedFull review finished. |
2 participants
Vault credential failures incremented
carbide_api_vault_requests_failed_totaland then wrote a separate diagnostic for the same request outcome.Four credential-specific sibling
Events now own those paired signals while preserving each historical log level, message, and context shape. The existing metric-onlyVaultRequestFailedcontinues to cover service-account and certificate failures, and the frozen request/status labels remain unchanged.A table-driven test covers all six failure shapes without adding irrelevant empty context to any operation.
Related issues
Type of Change
Breaking Changes
Testing
The six failure shapes share a
check_valuestable that asserts the returned status, counter delta, and complete log observation for each request type.Verified with:
cargo test -p carbide-secrets(49 passed)cargo make format-nightlycargo make clippycargo make carbide-lintscargo make check-event-namescargo xtask check-metric-docsAdditional Notes
CodeRabbit suggested adding
credential_keyto set/delete failures. That would expand their historical log schemas, so this compatibility pass deliberately leaves those fields out; the rationale is also recorded in the PR conversation.Closes #3735