Standardize error handling with error-stack #197
Conversation
Unified error reporting across storage and signing modules allows for better debugging and traceability. Bare error enums previously lacked context on where failures originated. Using meaningful reports with attached context ensures that logs provide actionable information for operations teams without requiring changes to the core error variants. Resolves: #191
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
👍 Looks good overall. Some improvement will get us closer to consistency. Thanks
crates/common/src/fastly_storage.rs
Outdated
| String::from_utf8(bytes).map_err(|e| TrustedServerError::Configuration { | ||
| message: format!("Failed to decode secret as UTF-8: {}", e), | ||
| String::from_utf8(bytes).change_context(TrustedServerError::Configuration { | ||
| message: format!("Failed to decode secret as UTF-8: {}", key), |
There was a problem hiding this comment.
🔧 We should not include key here.
| let active_kids_str = | ||
| store | ||
| .get("active-kids") | ||
| .change_context(TrustedServerError::Configuration { |
There was a problem hiding this comment.
🔧 change_context() to errors that already come wrapped in Report<TrustedServerError> from FastlyConfigStore::get(). This creates double-wrapping which may produce verbose error chains:
use attach_printable()
let active_kids_str = store
.get("active-kids")
.attach_printable("while fetching active kids list")?;
There was a problem hiding this comment.
attach_printable is deprecated since 0.6.0, using attach instead
| let key_bytes = | ||
| secret_store | ||
| .get(&key_id) | ||
| .change_context(TrustedServerError::Configuration { |
There was a problem hiding this comment.
🔧 change_context() to errors that already come wrapped in Report<TrustedServerError. This creates double-wrapping which may produce verbose error chains:
use attach_printable()
let key_bytes = secret_store
.get(&key_id)
.attach_printable(format!("Failed to get signing key for kid: {}", key_id))?;
There was a problem hiding this comment.
attach_printable is deprecated since 0.6.0, using attach instead
crates/common/src/fastly_storage.rs
Outdated
| message: format!("Failed to ensure API backend: {}", e), | ||
| } | ||
| })?; | ||
| message: "Failed to ensure API backend".to_string(), |
There was a problem hiding this comment.
🔧 Should use .attach_printable_lazy(|| e.to_string())
CONTRIBUTING.md
Outdated
|
|
||
| Consistency is the most important. Following the existing Rust style, formatting, and naming conventions of the file you are modifying and of the overall project. Failure to do so will result in a prolonged review process that has to focus on updating the superficial aspects of your code, rather than improving its functionality and performance. | ||
|
|
||
| Style and format will be enforced with a linter when PR is crated. |
There was a problem hiding this comment.
⛏️ Please fix typo crated -> created
|
|
||
| #[test] | ||
| fn test_request_signer_sign() { | ||
| // We propagate errors in tests too, or unwrap. |
There was a problem hiding this comment.
⛏️ Change to // Report unwraps print full error chain on test failure
Using on errors that are already of the correct type causes redundant nesting in the error report. This switches to to add context without changing the error type, producing cleaner error chains. Resolves: #191
…se-between-reporttrustedservererror-and-bare-trustedservererror
Unified error reporting across storage and signing modules allows for better debugging and traceability. Bare error enums previously lacked context on where failures originated. Using meaningful reports with attached context ensures that logs provide actionable information for operations teams without requiring changes to the core error variants.
Resolves: #191