Dont send pings if empty (#58)

Dont send pings if empty

glean-core/android/src/test/java/mozilla/telemetry/glean/GleanTest.kt

@@ -8,24 +8,26 @@ import android.content.Context
 import androidx.test.core.app.ApplicationProvider
 import org.junit.Assert.assertEquals
 import org.junit.Test
+import org.junit.Before
 import org.junit.runner.RunWith
 import org.robolectric.RobolectricTestRunner
 import java.io.File
 
 @RunWith(RobolectricTestRunner::class)
 class GleanTest {
-    @Test
-    fun `simple smoke test`() {
-        GleanInternalAPI().initialize(ApplicationProvider.getApplicationContext())
+
+    @Before
+    fun setUp() {
+        resetGlean()
     }
 
     @Test
     fun `send a ping`() {
         val context: Context = ApplicationProvider.getApplicationContext()
-        val glean = GleanInternalAPI()
-        glean.initialize(context)
-        glean.handleBackgroundEvent()
         val pingPath = File(context.applicationInfo.dataDir, "glean_data/pings")
-        assertEquals(2, pingPath.listFiles()?.size)
+
+        Glean.handleBackgroundEvent()
+        // Only the baseline ping should have been written
+        assertEquals(1, pingPath.listFiles()?.size)
     }
 }

glean-core/examples/sample.rs

@@ -59,16 +59,13 @@ fn main() {
     list.add(&glean, "upon");
 
     let ping_maker = PingMaker::new();
-    let ping = ping_maker.collect(glean.storage(), "core");
-    let ping = ::serde_json::to_string_pretty(&ping).unwrap();
+    let ping = ping_maker.collect_string(glean.storage(), "core").unwrap();
     println!("Ping:\n{}", ping);
 
     let mut long_string = std::iter::repeat('x').take(49).collect::<String>();
     long_string.push('a');
     long_string.push('b');
     local_metric.set(&glean, long_string);
-    let ping_maker = PingMaker::new();
-    let ping = ping_maker.collect(glean.storage(), "core");
-    let ping = ::serde_json::to_string_pretty(&ping).unwrap();
+    let ping = ping_maker.collect_string(glean.storage(), "core").unwrap();
     println!("Metrics Ping:\n{}", ping);
 }

glean-core/ffi/src/lib.rs

@@ -229,7 +229,9 @@ pub extern "C" fn glean_string_set(glean_handle: u64, metric_id: u64, value: Ffi
 pub extern "C" fn glean_ping_collect(glean_handle: u64, ping_name: FfiStr) -> *mut c_char {
     GLEAN.call_infallible(glean_handle, |glean| {
         let ping_maker = glean_core::ping::PingMaker::new();
-        let data = ping_maker.collect_string(glean.storage(), ping_name.as_str());
+        let data = ping_maker
+            .collect_string(glean.storage(), ping_name.as_str())
+            .unwrap_or_else(|| String::from(""));
         log::info!("Ping({}): {}", ping_name.as_str(), data);
         data
     })

glean-core/src/lib.rs

@@ -105,7 +105,9 @@ impl Glean {
     }
 
     pub fn snapshot(&mut self, store_name: &str, clear_store: bool) -> String {
-        StorageManager.snapshot(&self.storage(), store_name, clear_store)
+        StorageManager
+            .snapshot(&self.storage(), store_name, clear_store)
+            .unwrap_or_else(|| String::from(""))
     }
 
     fn make_path(&self, ping_name: &str, doc_id: &str) -> String {
@@ -130,12 +132,37 @@ impl Glean {
         let ping_maker = PingMaker::new();
         let doc_id = Uuid::new_v4().to_string();
         let url_path = self.make_path(ping_name, &doc_id);
-        let ping_content =
-            ::serde_json::to_string_pretty(&ping_maker.collect(self.storage(), ping_name))
-                .expect("Pretty-printing JSON into a string should always work");
+        let ping_content = match ping_maker.collect_string(self.storage(), ping_name) {
+            None => {
+                log::info!(
+                    "No content for ping '{}', therefore no ping queued.",
+                    ping_name
+                );
+                return Ok(());
+            }
+            Some(content) => content,
+        };
+
         // FIXME: Logging ping content for now.  Eventually this should be controlled by a flag
         log::info!("{}", ping_content);
         ping_maker.store_ping(&doc_id, &self.get_data_path(), &url_path, &ping_content)?;
         Ok(())
     }
 }
+
+#[cfg(test)]
+mod test {
+    use super::*;
+
+    #[test]
+    fn path_is_constructed_from_data() {
+        let t = tempfile::tempdir().unwrap();
+        let name = t.path().display().to_string();
+        let glean = Glean::new(&name, "org.mozilla.glean").unwrap();
+
+        assert_eq!(
+            "/submit/org-mozilla-glean/baseline/1/this-is-a-docid",
+            glean.make_path("baseline", "this-is-a-docid")
+        );
+    }
+}

glean-core/src/metrics/counter.rs

@@ -36,7 +36,10 @@ impl CounterMetric {
     ///
     /// This doesn't clear the stored value.
     pub fn test_get_value(&self, glean: &Glean, storage_name: &str) -> Option<u64> {
-        let snapshot = StorageManager.snapshot_as_json(glean.storage(), storage_name, false);
+        let snapshot = match StorageManager.snapshot_as_json(glean.storage(), storage_name, false) {
+            Some(snapshot) => snapshot,
+            None => return None,
+        };
         snapshot
             .as_object()
             .and_then(|o| o.get("counter"))

glean-core/src/metrics/string.rs

@@ -43,7 +43,10 @@ impl StringMetric {
     ///
     /// This doesn't clear the stored value.
     pub fn test_get_value(&self, glean: &Glean, storage_name: &str) -> Option<String> {
-        let snapshot = StorageManager.snapshot_as_json(glean.storage(), storage_name, false);
+        let snapshot = match StorageManager.snapshot_as_json(glean.storage(), storage_name, false) {
+            Some(snapshot) => snapshot,
+            None => return None,
+        };
         snapshot
             .as_object()
             .and_then(|o| o.get("string"))

glean-core/src/ping/mod.rs

@@ -52,7 +52,6 @@ impl PingMaker {
     }
 
     fn get_client_info(&self, storage: &Database) -> JsonValue {
-        let client_info = StorageManager.snapshot_as_json(storage, "glean_client_info", true);
         // Add the "telemetry_sdk_build", which is the glean-core version.
         let version = env!("CARGO_PKG_VERSION");
         let mut map = json!({
@@ -60,35 +59,42 @@ impl PingMaker {
         });
 
         // Flatten the whole thing.
-        let client_info_obj = client_info
-            .as_object()
-            .expect("Storage manager should guarantee an object for client_info");
-        for (_key, value) in client_info_obj {
-            merge(&mut map, value);
-        }
+        if let Some(client_info) =
+            StorageManager.snapshot_as_json(storage, "glean_client_info", true)
+        {
+            let client_info_obj = client_info.as_object().unwrap(); // safe, snapshot always returns an object.
+            for (_key, value) in client_info_obj {
+                merge(&mut map, value);
+            }
+        };
 
         json!(map)
     }
 
-    pub fn collect(&self, storage: &Database, storage_name: &str) -> JsonValue {
+    pub fn collect(&self, storage: &Database, storage_name: &str) -> Option<JsonValue> {
         info!("Collecting {}", storage_name);
 
-        let metrics_data = StorageManager.snapshot_as_json(storage, storage_name, true);
+        let metrics_data = match StorageManager.snapshot_as_json(storage, storage_name, true) {
+            None => {
+                info!("Storage for {} empty. Bailing out.", storage_name);
+                return None;
+            }
+            Some(data) => data,
+        };
 
         let ping_info = self.get_ping_info(storage_name);
         let client_info = self.get_client_info(storage);
 
-        json!({
+        Some(json!({
             "ping_info": ping_info,
             "client_info": client_info,
             "metrics": metrics_data,
-        })
+        }))
     }
 
-    pub fn collect_string(&self, storage: &Database, storage_name: &str) -> String {
-        let ping = self.collect(storage, storage_name);
-        ::serde_json::to_string_pretty(&ping)
-            .expect("Pretty-printing JSON into a string should always work")
+    pub fn collect_string(&self, storage: &Database, storage_name: &str) -> Option<String> {
+        self.collect(storage, storage_name)
+            .map(|ping| ::serde_json::to_string_pretty(&ping).unwrap())
     }
 
     fn get_pings_dir(&self, data_path: &Path) -> std::io::Result<PathBuf> {

glean-core/src/storage/mod.rs

@@ -15,10 +15,14 @@ use crate::Lifetime;
 pub struct StorageManager;
 
 impl StorageManager {
-    pub fn snapshot(&self, storage: &Database, store_name: &str, clear_store: bool) -> String {
-        let data = self.snapshot_as_json(storage, store_name, clear_store);
-        ::serde_json::to_string_pretty(&data)
-            .expect("Pretty-printing JSON into a string should always work")
+    pub fn snapshot(
+        &self,
+        storage: &Database,
+        store_name: &str,
+        clear_store: bool,
+    ) -> Option<String> {
+        self.snapshot_as_json(storage, store_name, clear_store)
+            .map(|data| ::serde_json::to_string_pretty(&data).unwrap())
     }
 
     pub fn snapshot_as_json(
@@ -26,7 +30,7 @@ impl StorageManager {
         storage: &Database,
         store_name: &str,
         clear_store: bool,
-    ) -> JsonValue {
+    ) -> Option<JsonValue> {
         let mut snapshot: HashMap<&str, HashMap<String, JsonValue>> = HashMap::new();
 
         let mut snapshotter = |metric_name: &[u8], metric: &Metric| {
@@ -45,6 +49,10 @@ impl StorageManager {
             storage.clear_ping_lifetime_storage(store_name);
         }
 
-        json!(snapshot)
+        if snapshot.is_empty() {
+            None
+        } else {
+            Some(json!(snapshot))
+        }
     }
 }

glean-core/tests/simple.rs

@@ -249,6 +249,15 @@ fn write_ping_to_disk() {
     let (temp, tmpname) = tempdir();
     let glean = Glean::new(&tmpname, GLOBAL_APPLICATION_ID).unwrap();
 
+    // We need to store a metric as an empty ping is not stored.
+    let counter = CounterMetric::new(CommonMetricData {
+        name: "counter".into(),
+        category: "local".into(),
+        send_in_pings: vec!["metrics".into()],
+        ..Default::default()
+    });
+    counter.add(&glean, 1);
+
     glean.send_ping("metrics").unwrap();
 
     let path = temp.path().join("pings");