Repository: shuttle-hq/synth (language: rust)

Issue:
Custom panic handler
**Required Functionality**
Whenever synth panics, the user should get the option to send an automated bug message with another option to give their contact info so we can get back to them. This will help our users be more active when bugs occur and help us get more and better bug reports.

**Proposed Solution**
A custom panic handler can get some information (synth version, operating system, etc.) to format and send to an endpoint we need to set up.

**Use case**
Improve our reaction to bugs, make synth better faster


Discussion / hints:
Hey, can I work on this?
Sure @juniorbassani ! Do you have ideas for the design?
> Sure @juniorbassani ! Do you have ideas for the design?

I was thinking of setting up a [panic hook](https://doc.rust-lang.org/std/panic/fn.set_hook.html#) asking if the user agrees on sending the panic info and if they want to send some contact info. Maybe sending the panic cause could be enabled by default on the installation script. As for the contact information, we would need to be a bit careful as it is personal data.

After that, we can send a post request to some endpoint (I see we're already using the [posthog](https://github.com/getsynth/synth/blob/a5025043d758d5332a1f917d99d10532ff002a79/synth/src/cli/telemetry.rs#L130) service).
I think collecting the stack trace of the panic plus some system information and perhaps a sanitized form of the arguments (just so we know if the user e.g. exported to a database) would be quite helpful.

Of course the user should have final say in what data gets sent.
> Maybe sending the panic cause could be enabled by default on the installation script. 

@juniorbassani  Yeah I think if telemetry is enabled, this should get sent off by default. We just need to do a sanity check and make sure that no sensitive information is being leaked (things like field names, collection names, db uris etc.)
I'm a bit uncertain on which option to choose for collecting the stack trace:

1. Use the [standard backtrace api](https://doc.rust-lang.org/std/backtrace/index.html) (currently unstable).
2. Use the [backtrace crate](https://docs.rs/backtrace/0.3.61/backtrace/).

Since we're on nightly, we could just use the standard backtrace api. The downside is that it's probably [not going to be stabilized soon](https://github.com/rust-lang/rust/pull/72981#issuecomment-880144242).
If we want something more stable, we could go with option 2, although that would require an additional dependency.
I don't think either would be a problem. Personally, I'd likely choose the backtrace crate, because I've seen unstable features break our build in the past.

Fix patch:
diff --git a/Cargo.lock b/Cargo.lock
index 24b23066..0b83e4f8 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -2,6 +2,21 @@
 # It is not intended for manual editing.
 version = 3
 
+[[package]]
+name = "addr2line"
+version = "0.16.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "3e61f2b7f93d2c7d2b08263acaa4a363b3e276806c68af6134c44f523bf1aacd"
+dependencies = [
+ "gimli",
+]
+
+[[package]]
+name = "adler"
+version = "1.0.2"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "f26201604c87b1e01bd3d98f8d5d9a8fcbb815e8cedb41ffccbeb4bf593a35fe"
+
 [[package]]
 name = "aead"
 version = "0.3.2"
@@ -381,6 +396,21 @@ version = "1.0.1"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "cdb031dd78e28731d87d56cc8ffef4a8f36ca26c38fe2de700543e627f8a464a"
 
+[[package]]
+name = "backtrace"
+version = "0.3.61"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "e7a905d892734eea339e896738c14b9afce22b5318f64b951e70bf3844419b01"
+dependencies = [
+ "addr2line",
+ "cc",
+ "cfg-if 1.0.0",
+ "libc",
+ "miniz_oxide",
+ "object",
+ "rustc-demangle",
+]
+
 [[package]]
 name = "base-x"
 version = "0.2.8"
@@ -1232,6 +1262,12 @@ dependencies = [
  "polyval",
 ]
 
+[[package]]
+name = "gimli"
+version = "0.25.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "f0a01e0497841a3b2db4f8afa483cce65f7e96a3498bd6c541734792aeac8fe7"
+
 [[package]]
 name = "git2"
 version = "0.13.21"
@@ -1764,6 +1800,16 @@ version = "0.1.3"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "0c835948974f68e0bd58636fc6c5b1fbff7b297e3046f11b3b3c18bbac012c6d"
 
+[[package]]
+name = "miniz_oxide"
+version = "0.4.4"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "a92518e98c078586bc6c934028adcca4c92a53d6a958196de835170a01d84e4b"
+dependencies = [
+ "adler",
+ "autocfg 1.0.1",
+]
+
 [[package]]
 name = "mio"
 version = "0.7.13"
@@ -2009,6 +2055,15 @@ version = "0.3.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "17b02fc0ff9a9e4b35b3342880f48e896ebf69f2967921fe8646bf5b7125956a"
 
+[[package]]
+name = "object"
+version = "0.26.2"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "39f37e50073ccad23b6d09bcb5b263f4e76d3bb6038e4a3c08e52162ffa8abc2"
+dependencies = [
+ "memchr",
+]
+
 [[package]]
 name = "once_cell"
 version = "1.8.0"
@@ -2546,6 +2601,12 @@ dependencies = [
  "serde",
 ]
 
+[[package]]
+name = "rustc-demangle"
+version = "0.1.21"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "7ef03e0a2b150c7a90d01faf6254c9c48a41e95fb2a8c2ac1c6f0d2b9aefc342"
+
 [[package]]
 name = "rustc_version"
 version = "0.2.3"
@@ -3113,9 +3174,11 @@ dependencies = [
  "anyhow",
  "async-std",
  "async-trait",
+ "backtrace",
  "beau_collector",
  "chrono",
  "colored",
+ "console",
  "ctrlc",
  "dirs",
  "env_logger",
diff --git a/synth/Cargo.toml b/synth/Cargo.toml
index ba2386fc..4f3e1f7d 100644
--- a/synth/Cargo.toml
+++ b/synth/Cargo.toml
@@ -15,7 +15,7 @@ harness = false
 
 [features]
 default = []
-telemetry = ["posthog-rs", "uuid"]
+telemetry = ["posthog-rs", "uuid", "backtrace", "console"]
 
 [build-dependencies]
 git2 = "0.13.20"
@@ -33,6 +33,8 @@ serde_json = "1.0"
 # Telemetry related dependencies
 uuid = { version = "0.8.2", features = ["v4"], optional = true }
 posthog-rs = { version = "0.1.3", optional = true }
+backtrace = { version = "0.3", optional = true }
+console = { version = "0.14.1", default-features = false, optional = true }
 
 colored = "2.0.0"
 anyhow = "1.0.32"
@@ -72,4 +74,4 @@ beau_collector = "0.2.1"
 
 reqwest = { version = "0.11", features = ["json", "blocking"] }
 
-semver = "1.0.4"
\ No newline at end of file
+semver = "1.0.4"
diff --git a/synth/src/cli/mod.rs b/synth/src/cli/mod.rs
index 2047b083..2b246310 100644
--- a/synth/src/cli/mod.rs
+++ b/synth/src/cli/mod.rs
@@ -13,6 +13,7 @@ use crate::cli::store::Store;
 use crate::cli::db_utils::DataSourceParams;
 use crate::version::print_version_message;
 
+use serde::Serialize;
 use anyhow::{Context, Result};
 use std::path::PathBuf;
 use structopt::StructOpt;
@@ -180,8 +181,10 @@ impl Cli {
     }
 }
 
-
-#[derive(StructOpt)]
+// The serialization of this enum is used for telemetry when synth panics and we want our logs to
+// contain the command that caused the panic. When modifying this, pay attention to skip
+// serialization of any privacy sensitive information.
+#[derive(StructOpt, Serialize)]
 #[structopt(
     name = "synth",
     about = "synthetic data engine on the command line",
@@ -190,7 +193,8 @@ impl Cli {
 pub enum Args {
     #[structopt(about = "(DEPRECATED). For backward compatibility and is a no-op.")]
     Init {
-	init_path: Option<PathBuf>
+        #[serde(skip)]
+        init_path: Option<PathBuf>
     },
     #[structopt(about = "Generate data from a namespace", alias = "gen")]
     Generate {
@@ -198,8 +202,10 @@ pub enum Args {
             help = "The namespace directory from which to read schema files",
             parse(from_os_str)
         )]
+        #[serde(skip)]
         namespace: PathBuf,
         #[structopt(long, help = "The specific collection from which to generate")]
+        #[serde(skip)]
         collection: Option<Name>,
         #[structopt(long, help = "the number of samples", default_value = "1")]
         size: usize,
@@ -207,6 +213,7 @@ pub enum Args {
             long,
             help = "The sink into which to generate data. Can be a postgres uri, a mongodb uri. If not specified, data will be written to stdout"
         )]
+        #[serde(skip)]
         to: Option<String>,
         #[structopt(
             long,
@@ -222,6 +229,7 @@ pub enum Args {
         long,
         help = "(Postgres only) Specify the schema into which to generate. Defaults to 'public'."
         )]
+        #[serde(skip)]
         schema: Option<String>,
     },
     #[structopt(about = "Import data from an external source")]
@@ -230,21 +238,25 @@ pub enum Args {
             help = "The namespace directory into which to save imported schema files",
             parse(from_os_str)
         )]
+        #[serde(skip)]
         namespace: PathBuf,
         #[structopt(
             long,
             help = "The name of a collection into which the data will be imported"
         )]
+        #[serde(skip)]
         collection: Option<Name>,
         #[structopt(
             long,
             help = "The source from which to import data. Can be a postgres uri, a mongodb uri, a mysql/mariadb uri or a path to a JSON file / directory. If not specified, data will be read from stdin"
         )]
