Audit of warn! and error! - #5640
Conversation
| // TODO: May need to split this from module errors vs host errors | ||
| log::warn!("Internal error while invoking procedure {procedure}: {e:#}"); | ||
| StatusCode::INTERNAL_SERVER_ERROR |
There was a problem hiding this comment.
Could we preserve whether this failure came from module code or the host? Module errors should remain low-level and eventually go to module logs, while internal host failures may need to stay at error! for alerting.
| if ws_version == WsVersion::V1 | ||
| && let MessageHandleError::Execution(err) = e | ||
| { | ||
| // TODO: Review log level after guest/client execution errors can be distinguished from internal failures. | ||
| log::error!("{err:#}"); | ||
| // If the send task has exited, also exit this recv task. | ||
| if unordered_tx.send(err.into()).is_err() { |
There was a problem hiding this comment.
This looks to combine module errors with possible host errors, that leads me to lean error! for now
| .map_err(|e| { | ||
| // TODO: Review log level after user SQL errors can be distinguished from internal database failures. | ||
| log::warn!("{e}"); | ||
| (StatusCode::BAD_REQUEST, e.to_string()) |
There was a problem hiding this comment.
Same as other areas we might want to split this up so we can use error! if there is a database failure.
| // TODO: Review each caller and split this helper by the appropriate log severity. | ||
| pub fn log_and_500(e: impl std::fmt::Display) -> ErrorResponse { | ||
| log::error!("internal error: {e:#}"); | ||
| (StatusCode::INTERNAL_SERVER_ERROR, format!("{e:#}")).into() |
There was a problem hiding this comment.
This seems to be very broadly used which makes it hard to know if it should be reduced from error! my gut reaction is to change it to warn! and slowly clean up it's use?
| // TODO: Review log level after configured issuers can be distinguished from arbitrary token issuers. | ||
| log::warn!("Error fetching public key for issuer {raw_issuer}: {e:?}"); |
There was a problem hiding this comment.
This one I'm not 100% sure if we need to split up or count everything as warn!
| // TODO: Review log level after migration/user errors can be distinguished from internal database failures. | ||
| log::warn!("Database update failed: {} @ {}", e, stdb.database_identity()); | ||
| system_logger.warn(&format!("Database update failed: {e}")); | ||
| let (_, tx_metrics, reducer) = stdb.rollback_mut_tx(tx); |
There was a problem hiding this comment.
Another spot where it looks like real failures can be mixed in with user issues.
| // TODO: Review log level after guest view errors can be distinguished from materialization failures. | ||
| log::error!("Error materializing view `{view_name}`: {err:?}"); | ||
| ViewOutcome::Failed(format!("Error materializing view `{view_name}`: {err}")) |
There was a problem hiding this comment.
This one looked similar as above but I'm not certain if it's true that it can be mixed here.
1 participant
Description of Changes
Reviewed all warn! and error! lines for host side issues to get ready for alerting.
API and ABI breaking changes
No breaking changes.
Expected complexity level and risk
1 - Only changes to logging
Testing
cargo testagainst impacted crates