From 67b32c3d77e2acd7ccb3b43fdfcd213ea049ce38 Mon Sep 17 00:00:00 2001 From: Andreas Umbreit Date: Tue, 14 Nov 2023 11:34:26 +0100 Subject: [PATCH] check_http: Prevent empty summaries/details Instead of allowing summaries/details with "" as text, we now yield a None instead of a CheckResult. The builder functions that come with this change also make the construction a bit easier. CMK-14257 Change-Id: I9f9f90c38de9916421c5d9032801ccfe2795ff0e --- packages/check-http/src/checking.rs | 184 +++++++++++++++------------- packages/check-http/src/output.rs | 50 +++----- packages/check-http/src/runner.rs | 29 ++--- 3 files changed, 122 insertions(+), 141 deletions(-) diff --git a/packages/check-http/src/checking.rs b/packages/check-http/src/checking.rs index ca27002a68a..a8b8af2d5f5 100644 --- a/packages/check-http/src/checking.rs +++ b/packages/check-http/src/checking.rs @@ -150,7 +150,7 @@ impl Display for Metric { pub struct CheckItem { pub state: State, - pub text: String, + text: String, } impl Display for CheckItem { @@ -169,12 +169,35 @@ impl Display for CheckItem { } } +impl CheckItem { + fn new(state: State, text: &str) -> Option { + if text.is_empty() { + return None; + }; + + Some(Self { + state, + text: text.to_string(), + }) + } +} + pub enum CheckResult { Summary(CheckItem), Details(CheckItem), Metric(Metric), } +impl CheckResult { + pub fn summary(state: State, text: &str) -> Option { + CheckItem::new(state, text).map(Self::Summary) + } + + pub fn details(state: State, text: &str) -> Option { + CheckItem::new(state, text).map(Self::Details) + } +} + pub struct CheckParameters { pub onredirect: OnRedirect, pub page_size: Option>, @@ -188,18 +211,21 @@ pub fn collect_response_checks( params: CheckParameters, ) -> Vec { vec![ - check_status(response.status, response.version, params.onredirect) - .map(CheckResult::Summary), - check_body(response.body, params.page_size).map(CheckResult::Summary), - check_response_time(response_time, params.response_time_levels).map(CheckResult::Summary), - check_document_age(&response.headers, params.document_age_levels).map(CheckResult::Summary), + check_status(response.status, response.version, params.onredirect), + check_body(response.body, params.page_size), + check_response_time(response_time, params.response_time_levels), + check_document_age(&response.headers, params.document_age_levels), ] .into_iter() .flatten() .collect() } -fn check_status(status: StatusCode, version: Version, onredirect: OnRedirect) -> Option { +fn check_status( + status: StatusCode, + version: Version, + onredirect: OnRedirect, +) -> Option { let state = if status.is_client_error() { State::Warn } else if status.is_server_error() { @@ -214,72 +240,60 @@ fn check_status(status: StatusCode, version: Version, onredirect: OnRedirect) -> State::Ok }; - Some(CheckItem { - state, - text: format!("{:?} {}", version, status), - }) + CheckResult::summary(state, &format!("{:?} {}", version, status)) } fn check_body( body: Option>, page_size_limits: Option>, -) -> Option { +) -> Option { let body = body?; match body { - Ok(bd) => Some(check_page_size(bd.len(), page_size_limits)), - Err(_) => Some(CheckItem { - state: State::Crit, - text: "Error fetching the reponse body".to_string(), - }), + Ok(bd) => check_page_size(bd.len(), page_size_limits), + Err(_) => CheckResult::summary(State::Crit, "Error fetching the reponse body"), } } -fn check_page_size(page_size: usize, page_size_limits: Option>) -> CheckItem { +fn check_page_size( + page_size: usize, + page_size_limits: Option>, +) -> Option { let state = page_size_limits .and_then(|bounds| bounds.evaluate(&page_size, State::Warn)) .unwrap_or(State::Ok); - CheckItem { - state, - text: format!("Page size: {} bytes", page_size), - } + CheckResult::summary(state, &format!("Page size: {} bytes", page_size)) } fn check_response_time( response_time: Duration, response_time_levels: Option>, -) -> Option { +) -> Option { let state = response_time_levels .and_then(|levels| levels.evaluate(&response_time.as_secs_f64())) .unwrap_or(State::Ok); - Some(CheckItem { + CheckResult::summary( state, - text: format!( + &format!( "Response time: {}.{}s", response_time.as_secs(), response_time.subsec_millis() ), - }) + ) } fn check_document_age( headers: &HeaderMap, document_age_levels: Option>, -) -> Option { +) -> Option { let document_age_levels = document_age_levels?; let now = SystemTime::now(); - let cr_no_document_age = Some(CheckItem { - state: State::Crit, - text: "Can't determine document age".to_string(), - }); - let cr_document_age_error = Some(CheckItem { - state: State::Crit, - text: "Can't decode document age".to_string(), - }); + let cr_no_document_age = CheckResult::summary(State::Crit, "Can't determine document age"); + let cr_document_age_error = CheckResult::summary(State::Crit, "Can't decode document age"); let age_header = headers.get("last-modified").or(headers.get("date")); let Some(age) = age_header else { @@ -298,127 +312,127 @@ fn check_document_age( let state = document_age_levels.evaluate(&age.as_secs())?; //TODO(au): Specify "too old" in Output - Some(CheckItem { - state, - text: "Document age too old".to_string(), - }) + CheckResult::summary(state, "Document age too old") +} + +#[cfg(test)] +mod test_helper { + use super::CheckResult; + use crate::checking::State; + pub fn has_state(cr: Option, state: State) -> bool { + let Some(cr) = cr else { return false }; + match cr { + CheckResult::Details(ci) | CheckResult::Summary(ci) => ci.state == state, + _ => false, + } + } } #[cfg(test)] mod test_check_page_size { + use super::test_helper::has_state; use crate::checking::check_page_size; use crate::checking::Bounds; use crate::checking::State; #[test] fn test_without_bounds() { - assert_eq!(check_page_size(42, None).state, State::Ok); + assert!(has_state(check_page_size(42, None), State::Ok)); } #[test] fn test_lower_within_bounds() { - assert_eq!( - check_page_size(42, Some(Bounds::lower(12))).state, + assert!(has_state( + check_page_size(42, Some(Bounds::lower(12))), State::Ok - ); + )); } #[test] fn test_lower_out_of_bounds() { - assert_eq!( - check_page_size(42, Some(Bounds::lower(56))).state, + assert!(has_state( + check_page_size(42, Some(Bounds::lower(56))), State::Warn - ); + )); } #[test] fn test_lower_and_higher_within_bounds() { - assert_eq!( - check_page_size(56, Some(Bounds::lower_upper(42, 100))).state, + assert!(has_state( + check_page_size(56, Some(Bounds::lower_upper(42, 100))), State::Ok - ); + )); } #[test] fn test_lower_and_higher_too_low() { - assert_eq!( - check_page_size(42, Some(Bounds::lower_upper(56, 100))).state, + assert!(has_state( + check_page_size(42, Some(Bounds::lower_upper(56, 100))), State::Warn - ); + )); } #[test] fn test_lower_and_higher_too_high() { - assert_eq!( - check_page_size(142, Some(Bounds::lower_upper(56, 100))).state, + assert!(has_state( + check_page_size(142, Some(Bounds::lower_upper(56, 100))), State::Warn - ); + )); } } #[cfg(test)] mod test_check_response_time { + use super::test_helper::has_state; use crate::checking::check_response_time; use crate::checking::{State, UpperLevels}; use std::time::Duration; #[test] fn test_unbounded() { - assert_eq!( - check_response_time(Duration::new(5, 0), None) - .unwrap() - .state, + assert!(has_state( + check_response_time(Duration::new(5, 0), None), State::Ok - ); + ),); } #[test] fn test_warn_within_bounds() { - assert_eq!( - check_response_time(Duration::new(5, 0), Some(UpperLevels::warn(6.))) - .unwrap() - .state, + assert!(has_state( + check_response_time(Duration::new(5, 0), Some(UpperLevels::warn(6.))), State::Ok - ); + )); } #[test] fn test_warn_is_warn() { - assert_eq!( - check_response_time(Duration::new(5, 0), Some(UpperLevels::warn(4.))) - .unwrap() - .state, + assert!(has_state( + check_response_time(Duration::new(5, 0), Some(UpperLevels::warn(4.))), State::Warn - ); + )); } #[test] fn test_warncrit_within_bounds() { - assert_eq!( - check_response_time(Duration::new(5, 0), Some(UpperLevels::warn_crit(6., 7.))) - .unwrap() - .state, + assert!(has_state( + check_response_time(Duration::new(5, 0), Some(UpperLevels::warn_crit(6., 7.))), State::Ok - ); + )); } #[test] fn test_warncrit_is_warn() { - assert_eq!( - check_response_time(Duration::new(5, 0), Some(UpperLevels::warn_crit(4., 6.))) - .unwrap() - .state, + assert!(has_state( + check_response_time(Duration::new(5, 0), Some(UpperLevels::warn_crit(4., 6.))), State::Warn - ); + )); } #[test] fn test_warncrit_is_crit() { - assert_eq!( - check_response_time(Duration::new(5, 0), Some(UpperLevels::warn_crit(2., 3.))) - .unwrap() - .state, + assert!(has_state( + check_response_time(Duration::new(5, 0), Some(UpperLevels::warn_crit(2., 3.))), State::Crit - ); + )); } } diff --git a/packages/check-http/src/output.rs b/packages/check-http/src/output.rs index 1751ee3af96..6e127027fd0 100644 --- a/packages/check-http/src/output.rs +++ b/packages/check-http/src/output.rs @@ -32,14 +32,10 @@ impl Display for Output { write!(f, "HTTP {}", self.worst_state)?; - let summaries = self - .check_results - .iter() - .filter_map(|cr| match cr { - CheckResult::Summary(check_item) => Some(check_item), - _ => None, - }) - .filter(|summ| !summ.text.is_empty()); + let summaries = self.check_results.iter().filter_map(|cr| match cr { + CheckResult::Summary(check_item) => Some(check_item), + _ => None, + }); write_joined(f, summaries, " - ", ", ")?; let metrics = self.check_results.iter().filter_map(|cr| match cr { @@ -48,14 +44,10 @@ impl Display for Output { }); write_joined(f, metrics, " | ", " ")?; - let details = self - .check_results - .iter() - .filter_map(|cr| match cr { - CheckResult::Details(check_item) => Some(check_item), - _ => None, - }) - .filter(|summ| !summ.text.is_empty()); + let details = self.check_results.iter().filter_map(|cr| match cr { + CheckResult::Details(check_item) => Some(check_item), + _ => None, + }); write_joined(f, details, "\n", "\n")?; Ok(()) @@ -88,20 +80,14 @@ impl Output { #[cfg(test)] mod test_output_format { use super::*; - use crate::checking::{CheckItem, Metric}; + use crate::checking::Metric; fn summary(state: State, text: &str) -> CheckResult { - CheckResult::Summary(CheckItem { - state, - text: text.to_string(), - }) + CheckResult::summary(state, text).unwrap() } fn details(state: State, text: &str) -> CheckResult { - CheckResult::Details(CheckItem { - state, - text: text.to_string(), - }) + CheckResult::details(state, text).unwrap() } fn metric( @@ -128,14 +114,9 @@ mod test_output_format { } #[test] + #[should_panic] fn test_merge_check_results_with_state_only() { - let cr1 = summary(State::Ok, ""); - let cr2 = summary(State::Ok, ""); - let cr3 = details(State::Ok, ""); - assert_eq!( - format!("{}", Output::from_check_results(vec![cr1, cr2, cr3])), - "HTTP OK" - ); + let _ = summary(State::Ok, ""); } #[test] @@ -184,11 +165,10 @@ mod test_output_format { } #[test] - fn test_merge_empty_checks_basic_metric() { - let cr1 = summary(State::Ok, ""); + fn test_basic_metric() { let m1 = metric("my_metric", 123., None, None, None, None); assert_eq!( - format!("{}", Output::from_check_results(vec![cr1, m1])), + format!("{}", Output::from_check_results(vec![m1])), "HTTP OK | my_metric=123;;;;" ); } diff --git a/packages/check-http/src/runner.rs b/packages/check-http/src/runner.rs index e33d9ec8a3c..0707bed0d93 100644 --- a/packages/check-http/src/runner.rs +++ b/packages/check-http/src/runner.rs @@ -4,7 +4,7 @@ use std::time::Instant; -use crate::checking::{self, CheckItem, CheckParameters, CheckResult, State}; +use crate::checking::{self, CheckParameters, CheckResult, State}; use crate::connection::ConnectionConfig; use crate::http::{self, ClientConfig, RequestConfig}; @@ -15,10 +15,7 @@ pub async fn collect_checks( check_params: CheckParameters, ) -> Vec { let Ok(request) = http::prepare_request(client_config, connection_cfg) else { - return vec![CheckResult::Summary(CheckItem { - state: State::Unknown, - text: "Error building the request".to_string(), - })]; + return vec![CheckResult::summary(State::Unknown, "Error building the request").unwrap()]; }; let now = Instant::now(); @@ -26,26 +23,16 @@ pub async fn collect_checks( Ok(resp) => resp, Err(err) => { if err.is_timeout() { - return vec![CheckResult::Summary(CheckItem { - state: State::Crit, - text: "timeout".to_string(), - })]; + return vec![CheckResult::summary(State::Crit, "timeout").unwrap()]; } else if err.is_connect() { - return vec![CheckResult::Summary(CheckItem { - state: State::Crit, - text: "Failed to connect".to_string(), - })]; + return vec![CheckResult::summary(State::Crit, "Failed to connect").unwrap()]; } else if err.is_redirect() { - return vec![CheckResult::Summary(CheckItem { - state: State::Crit, - text: err.to_string(), - })]; + return vec![CheckResult::summary(State::Crit, &err.to_string()).unwrap()]; // Hit one of max_redirs, sticky, stickyport } else { - return vec![CheckResult::Summary(CheckItem { - state: State::Unknown, - text: "Error while sending request".to_string(), - })]; + return vec![ + CheckResult::summary(State::Unknown, "Error while sending request").unwrap(), + ]; } } };