+        #[serde(skip)]
         from: Option<String>,
         #[structopt(
         long,
         help = "(Postgres only) Specify the schema from which to import. Defaults to 'public'."
         )]
+        #[serde(skip)]
         schema: Option<String>,
     },
     #[cfg(feature = "telemetry")]
@@ -255,7 +267,7 @@ pub enum Args {
 }
 
 #[cfg(feature = "telemetry")]
-#[derive(StructOpt)]
+#[derive(StructOpt, Serialize)]
 pub enum TelemetryCommand {
     #[structopt(about = "Enable anonymous usage data collection")]
     Enable,
diff --git a/synth/src/cli/telemetry.rs b/synth/src/cli/telemetry.rs
index ea6645d0..58c4e020 100644
--- a/synth/src/cli/telemetry.rs
+++ b/synth/src/cli/telemetry.rs
@@ -1,4 +1,10 @@
-use anyhow::Result;
+use anyhow::{Context, Result};
+use backtrace::Backtrace;
+use colored::Colorize;
+use lazy_static::lazy_static;
+
+use std::io::{self, BufRead, Read, Write};
+use std::panic;
 use std::collections::HashMap;
 use std::future::Future;
 use std::error::Error;
@@ -13,6 +19,63 @@ use super::{Args, TelemetryCommand};
 const API_KEY: &str = "L-AQtrFVtZGL_PjK2FbFLBR3oXNtfv8OrCD8ObyeBQo";
 const EVENT_NAME: &str = "synth-command";
 
+lazy_static! {
+    static ref TELEMETRY_CLIENT: TelemetryClient = TelemetryClient::new();
+}
+
+fn send_panic_report(synth_command: &str, telemetry_client: &TelemetryClient) {
+    // This exists only to return early instead of panicking inside this panic hook
+    let send_panic_report_impl = || -> Result<()> {
+        let (stdin, stderr) = (io::stdin(), io::stderr());
+        let (mut stdin, mut stderr) = (stdin.lock(), stderr.lock());
+        let mut username = None;
+        let mut email = None;
+
+        eprintln!("\n{}", "Synth encountered a fatal error. Because telemetry is enabled, an anonymous \
+            bug report will be sent to the Synth developers so that we may investigate the problem.".red());
+
+        // Only interact with the user if there is an actual user running Synth; if the program's
+        // stderr was redirected or this is running as part of CI, skip the interaction
+        if console::user_attended_stderr() {
+            eprint!("\nWould you also like to send your name and e-mail? One of our developers may enter \
+                in contact to obtain further information if deemed necessary.\nAnswer (y/n): ");
+            stderr.flush()?;
+
+            // Newline character is also stored
+            let mut answer = [0; 2];
+            stdin.read_exact(&mut answer).context("Couldn't read answer.")?;
+
+            if answer[0] == b'y' {
+                eprint!("Name: ");
+                stderr.flush()?;
+
+                let mut n = String::new();
+                stdin.read_line(&mut n).context("Couldn't read name.")?;
+                username = Some(n.trim().to_owned());
+
+                eprint!("E-mail: ");
+                stderr.flush()?;
+
+                let mut e = String::new();
+                stdin.read_line(&mut e).context("Couldn't read e-mail.")?;
+                email = Some(e.trim().to_owned());
+            }
+        }
+        
+        let backtrace = Backtrace::new();
+        let panic_report = PanicReport::new(username, email, synth_command.to_owned(), backtrace);
+
+        telemetry_client.send_panic_report(panic_report)?;
+
+        Ok(())
+    };
+
+    match send_panic_report_impl() {
+        Ok(()) => eprintln!("Bug report sent with success. Thanks for your patience!"),
+        Err(e) => eprintln!("Error sending bug report: {}", e),
+    }
+}
+
 pub(crate) fn enable() -> Result<()> {
     // Initialise the `uuid` if it hasn't been initialised yet.
     let _ = get_or_initialise_uuid();
@@ -40,7 +103,18 @@ where
     Fut: Future<Output = Result<T, E>>,
     E: AsRef<dyn Error + 'static>
 {
-    let client = TelemetryClient::new();
+    if !is_enabled() {
+        return func(args).await;
+    }
+
+    let default_panic_hook = panic::take_hook();
+    let synth_command = serde_json::to_string(&args).unwrap();
+
+    // Register a custom panic hook that will run the default hook and send a bug report to PostHog
+    panic::set_hook(Box::new(move |panic_info| {
+        default_panic_hook(panic_info);
+        send_panic_report(&synth_command, &TELEMETRY_CLIENT);
+    }));
 
     let command_name = match &args {
         Args::Init { .. } => "init",
@@ -54,8 +128,26 @@ where
 
     func(args)
         .await
-        .and_then(|success| client.success(command_name, success))
-        .or_else(|err| client.failed(command_name, err))
+        .and_then(|success| TELEMETRY_CLIENT.success(command_name, success))
+        .or_else(|err| TELEMETRY_CLIENT.failed(command_name, err))
+}
+
+struct PanicReport {
+    username: Option<String>,
+    email: Option<String>,
+    synth_command: String,
+    backtrace: Backtrace
+}
+
+impl PanicReport {
+    fn new(
+        username: Option<String>,
+        email: Option<String>,
+        synth_command: String,
+        backtrace: Backtrace
+    ) -> Self {
+        Self { username, email, synth_command, backtrace }
+    }
 }
 
 enum CommandResult {
@@ -77,7 +169,6 @@ pub(crate) struct TelemetryClient {
     uuid: String,
     synth_version: String,
     os: String,
-    enabled: bool,
 }
 
 impl TelemetryClient {
@@ -90,12 +181,23 @@ impl TelemetryClient {
             uuid: get_or_initialise_uuid(),
             synth_version,
             os,
-            enabled: is_enabled(),
         }
     }
 
+    fn default_telemetry_properties(&self) -> HashMap<String, String> {
+        let mut prop_map = HashMap::new();
+        prop_map.insert("version".to_string(), self.synth_version.clone());
+        prop_map.insert("os".to_string(), self.os.clone());
+
+        prop_map
+    }
+
     pub fn success<T, E>(&self, command_name: &str, output: T) -> Result<T, E> {
-        self.send(command_name, CommandResult::Success)
+        let mut prop_map = self.default_telemetry_properties();
+        prop_map.insert("command".to_string(), command_name.to_string());
+        prop_map.insert("success".to_string(), CommandResult::Success.to_string());
+
+        self.send(EVENT_NAME.to_string(), prop_map)
             .or_else(|err| {
                 info!("failed to push ok of command: {}", err);
                 Ok(())
@@ -107,7 +209,11 @@ impl TelemetryClient {
     where
         E: AsRef<dyn Error + 'static>
     {
-        self.send(command_name, CommandResult::Failed)
+        let mut prop_map = self.default_telemetry_properties();
+        prop_map.insert("command".to_string(), command_name.to_string());
+        prop_map.insert("success".to_string(), CommandResult::Failed.to_string());
+
+        self.send(EVENT_NAME.to_string(), prop_map)
             .or_else(|err| {
                 info!("failed to push err of command: {}", err);
                 Ok(())
@@ -115,31 +221,33 @@ impl TelemetryClient {
         Err(error)
     }
 
-    fn send(&self, command_name: &str, res: CommandResult) -> Result<()> {
-        if !self.enabled {
-            return Ok(());
-        }
+    fn send_panic_report(&self, mut panic_report: PanicReport) -> Result<()> {
+        panic_report.backtrace.resolve();
 
-        let mut prop_map = HashMap::new();
+        let mut prop_map = self.default_telemetry_properties();
+        prop_map.insert("username".to_string(), panic_report.username.unwrap_or_default());
+        prop_map.insert("email".to_string(), panic_report.email.unwrap_or_default());
+        prop_map.insert("synth_command".to_string(), panic_report.synth_command);
+        prop_map.insert("backtrace".to_string(), format!("{:?}", panic_report.backtrace));
 
-        prop_map.insert("command".to_string(), command_name.to_string());
-        prop_map.insert("success".to_string(), res.to_string());
-        prop_map.insert("version".to_string(), self.synth_version.clone());
-        prop_map.insert("os".to_string(), self.os.clone());
+        self.send(String::from("synth-panic-report"), prop_map)
+    }
 
+    fn send(&self, event: String, prop_map: HashMap<String, String>) -> Result<()> {
         let props = posthog_rs::Properties {
             distinct_id: self.uuid.clone(),
             props: prop_map,
         };
 
         let event = posthog_rs::Event {
-            event: EVENT_NAME.to_string(),
+            event,
             properties: props,
             timestamp: None,
         };
 
         if let Err(err) = self.ph_client.capture(event) {
             debug!("Failed to send message to PostHog. Error: {:?}", err);
+            return Err(anyhow!("Failed to send message to PostHog."));
         }
 
         Ok(())
diff --git a/synth/src/main.rs b/synth/src/main.rs
index 4416d539..da9a8c39 100644
--- a/synth/src/main.rs
+++ b/synth/src/main.rs
@@ -5,7 +5,6 @@ use synth::cli::Cli;
 use std::thread;
 use std::thread::JoinHandle;
 
-
 #[async_std::main]
 async fn main() -> Result<()> {
     let args = Args::from_args();


Test patch: