feat: add support for reporting patch install events (#71)

* feat: add client_id to UpdaterState

* feat: add support for reporting patch install events

* Ensure we save UpdaterState after it's created

* Save state if we assign it a client id on load

* Don't error in report_launch_success if no patch exists

* coverage

* Rename

* Minor refactor

* coverage

* coverage

* Update library/src/updater.rs

Co-authored-by: Felix Angelov <felix@shorebird.dev>

* restore debug logs

* make test serial

---------

Co-authored-by: Felix Angelov <felix@shorebird.dev>

library/src/c_api.rs

@@ -245,6 +245,7 @@ mod test {
     use crate::{
         network::PatchCheckResponse, testing_set_network_hooks, updater::testing_reset_config,
     };
+    use anyhow::Ok;
     use serial_test::serial;
     use tempdir::TempDir;
 
@@ -442,6 +443,7 @@ mod test {
                 ];
                 Ok(patch_bytes)
             },
+            |_url, _event| Ok(()),
         );
         // There is an update available.
         assert!(shorebird_check_for_update());
@@ -536,6 +538,7 @@ mod test {
                 // Never called.
                 Ok(Vec::new())
             },
+            |_url, _event| Ok(()),
         );
         {
             // Lock the mutex before starting the thread.

library/src/cache.rs

@@ -54,7 +54,7 @@ pub struct UpdaterState {
     /// List of slots.
     slots: Vec<Slot>,
     /// The client ID for this device.
-    client_id: Option<String>,
+    pub client_id: Option<String>,
     // Add file path or FD so modifying functions can save it to disk?
 }
 
@@ -108,8 +108,6 @@ impl UpdaterState {
         Ok(())
     }
 
-    // TODO(eseidel): Should return Result instead of logging, the c_api
-    // layer can log if desired.
     pub fn mark_patch_as_good(&mut self, patch_number: usize) -> Result<()> {
         if self.is_known_bad_patch(patch_number) {
             bail!("Tried to report successful launch for a known bad patch.  Ignoring.");

library/src/network.rs

@@ -18,8 +18,13 @@ fn patches_check_url(base_url: &str) -> String {
     return format!("{}/api/v1/patches/check", base_url);
 }
 
+fn patches_events_url(base_url: &str) -> String {
+    return format!("{}/api/v1/patches/events", base_url);
+}
+
 pub type PatchCheckRequestFn = fn(&str, PatchCheckRequest) -> anyhow::Result<PatchCheckResponse>;
 pub type DownloadFileFn = fn(&str) -> anyhow::Result<Vec<u8>>;
+pub type PatchInstallSuccessFn = fn(&str, CreatePatchInstallEventRequest) -> anyhow::Result<()>;
 
 /// A container for network clalbacks which can be mocked out for testing.
 #[derive(Clone)]
@@ -28,6 +33,8 @@ pub struct NetworkHooks {
     pub patch_check_request_fn: PatchCheckRequestFn,
     /// The function to call to download a file.
     pub download_file_fn: DownloadFileFn,
+    /// The function to call to report patch install success.
+    pub patch_install_success_fn: PatchInstallSuccessFn,
 }
 
 // We have to implement Debug by hand since fn types don't implement it.
@@ -36,6 +43,7 @@ impl core::fmt::Debug for NetworkHooks {
         f.debug_struct("NetworkHooks")
             .field("patch_check_request_fn", &"<fn>")
             .field("download_file_fn", &"<fn>")
+            .field("patch_install_success_fn", &"<fn>")
             .finish()
     }
 }
@@ -53,12 +61,21 @@ fn download_file_throws(_url: &str) -> anyhow::Result<Vec<u8>> {
     anyhow::bail!("please set a download_file_fn");
 }
 
+#[cfg(test)]
+pub fn patch_install_success_throws(
+    _url: &str,
+    _request: CreatePatchInstallEventRequest,
+) -> anyhow::Result<()> {
+    anyhow::bail!("please set a patch_install_success_fn");
+}
+
 impl Default for NetworkHooks {
     #[cfg(not(test))]
     fn default() -> Self {
         Self {
             patch_check_request_fn: patch_check_request_default,
             download_file_fn: download_file_default,
+            patch_install_success_fn: patch_install_success_default,
         }
     }
 
@@ -67,6 +84,7 @@ impl Default for NetworkHooks {
         Self {
             patch_check_request_fn: patch_check_request_throws,
             download_file_fn: download_file_throws,
+            patch_install_success_fn: patch_install_success_throws,
         }
     }
 }
@@ -90,17 +108,29 @@ pub fn download_file_default(url: &str) -> anyhow::Result<Vec<u8>> {
     Ok(bytes.to_vec())
 }
 
+#[cfg(not(test))]
+pub fn patch_install_success_default(
+    url: &str,
+    request: CreatePatchInstallEventRequest,
+) -> anyhow::Result<()> {
+    let client = reqwest::blocking::Client::new();
+    let _ = client.post(url).json(&request).send()?;
+    Ok(())
+}
+
 #[cfg(test)]
 /// Unit tests can call this to mock out the network calls.
 pub fn testing_set_network_hooks(
     patch_check_request_fn: PatchCheckRequestFn,
     download_file_fn: DownloadFileFn,
+    patch_install_success_fn: PatchInstallSuccessFn,
 ) {
     crate::config::with_config_mut(|maybe_config| match maybe_config {
         Some(config) => {
             config.network_hooks = NetworkHooks {
                 patch_check_request_fn,
                 download_file_fn,
+                patch_install_success_fn,
             };
         }
         None => {
@@ -140,6 +170,62 @@ pub struct PatchCheckRequest {
     pub arch: String,
 }
 
+/// An event that is sent to the server when a patch is successfully installed.
+#[derive(Debug, Serialize)]
+pub struct PatchInstallEvent {
+    /// The Shorebird app_id built into the shorebird.yaml in the app.
+    pub app_id: String,
+
+    /// The architecture we're running (e.g. "aarch64", "x86", "x86_64").
+    pub arch: String,
+
+    /// The unique ID of this device.
+    pub client_id: String,
+
+    /// The identifier of this event.
+    #[serde(rename = "type")]
+    pub identifier: String,
+
+    /// The patch number that was installed.
+    pub patch_number: usize,
+
+    /// The platform we're running on (e.g. "android", "ios", "windows", "macos", "linux").
+    pub platform: String,
+
+    /// The release version from AndroidManifest.xml, Info.plist in the app.
+    pub release_version: String,
+}
+
+impl PatchInstallEvent {
+    pub fn new(
+        app_id: String,
+        arch: String,
+        client_id: String,
+        patch_number: usize,
+        platform: String,
+        release_version: String,
+    ) -> Self {
+        Self {
+            app_id,
+            arch,
+            client_id,
+            identifier: "__patch_install__".to_string(),
+            patch_number,
+            platform,
+            release_version,
+        }
+    }
+}
+
+/// The request body for the create patch install event endpoint.
+///
+/// We may want to consider making this more generic if/when we add more events
+/// using something like https://github.com/dtolnay/typetag.
+#[derive(Debug, Serialize)]
+pub struct CreatePatchInstallEventRequest {
+    event: PatchInstallEvent,
+}
+
 #[derive(Debug, Deserialize)]
 pub struct PatchCheckResponse {
     pub patch_available: bool,
@@ -171,6 +257,32 @@ pub fn send_patch_check_request(
     return Ok(response);
 }
 
+pub fn report_successful_patch_install(
+    config: &UpdateConfig,
+    state: &UpdaterState,
+    patch_number: usize,
+) -> anyhow::Result<()> {
+    let client_id = state
+        .client_id
+        .clone()
+        .unwrap_or("".to_string())
+        .to_string();
+
+    let event = PatchInstallEvent::new(
+        config.app_id.clone(),
+        current_arch().to_string(),
+        client_id.to_string(),
+        patch_number,
+        current_platform().to_string(),
+        config.release_version.clone(),
+    );
+    let request = CreatePatchInstallEventRequest { event };
+
+    let patch_install_success_fn = config.network_hooks.patch_install_success_fn;
+    let url = &patches_events_url(&config.base_url);
+    patch_install_success_fn(url, request)
+}
+
 pub fn download_to_path(
     network_hooks: &NetworkHooks,
     url: &str,
@@ -219,6 +331,25 @@ mod tests {
         assert_eq!(patch.hash, "#");
     }
 
+    #[test]
+    fn create_patch_install_event_request_serializes() {
+        let request = super::CreatePatchInstallEventRequest {
+            event: super::PatchInstallEvent::new(
+                "app_id".to_string(),
+                "arch".to_string(),
+                "client_id".to_string(),
+                1,
+                "platform".to_string(),
+                "release_version".to_string(),
+            ),
+        };
+        let json_string = serde_json::to_string(&request).unwrap();
+        assert_eq!(
+            json_string,
+            r#"{"event":{"app_id":"app_id","arch":"arch","client_id":"client_id","type":"__patch_install__","patch_number":1,"platform":"platform","release_version":"release_version"}}"#
+        )
+    }
+
     // This confirms that the default network hooks throw an error in cfg(test).
     // In cfg(not(test)) they should be set to the default implementation
     // which makes real network calls.
@@ -247,5 +378,6 @@ mod tests {
         let debug = format!("{:?}", network_hooks);
         assert!(debug.contains("patch_check_request_fn"));
         assert!(debug.contains("download_file_fn"));
+        assert!(debug.contains("patch_install_success_fn"));
     }
 }

library/src/updater.rs

@@ -13,7 +13,8 @@ use crate::cache::{PatchInfo, UpdaterState};
 use crate::config::{set_config, with_config, UpdateConfig};
 use crate::logging::init_logging;
 use crate::network::{
-    download_to_path, send_patch_check_request, NetworkHooks, PatchCheckResponse,
+    download_to_path, report_successful_patch_install, send_patch_check_request, NetworkHooks,
+    PatchCheckResponse,
 };
 use crate::updater_lock::{with_updater_thread_lock, UpdaterLockState};
 use crate::yaml::YamlConfig;
@@ -378,18 +379,25 @@ pub fn report_launch_success() -> anyhow::Result<()> {
         let mut state =
             UpdaterState::load_or_new_on_error(&config.cache_dir, &config.release_version);
 
-        let patch =
+        if let Some(patch) = state.current_boot_patch() {
+            if !state.is_known_good_patch(patch.number) {
+                // Ignore the error here, we'll try to activate the next best patch
+                // even if we fail to mark this one as good.
+                if state.mark_patch_as_good(patch.number).is_ok() {
+                    let report_result =
+                        report_successful_patch_install(&config, &state, patch.number);
+                    if let Err(err) = report_result {
+                        error!("Failed to report successful patch install: {:?}", err);
+                    }
+                }
+            }
+
             state
-                .current_boot_patch()
-                .ok_or(anyhow::Error::from(UpdateError::InvalidState(
-                    "No current patch".to_string(),
-                )))?;
-        // Ignore the error here, we'll try to activate the next best patch
-        // even if we fail to mark this one as good.
-        let _ = state.mark_patch_as_good(patch.number);
-        state
-            .save()
-            .map_err(|_| anyhow::Error::from(UpdateError::FailedToSaveState))
+                .save()
+                .map_err(|_| anyhow::Error::from(UpdateError::FailedToSaveState))
+        } else {
+            Ok(())
+        }
     })
 }
 
@@ -543,12 +551,57 @@ mod tests {
                 .unwrap(),
             crate::UpdateError::InvalidState("No current patch".to_string())
         );
-        assert_eq!(
-            crate::report_launch_success()
-                .unwrap_err()
-                .downcast::<crate::UpdateError>()
-                .unwrap(),
-            crate::UpdateError::InvalidState("No current patch".to_string())
-        );
+        assert!(crate::report_launch_success().is_ok());
+    }
+
+    #[serial]
+    #[test]
+    fn report_launch_success_with_patch() {
+        use crate::cache::{PatchInfo, UpdaterState};
+        use crate::config::with_config;
+        let tmp_dir = TempDir::new("example").unwrap();
+        init_for_testing(&tmp_dir);
+
+        // Install a fake patch.
+        with_config(|config| {
+            let download_dir = std::path::PathBuf::from(&config.download_dir);
+            let artifact_path = download_dir.join("1");
+            fs::create_dir_all(&download_dir).unwrap();
+            fs::write(&artifact_path, "hello").unwrap();
+
+            let mut state =
+                UpdaterState::load_or_new_on_error(&config.cache_dir, &config.release_version);
+            state
+                .install_patch(PatchInfo {
+                    path: artifact_path,
+                    number: 1,
+                })
+                .expect("move failed");
+            state.save().expect("save failed");
+            Ok(())
+        })
+        .unwrap();
+
+        // Pretend we booted from it.
+        crate::report_launch_start().unwrap();
+
+        let next_boot_patch = crate::next_boot_patch().unwrap().unwrap();
+        with_config(|config| {
+            let state =
+                UpdaterState::load_or_new_on_error(&config.cache_dir, &config.release_version);
+            assert!(!state.is_known_good_patch(next_boot_patch.number));
+            Ok(())
+        })
+        .unwrap();
+
+        super::report_launch_success().unwrap();
+
+        with_config(|config| {
+            let state =
+                UpdaterState::load_or_new_on_error(&config.cache_dir, &config.release_version);
+            assert!(state.is_known_good_patch(next_boot_patch.number));
+            Ok(())
+        })
+        .unwrap();
     }
 }