1551692: Don't schedule the uploading worker if no ping queued (#67)

* 1551692: Don't schedule the uploading worker if no ping queued

* Address feedback in the PR

* Use toByte

glean-core/android/src/main/java/mozilla/telemetry/glean/Glean.kt

@@ -15,6 +15,7 @@ import java.io.File
 import mozilla.telemetry.glean.rust.LibGleanFFI
 import mozilla.telemetry.glean.rust.MetricHandle
 import mozilla.telemetry.glean.rust.RustError
+import mozilla.telemetry.glean.rust.toByte
 import mozilla.telemetry.glean.GleanMetrics.GleanBaseline
 import mozilla.telemetry.glean.GleanMetrics.GleanInternalMetrics
 import mozilla.telemetry.glean.scheduler.PingUploadWorker
@@ -169,10 +170,15 @@ open class GleanInternalAPI internal constructor () {
         sendPing("events")
     }
 
-    private fun sendPing(pingName: String) {
-        LibGleanFFI.INSTANCE.glean_send_ping(handle, pingName)
-        // TODO: 1551692 Only do this when ping content was actually queued
-        PingUploadWorker.enqueueWorker()
+    internal fun sendPing(pingName: String) {
+        val queued = LibGleanFFI.INSTANCE.glean_send_ping(
+            handle,
+            pingName,
+            (configuration.logPings).toByte()
+        )
+        if (queued != 0.toByte()) {
+            PingUploadWorker.enqueueWorker()
+        }
     }
 
     /**

glean-core/android/src/main/java/mozilla/telemetry/glean/rust/LibGleanFFI.kt

@@ -105,7 +105,7 @@ internal interface LibGleanFFI : Library {
 
     fun glean_ping_collect(glean_handle: Long, ping_name: String): Pointer?
 
-    fun glean_send_ping(glean_handle: Long, ping_name: String)
+    fun glean_send_ping(glean_handle: Long, ping_name: String, log_ping: Byte): Byte
 
     fun glean_set_upload_enabled(glean_handle: Long, flag: Byte)
 

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

@@ -6,7 +6,9 @@ package mozilla.telemetry.glean
 
 import android.content.Context
 import androidx.test.core.app.ApplicationProvider
+import mozilla.telemetry.glean.scheduler.PingUploadWorker
 import org.junit.Assert.assertEquals
+import org.junit.Assert.assertFalse
 import org.junit.Before
 import org.junit.Test
 import org.junit.runner.RunWith
@@ -44,4 +46,10 @@ class GleanTest {
         val docType = request.path.split("/")[3]
         assertEquals("baseline", docType)
     }
+
+    @Test
+    fun `sending an empty ping doesn't queue work`() {
+        Glean.sendPing("custom")
+        assertFalse(isWorkScheduled(PingUploadWorker.PING_WORKER_TAG))
+    }
 }

glean-core/ffi/examples/glean.h

@@ -97,7 +97,7 @@ uint64_t glean_new_string_metric(FfiStr category,
 
 char *glean_ping_collect(uint64_t glean_handle, FfiStr ping_name);
 
-void glean_send_ping(uint64_t glean_handle, FfiStr ping_name);
+uint8_t glean_send_ping(uint64_t glean_handle, FfiStr ping_name, uint8_t log_ping);
 
 void glean_set_upload_enabled(uint64_t glean_handle, uint8_t flag);
 

glean-core/ffi/src/lib.rs

@@ -78,10 +78,9 @@ pub extern "C" fn glean_set_upload_enabled(glean_handle: u64, flag: u8) {
 }
 
 #[no_mangle]
-pub extern "C" fn glean_send_ping(glean_handle: u64, ping_name: FfiStr) {
+pub extern "C" fn glean_send_ping(glean_handle: u64, ping_name: FfiStr, log_ping: u8) -> u8 {
     GLEAN.call_with_log(glean_handle, |glean| {
-        glean.send_ping(ping_name.as_str())?;
-        Ok(())
+        glean.send_ping(ping_name.as_str(), log_ping != 0)
     })
 }
 

glean-core/src/lib.rs

@@ -128,7 +128,10 @@ impl Glean {
     ///
     /// TODO: (Verify this is correct):
     /// If the ping currently contains no content, it will not be sent.
-    pub fn send_ping(&self, ping_name: &str) -> Result<()> {
+    ///
+    /// Returns true if a ping was sent, false otherwise.
+    /// Returns an error if collecting or writing the ping to disk failed.
+    pub fn send_ping(&self, ping_name: &str, log_ping: bool) -> Result<bool> {
         let ping_maker = PingMaker::new();
         let doc_id = Uuid::new_v4().to_string();
         let url_path = self.make_path(ping_name, &doc_id);
@@ -138,15 +141,17 @@ impl Glean {
                     "No content for ping '{}', therefore no ping queued.",
                     ping_name
                 );
-                return Ok(());
+                return Ok(false);
             }
             Some(content) => content,
         };
 
-        // FIXME: Logging ping content for now.  Eventually this should be controlled by a flag
-        log::info!("{}", ping_content);
+        if log_ping {
+            log::info!("{}", ping_content);
+        }
+
         ping_maker.store_ping(&doc_id, &self.get_data_path(), &url_path, &ping_content)?;
-        Ok(())
+        Ok(true)
     }
 }
 

glean-core/tests/simple.rs

@@ -258,7 +258,7 @@ fn write_ping_to_disk() {
     });
     counter.add(&glean, 1);
 
-    glean.send_ping("metrics").unwrap();
+    assert!(glean.send_ping("metrics", false).unwrap());
 
     let path = temp.path().join("pings");