From 4d00c7bdb18b24ec1558bf1a21b8dbe154bdc499 Mon Sep 17 00:00:00 2001 From: Abid Omar Date: Tue, 25 Jan 2022 20:35:58 +0100 Subject: [PATCH 1/7] imp(error): log errors instead of panic --- src/pyroscope.rs | 69 ++++++++++++++++++++++++++++++++---------------- 1 file changed, 46 insertions(+), 23 deletions(-) diff --git a/src/pyroscope.rs b/src/pyroscope.rs index 95af6c54..b5ab0c91 100644 --- a/src/pyroscope.rs +++ b/src/pyroscope.rs @@ -225,32 +225,41 @@ impl Drop for PyroscopeAgent { fn drop(&mut self) { log::debug!("PyroscopeAgent::drop()"); - // Stop Timer - self.timer.drop_listeners().unwrap(); // Drop listeners - log::trace!("PyroscopeAgent - Dropped timer listeners"); - self.timer.handle.take().unwrap().join().unwrap().unwrap(); // Wait for the Timer thread to finish - log::trace!("PyroscopeAgent - Dropped timer thread"); + // Drop Timer listeners + match self.timer.drop_listeners() { + Ok(_) => log::trace!("PyroscopeAgent - Dropped timer listeners"), + Err(_) => log::error!("PyroscopeAgent - Error Dropping timer listeners"), + } + + // Wait for the Timer thread to finish + match self.timer.handle.take().unwrap().join() { + Ok(_) => log::trace!("PyroscopeAgent - Dropped timer thread"), + Err(_) => log::error!("PyroscopeAgent - Error Dropping timer thread"), + } // Stop the SessionManager - self.session_manager.push(SessionSignal::Kill).unwrap(); - log::trace!("PyroscopeAgent - Sent kill signal to SessionManager"); - self.session_manager - .handle - .take() - .unwrap() - .join() - .unwrap() - .unwrap(); - log::trace!("PyroscopeAgent - Dropped SessionManager thread"); + match self.session_manager.push(SessionSignal::Kill) { + Ok(_) => log::trace!("PyroscopeAgent - Sent kill signal to SessionManager"), + Err(_) => log::error!("PyroscopeAgent - Error sending kill signal to SessionManager"), + } + + // Stop SessionManager + match self.session_manager.handle.take().unwrap().join() { + Ok(_) => log::trace!("PyroscopeAgent - Dropped SessionManager thread"), + Err(_) => log::error!("PyroscopeAgent - Error Dropping SessionManager thread"), + } // Wait for main thread to finish - self.handle.take().unwrap().join().unwrap().unwrap(); - log::trace!("PyroscopeAgent - Dropped main thread"); + match self.handle.take().unwrap().join() { + Ok(_) => log::trace!("PyroscopeAgent - Dropped main thread"), + Err(_) => log::error!("PyroscopeAgent - Error Dropping main thread"), + } } } impl PyroscopeAgent { /// Short-hand for PyroscopeAgentBuilder::build(). This is a convenience method. + /// /// # Example /// ```ignore /// let agent = PyroscopeAgent::builder("http://localhost:8080", "my-app").build().unwrap(); @@ -266,7 +275,7 @@ impl PyroscopeAgent { /// let agent = PyroscopeAgent::builder("http://localhost:8080", "my-app").build().unwrap(); /// agent.start().unwrap(); /// ``` - pub fn start(&mut self) -> Result<()> { + fn _start(&mut self) -> Result<()> { log::debug!("PyroscopeAgent - Starting"); // Create a clone of Backend @@ -325,6 +334,13 @@ impl PyroscopeAgent { Ok(()) } + pub fn start(&mut self) { + match self._start() { + Ok(_) => log::trace!("PyroscopeAgent - Agent started"), + Err(_) => log::error!("PyroscopeAgent - Error starting agent"), + } + } + /// Stop the agent. The agent will stop profiling and send a last report to the server. /// # Example /// ```ignore @@ -333,7 +349,7 @@ impl PyroscopeAgent { /// // Expensive operation /// agent.stop().unwrap(); /// ``` - pub fn stop(&mut self) -> Result<()> { + fn _stop(&mut self) -> Result<()> { log::debug!("PyroscopeAgent - Stopping"); // get tx and send termination signal self.tx.take().unwrap().send(0)?; @@ -351,6 +367,13 @@ impl PyroscopeAgent { Ok(()) } + pub fn stop(&mut self) { + match self._stop() { + Ok(_) => log::trace!("PyroscopeAgent - Agent stopped"), + Err(_) => log::error!("PyroscopeAgent - Error stopping agent"), + } + } + /// Add tags. This will restart the agent. /// # Example /// ```ignore @@ -369,7 +392,7 @@ impl PyroscopeAgent { } // Stop Agent - self.stop()?; + self.stop(); // Convert &[(&str, &str)] to HashMap(String, String) let tags_hashmap: HashMap = tags @@ -382,7 +405,7 @@ impl PyroscopeAgent { self.config.tags.extend(tags_hashmap); // Restart Agent - self.start()?; + self.start(); Ok(()) } @@ -407,7 +430,7 @@ impl PyroscopeAgent { } // Stop Agent - self.stop()?; + self.stop(); // Iterate through every tag tags.iter().for_each(|key| { @@ -416,7 +439,7 @@ impl PyroscopeAgent { }); // Restart Agent - self.start()?; + self.start(); Ok(()) } From 0665f41ffff223547fc545cedbbef477dab8b3e8 Mon Sep 17 00:00:00 2001 From: Abid Omar Date: Tue, 25 Jan 2022 20:38:54 +0100 Subject: [PATCH 2/7] refactor(example): update examples --- examples/async.rs | 4 ++-- examples/backend.rs | 4 ++-- examples/basic.rs | 4 ++-- examples/error.rs | 13 +++++-------- examples/multi-thread.rs | 4 ++-- examples/tags.rs | 4 ++-- examples/with-logger.rs | 4 ++-- 7 files changed, 17 insertions(+), 20 deletions(-) diff --git a/examples/async.rs b/examples/async.rs index 0d7fe751..d25179cd 100644 --- a/examples/async.rs +++ b/examples/async.rs @@ -29,7 +29,7 @@ async fn main() -> Result<()> { .build()?; // Start Agent - agent.start()?; + agent.start(); tokio::task::spawn(async { let n = fibonacci1(45); @@ -46,7 +46,7 @@ async fn main() -> Result<()> { .unwrap(); // Stop Agent - agent.stop()?; + agent.stop(); Ok(()) } diff --git a/examples/backend.rs b/examples/backend.rs index 50ae28cb..063e3839 100644 --- a/examples/backend.rs +++ b/examples/backend.rs @@ -23,9 +23,9 @@ fn main() -> Result<()> { .tags(&[("TagA", "ValueA"), ("TagB", "ValueB")]) .build()?; - agent.start()?; + agent.start(); let _result = fibonacci(45); - agent.stop()?; + agent.stop(); Ok(()) } diff --git a/examples/basic.rs b/examples/basic.rs index b11db348..1c9f0f43 100644 --- a/examples/basic.rs +++ b/examples/basic.rs @@ -28,7 +28,7 @@ fn main() -> Result<()> { println!("Start Time: {}", start); // Start Agent - agent.start()?; + agent.start(); let _result = fibonacci(47); @@ -40,7 +40,7 @@ fn main() -> Result<()> { println!("Stop Time: {}", stop); // Stop Agent - agent.stop()?; + agent.stop(); drop(agent); diff --git a/examples/error.rs b/examples/error.rs index 15a00015..f2fda381 100644 --- a/examples/error.rs +++ b/examples/error.rs @@ -16,25 +16,22 @@ fn fibonacci(n: u64) -> u64 { } fn main() -> Result<()> { -// Force rustc to display the log messages in the console. + // Force rustc to display the log messages in the console. std::env::set_var("RUST_LOG", "trace"); // Initialize the logger. pretty_env_logger::init_timed(); - // This example should fail and return an error. - println!("This example should fail and return an error."); - println!("Run this with: RUST_BACKTRACE=1 cargo run --example error"); - let mut agent = PyroscopeAgent::builder("http://invalid_url", "example.error") - .build()?; + .build() + .unwrap(); // Start Agent - agent.start()?; + agent.start(); let _result = fibonacci(47); // Stop Agent - agent.stop()?; + agent.stop(); drop(agent); diff --git a/examples/multi-thread.rs b/examples/multi-thread.rs index 9b2a4bec..7cd25a0c 100644 --- a/examples/multi-thread.rs +++ b/examples/multi-thread.rs @@ -30,7 +30,7 @@ fn main() -> Result<()> { .build()?; // Start Agent - agent.start()?; + agent.start(); let handle_1 = thread::spawn(|| { fibonacci1(45); @@ -45,7 +45,7 @@ fn main() -> Result<()> { handle_2.join().unwrap(); // Stop Agent - agent.stop()?; + agent.stop(); Ok(()) } diff --git a/examples/tags.rs b/examples/tags.rs index 24e80aad..ba9b720e 100644 --- a/examples/tags.rs +++ b/examples/tags.rs @@ -22,7 +22,7 @@ fn main() -> Result<()> { .build()?; // Start Agent - agent.start()?; + agent.start(); // Make some calculation let _result = fibonacci(47); @@ -34,7 +34,7 @@ fn main() -> Result<()> { let _result = fibonacci(47); // Stop Agent - agent.stop()?; + agent.stop(); Ok(()) } diff --git a/examples/with-logger.rs b/examples/with-logger.rs index a5406fc8..95f45b48 100644 --- a/examples/with-logger.rs +++ b/examples/with-logger.rs @@ -30,12 +30,12 @@ fn main() -> Result<()> { let mut agent = PyroscopeAgent::builder("http://localhost:4040", "example.logger").build()?; // Start Agent - agent.start()?; + agent.start(); let _result = fibonacci(47); // Stop Agent - agent.stop()?; + agent.stop(); Ok(()) } From 6c4ab1bab4395a1fd4a01a05a486458c41258e4a Mon Sep 17 00:00:00 2001 From: Abid Omar Date: Tue, 25 Jan 2022 21:26:46 +0100 Subject: [PATCH 3/7] docs(agent): move start/stop functions docs --- src/pyroscope.rs | 34 +++++++++++++++++----------------- src/utils.rs | 1 + 2 files changed, 18 insertions(+), 17 deletions(-) diff --git a/src/pyroscope.rs b/src/pyroscope.rs index b5ab0c91..af0aef40 100644 --- a/src/pyroscope.rs +++ b/src/pyroscope.rs @@ -220,6 +220,7 @@ pub struct PyroscopeAgent { pub config: PyroscopeConfig, } +/// Gracefully stop the profiler. impl Drop for PyroscopeAgent { /// Properly shutdown the agent. fn drop(&mut self) { @@ -269,12 +270,6 @@ impl PyroscopeAgent { PyroscopeAgentBuilder::new(url, application_name) } - /// Start profiling and sending data. The agent will keep running until stopped. The agent will send data to the server every 10s secondy. - /// # Example - /// ```ignore - /// let agent = PyroscopeAgent::builder("http://localhost:8080", "my-app").build().unwrap(); - /// agent.start().unwrap(); - /// ``` fn _start(&mut self) -> Result<()> { log::debug!("PyroscopeAgent - Starting"); @@ -294,7 +289,7 @@ impl PyroscopeAgent { let (tx, rx): (Sender, Receiver) = channel(); self.timer.attach_listener(tx.clone())?; - self.tx = Some(tx.clone()); + self.tx = Some(tx); let config = self.config.clone(); @@ -327,13 +322,18 @@ impl PyroscopeAgent { return Ok(()); } } - - return Ok(()); + Ok(()) })); Ok(()) } + /// Start profiling and sending data. The agent will keep running until stopped. The agent will send data to the server every 10s secondy. + /// # Example + /// ```ignore + /// let agent = PyroscopeAgent::builder("http://localhost:8080", "my-app").build().unwrap(); + /// agent.start().unwrap(); + /// ``` pub fn start(&mut self) { match self._start() { Ok(_) => log::trace!("PyroscopeAgent - Agent started"), @@ -341,14 +341,6 @@ impl PyroscopeAgent { } } - /// Stop the agent. The agent will stop profiling and send a last report to the server. - /// # Example - /// ```ignore - /// let agent = PyroscopeAgent::builder("http://localhost:8080", "my-app").build().unwrap(); - /// agent.start().unwrap(); - /// // Expensive operation - /// agent.stop().unwrap(); - /// ``` fn _stop(&mut self) -> Result<()> { log::debug!("PyroscopeAgent - Stopping"); // get tx and send termination signal @@ -367,6 +359,14 @@ impl PyroscopeAgent { Ok(()) } + /// Stop the agent. The agent will stop profiling and send a last report to the server. + /// # Example + /// ```ignore + /// let agent = PyroscopeAgent::builder("http://localhost:8080", "my-app").build().unwrap(); + /// agent.start().unwrap(); + /// // Expensive operation + /// agent.stop().unwrap(); + /// ``` pub fn stop(&mut self) { match self._stop() { Ok(_) => log::trace!("PyroscopeAgent - Agent stopped"), diff --git a/src/utils.rs b/src/utils.rs index 5ac38132..a3c4a20a 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -55,6 +55,7 @@ mod merge_tags_with_app_name_tests { ) } } + /// Error Wrapper for libc return. Only check for errors. pub fn check_err(num: T) -> Result { if num < T::default() { From 01c8793022e471b072ff223077147ec85fe16b4b Mon Sep 17 00:00:00 2001 From: Abid Omar Date: Wed, 26 Jan 2022 00:20:08 +0100 Subject: [PATCH 4/7] chore(code): re-format code --- src/backends/mod.rs | 2 +- src/backends/pprof.rs | 4 +--- src/pyroscope.rs | 4 +--- src/timer/mod.rs | 18 ++++++------------ tests/backends.rs | 2 +- 5 files changed, 10 insertions(+), 20 deletions(-) diff --git a/src/backends/mod.rs b/src/backends/mod.rs index fca51677..9c82c121 100644 --- a/src/backends/mod.rs +++ b/src/backends/mod.rs @@ -8,7 +8,7 @@ use crate::Result; use std::fmt::Debug; -/// Backend State +/// Backend State #[derive(Debug, Clone, Copy, PartialEq)] pub enum State { /// Backend is uninitialized. diff --git a/src/backends/pprof.rs b/src/backends/pprof.rs index c4097335..46e78faf 100644 --- a/src/backends/pprof.rs +++ b/src/backends/pprof.rs @@ -95,9 +95,7 @@ impl Backend for Pprof<'_> { // Copyright: https://github.com/YangKeao fn fold(report: &Report, with_thread_name: bool, mut writer: W) -> Result<()> -where - W: std::io::Write, -{ +where W: std::io::Write { for (key, value) in report.data.iter() { if with_thread_name { if !key.thread_name.is_empty() { diff --git a/src/pyroscope.rs b/src/pyroscope.rs index af0aef40..a4da8555 100644 --- a/src/pyroscope.rs +++ b/src/pyroscope.rs @@ -138,9 +138,7 @@ impl PyroscopeAgentBuilder { /// .unwrap(); /// ``` pub fn backend(self, backend: T) -> Self - where - T: Backend, - { + where T: Backend { Self { backend: Arc::new(Mutex::new(backend)), ..self diff --git a/src/timer/mod.rs b/src/timer/mod.rs index c2f11a1c..d895378e 100644 --- a/src/timer/mod.rs +++ b/src/timer/mod.rs @@ -5,18 +5,12 @@ // except according to those terms. // Possibly: ios, netbsd, openbsd, freebsd -#[cfg(target_os = "macos")] -pub mod kqueue; -#[cfg(target_os = "macos")] -pub use kqueue::Timer; +#[cfg(target_os = "macos")] pub mod kqueue; +#[cfg(target_os = "macos")] pub use kqueue::Timer; // Possibly: android -#[cfg(target_os = "linux")] -pub mod epoll; -#[cfg(target_os = "linux")] -pub use epoll::Timer; +#[cfg(target_os = "linux")] pub mod epoll; +#[cfg(target_os = "linux")] pub use epoll::Timer; -#[cfg(not(any(target_os = "linux", target_os = "macos")))] -pub mod sleep; -#[cfg(not(any(target_os = "linux", target_os = "macos")))] -pub use sleep::Timer; +#[cfg(not(any(target_os = "linux", target_os = "macos")))] pub mod sleep; +#[cfg(not(any(target_os = "linux", target_os = "macos")))] pub use sleep::Timer; diff --git a/tests/backends.rs b/tests/backends.rs index 5e5f33e8..899217e2 100644 --- a/tests/backends.rs +++ b/tests/backends.rs @@ -1,4 +1,4 @@ -use pyroscope::backends::{State}; +use pyroscope::backends::State; #[test] fn test_state_default() { From 9186e5a24a79b2c46eaf288c91874c6f540283e3 Mon Sep 17 00:00:00 2001 From: Abid Omar Date: Wed, 26 Jan 2022 00:32:27 +0100 Subject: [PATCH 5/7] chore(github): add release action --- .github/workflows/release.yml | 15 +++++++++++++++ 1 file changed, 15 insertions(+) create mode 100644 .github/workflows/release.yml diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml new file mode 100644 index 00000000..2b57a59e --- /dev/null +++ b/.github/workflows/release.yml @@ -0,0 +1,15 @@ +name: Pre-Release + +on: [push] + +jobs: + release: + name: Create draft release + runs-on: ubuntu-latest + if: "startsWith(github.ref, 'refs/tags/')" + steps: + - uses: "marvinpinto/action-automatic-releases@v1.1.1" + with: + repo_token: "${{ secrets.GITHUB_TOKEN }}" + draft: true + prerelease: false From eea0f9b1021db9711a3965e5f0c47184a44b83e3 Mon Sep 17 00:00:00 2001 From: Abid Omar Date: Wed, 26 Jan 2022 00:50:29 +0100 Subject: [PATCH 6/7] chore(github): add publish action --- .github/workflows/publish.yml | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) create mode 100644 .github/workflows/publish.yml diff --git a/.github/workflows/publish.yml b/.github/workflows/publish.yml new file mode 100644 index 00000000..2705ad4c --- /dev/null +++ b/.github/workflows/publish.yml @@ -0,0 +1,22 @@ +name: Publish to crates.io + +on: + workflow_dispatch: + release: + types: [published] + +jobs: + publish-pyroscope: + name: Publish pyroscope crate + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v1 + - uses: actions-rs/toolchain@v1 + with: + toolchain: stable + override: true + - name: publish pyroscope crate + continue-on-error: true + run: | + cargo login ${{ secrets.CARGO_TOKEN }} + cargo publish From 065c5963ef930ebc1c992d7ae82ba84f85bee32a Mon Sep 17 00:00:00 2001 From: Abid Omar Date: Wed, 26 Jan 2022 01:32:59 +0100 Subject: [PATCH 7/7] refactor(clippy): fix clippy warnings --- src/pyroscope.rs | 4 +--- src/timer/epoll.rs | 27 +++++++++++++++++---------- tests/timer-epoll.rs | 4 +++- 3 files changed, 21 insertions(+), 14 deletions(-) diff --git a/src/pyroscope.rs b/src/pyroscope.rs index a4da8555..e2d64da1 100644 --- a/src/pyroscope.rs +++ b/src/pyroscope.rs @@ -278,11 +278,9 @@ impl PyroscopeAgent { // set running to true let pair = Arc::clone(&self.running); - let (lock, cvar) = &*pair; + let (lock, _cvar) = &*pair; let mut running = lock.lock()?; *running = true; - drop(lock); - drop(cvar); drop(running); let (tx, rx): (Sender, Receiver) = channel(); diff --git a/src/timer/epoll.rs b/src/timer/epoll.rs index 666c9e8c..931af39c 100644 --- a/src/timer/epoll.rs +++ b/src/timer/epoll.rs @@ -64,10 +64,7 @@ impl Timer { // Iterate through Senders txs.lock()?.iter().for_each(|tx| { // Send event to attached Sender - match tx.send(current) { - Ok(_) => {} - Err(_) => {} - } + if tx.send(current).is_ok() {} }); } })); @@ -151,12 +148,16 @@ impl Timer { let mut events = Vec::with_capacity(1); // wait for the timer to fire an event. This is function will block. - epoll_wait(epoll_fd, events.as_mut_ptr(), 1, -1)?; + unsafe { + epoll_wait(epoll_fd, events.as_mut_ptr(), 1, -1)?; + } // read the value from the timerfd. This is required to re-arm the timer. let mut buffer: u64 = 0; let bufptr: *mut _ = &mut buffer; - read(timer_fd, bufptr as *mut libc::c_void, 8)?; + unsafe { + read(timer_fd, bufptr as *mut libc::c_void, 8)?; + } Ok(()) } @@ -217,15 +218,21 @@ pub fn epoll_ctl( } /// libc::epoll_wait wrapper -pub fn epoll_wait( +/// +/// # Safety +/// This function is a wrapper for libc::epoll_wait. +pub unsafe fn epoll_wait( epoll_fd: i32, events: *mut libc::epoll_event, maxevents: libc::c_int, timeout: libc::c_int, ) -> Result<()> { - check_err(unsafe { libc::epoll_wait(epoll_fd, events, maxevents, timeout) })?; + check_err(libc::epoll_wait(epoll_fd, events, maxevents, timeout))?; Ok(()) } /// libc::read wrapper -pub fn read(timer_fd: i32, bufptr: *mut libc::c_void, count: libc::size_t) -> Result<()> { - check_err(unsafe { libc::read(timer_fd, bufptr, count) })?; +/// +/// # Safety +/// This function is a wrapper for libc::read. +pub unsafe fn read(timer_fd: i32, bufptr: *mut libc::c_void, count: libc::size_t) -> Result<()> { + check_err(libc::read(timer_fd, bufptr, count))?; Ok(()) } diff --git a/tests/timer-epoll.rs b/tests/timer-epoll.rs index fa65d0a3..113cb058 100644 --- a/tests/timer-epoll.rs +++ b/tests/timer-epoll.rs @@ -78,7 +78,9 @@ mod tests { let mut events = vec![libc::epoll_event { events: 0, u64: 0 }]; // Expire in 1ms - let void = epoll_wait(epoll_fd, events.as_mut_ptr(), 1, 1).unwrap(); + let void = unsafe { + epoll_wait(epoll_fd, events.as_mut_ptr(), 1, 1).unwrap(); + }; assert!(void == ()); }