diff --git a/shotover-proxy/benches/windsock/profilers.rs b/shotover-proxy/benches/windsock/profilers.rs index 3911446c8..f55b34e4f 100644 --- a/shotover-proxy/benches/windsock/profilers.rs +++ b/shotover-proxy/benches/windsock/profilers.rs @@ -56,7 +56,14 @@ impl ProfilerRunner { }; self.samply = if self.run_samply { if let Some(shotover) = &shotover { - Some(Samply::run(self.results_path.clone(), shotover.child().id().unwrap()).await) + Some( + Samply::run( + self.bench_name.clone(), + self.results_path.clone(), + shotover.child().id().unwrap(), + ) + .await, + ) } else { panic!("samply not supported when benching without shotover") } diff --git a/shotover-proxy/benches/windsock/profilers/samply.rs b/shotover-proxy/benches/windsock/profilers/samply.rs index 56d2a60e4..36e841049 100644 --- a/shotover-proxy/benches/windsock/profilers/samply.rs +++ b/shotover-proxy/benches/windsock/profilers/samply.rs @@ -1,10 +1,15 @@ use std::path::PathBuf; use tokio::task::JoinHandle; +use windsock::ReportArchive; -pub struct Samply(JoinHandle<()>); +pub struct Samply { + bench_name: String, + handle: JoinHandle<()>, + output_file: String, +} impl Samply { - pub async fn run(results_path: PathBuf, pid: u32) -> Samply { + pub async fn run(bench_name: String, results_path: PathBuf, pid: u32) -> Samply { run_command( "cargo", &[ @@ -13,11 +18,16 @@ impl Samply { "--git", "https://github.com/mstange/samply", "--rev", - "4c8d5eb164e44c4eda1b29de116f5ea546d64c65", + "59a03a716cadab7835e3a961d0107ec797e458ec", ], ) .await; - let output_file = results_path.join("samply.json"); + let output_file = results_path + .join("samply.json") + .as_os_str() + .to_str() + .unwrap() + .to_owned(); let home = std::env::var("HOME").unwrap(); // Run `sudo ls` so that we can get sudo to stop asking for password for a while. @@ -26,28 +36,38 @@ impl Samply { // But that would require a bunch of work so its out of scope for now. run_command("sudo", &["ls"]).await; - Samply(tokio::spawn(async move { - run_command( - "sudo", - &[ - // specify the full path as CI has trouble finding it for some reason. - &format!("{home}/.cargo/bin/samply"), - "record", - "-p", - &pid.to_string(), - "--save-only", - "--profile-name", - results_path.file_name().unwrap().to_str().unwrap(), - "--output", - output_file.as_os_str().to_str().unwrap(), - ], - ) - .await; - })) + Samply { + bench_name, + output_file: output_file.clone(), + handle: tokio::spawn(async move { + run_command( + "sudo", + &[ + // specify the full path as CI has trouble finding it for some reason. + &format!("{home}/.cargo/bin/samply"), + "record", + "-p", + &pid.to_string(), + "--save-only", + "--profile-name", + results_path.file_name().unwrap().to_str().unwrap(), + "--output", + &output_file, + ], + ) + .await; + }), + } } pub fn wait(self) { - futures::executor::block_on(self.0).unwrap(); + futures::executor::block_on(self.handle).unwrap(); + let mut report = ReportArchive::load(&self.bench_name).unwrap(); + report.info_messages.push(format!( + "To open profiler results run: samply load {}", + self.output_file + )); + report.save(); } } diff --git a/windsock/src/report.rs b/windsock/src/report.rs index cd20d53cc..f31e04228 100644 --- a/windsock/src/report.rs +++ b/windsock/src/report.rs @@ -8,25 +8,46 @@ use tokio::sync::mpsc::UnboundedReceiver; #[derive(Debug, Serialize, Deserialize)] pub enum Report { + /// Indicates the warmup is over and the benchmark has begun. + /// Any Completed/Errored Events received before this are considered warmups and discarded. Start, + + /// Indicates a response came back from the service. + /// The Duration should be the time between the request being sent and the response being received QueryCompletedIn(Duration), + + /// Indicates an an error response came back from the service. QueryErrored { + /// The time between the request being sent and the response being received completed_in: Duration, + /// The error message received from the service or the local error that occured while trying to communicate with the service. message: String, }, + + /// Indicates a pubsub produce ack came back from the service. + /// The Duration should be the time between the request being sent and the response being received ProduceCompletedIn(Duration), + + /// Indicates a pubsub produce error response came back from the service. ProduceErrored { completed_in: Duration, message: String, }, + /// Indicates a pubsub consume response comes back from the service. ConsumeCompleted, - ConsumeErrored { - message: String, - }, + + /// Indicates pubsub consume error response came back from the service. + ConsumeErrored { message: String }, + + /// Indicates a second has passed for the benchmarker SecondPassed(Duration), - /// contains the time that the test ran for + + /// Contains the time that the test ran for FinishedIn(Duration), + /// Adds a note that will be visible to the user when viewing the benchmark results. + AddInfoMessage(String), + /// Ignore all other reports and use the ManualReport as the only source of benchmark metrics. /// Do not use this under normal circumstances. /// Instead this should only be used if you have an independent benchmarker that you want to call from windsock and include in windsocks results. @@ -112,7 +133,8 @@ pub struct ReportArchive { pub(crate) operations_report: Option, pub(crate) pubsub_report: Option, pub metrics: Vec, - pub(crate) error_messages: Vec, + pub error_messages: Vec, + pub info_messages: Vec, } #[derive(Clone, Debug, Serialize, Deserialize, Default)] @@ -341,12 +363,14 @@ pub(crate) async fn report_builder( let mut total_operation_time = Duration::from_secs(0); let mut total_produce_time = Duration::from_secs(0); let mut error_messages = vec![]; + let mut info_messages = vec![]; while let Some(report) = rx.recv().await { match report { Report::Start => { started = Some(OffsetDateTime::now_utc()); } + Report::AddInfoMessage(message) => info_messages.push(message), Report::QueryCompletedIn(duration) => { let report = operations_report.get_or_insert_with(OperationsReport::default); if started.is_some() { @@ -490,6 +514,7 @@ pub(crate) async fn report_builder( tags, pubsub_report, error_messages, + info_messages, operations_report, metrics: vec![], }; diff --git a/windsock/src/tables.rs b/windsock/src/tables.rs index c2cca1444..aa5fae1b6 100644 --- a/windsock/src/tables.rs +++ b/windsock/src/tables.rs @@ -735,8 +735,7 @@ fn base(reports: &[ReportColumn], table_type: &str) { ); println!("{}", style(error).red().bold()); for (i, message) in report.current.error_messages.iter().enumerate() { - let line = format!(" {i}. {message}"); - println!("{}", line); + println!(" {i}. {message}"); } } @@ -748,8 +747,7 @@ fn base(reports: &[ReportColumn], table_type: &str) { ); println!("{}", style(error).red().bold()); for (i, message) in report.current.error_messages.iter().enumerate() { - let line = format!(" {i}. {message}"); - println!("{}", line); + println!(" {i}. {message}"); } } } @@ -766,9 +764,17 @@ fn base(reports: &[ReportColumn], table_type: &str) { !x.current.running_in_release || x.baseline .as_ref() - .map(|x| !x.error_messages.is_empty()) + .map(|x| !x.running_in_release) + .unwrap_or(false) + }); + let info_found = reports.iter().any(|x| { + !x.current.info_messages.is_empty() + || x.baseline + .as_ref() + .map(|x| !x.info_messages.is_empty()) .unwrap_or(false) }); + if errors_found && not_running_in_release_found { // ensure these two sections are kept apart println!(); @@ -793,6 +799,34 @@ fn base(reports: &[ReportColumn], table_type: &str) { } } } + + #[allow(clippy::nonminimal_bool)] + if info_found + && (not_running_in_release_found || (errors_found && !not_running_in_release_found)) + { + // ensure these two sections are kept apart + println!(); + } + + for report in reports { + if !report.current.info_messages.is_empty() { + let error = format!("notes for {}", report.current.tags.get_name()); + println!("{}", style(error).blue().bold()); + for (i, message) in report.current.info_messages.iter().enumerate() { + println!(" {i}. {message}"); + } + } + + if let Some(baseline) = &report.baseline { + if !baseline.info_messages.is_empty() { + let error = format!("notes for baseline {}", report.current.tags.get_name()); + println!("{}", style(error).blue().bold()); + for (i, message) in report.current.info_messages.iter().enumerate() { + println!(" {i}. {message}"); + } + } + } + } } fn duration_ms(duration: Duration) -> String {