From 110e44584de68a960e92542903b0c3e093f3eaa4 Mon Sep 17 00:00:00 2001 From: Michael Droettboom Date: Thu, 16 May 2019 18:07:21 -0400 Subject: [PATCH] 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 --- .../main/java/mozilla/telemetry/glean/Glean.kt | 14 ++++++++++---- .../mozilla/telemetry/glean/rust/LibGleanFFI.kt | 2 +- .../java/mozilla/telemetry/glean/GleanTest.kt | 8 ++++++++ glean-core/ffi/examples/glean.h | 2 +- glean-core/ffi/src/lib.rs | 5 ++--- glean-core/src/lib.rs | 15 ++++++++++----- glean-core/tests/simple.rs | 2 +- 7 files changed, 33 insertions(+), 15 deletions(-) diff --git a/glean-core/android/src/main/java/mozilla/telemetry/glean/Glean.kt b/glean-core/android/src/main/java/mozilla/telemetry/glean/Glean.kt index 1809bc613..f9b6969c0 100644 --- a/glean-core/android/src/main/java/mozilla/telemetry/glean/Glean.kt +++ b/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() + } } /** diff --git a/glean-core/android/src/main/java/mozilla/telemetry/glean/rust/LibGleanFFI.kt b/glean-core/android/src/main/java/mozilla/telemetry/glean/rust/LibGleanFFI.kt index a14b5c57f..b525ee8d3 100644 --- a/glean-core/android/src/main/java/mozilla/telemetry/glean/rust/LibGleanFFI.kt +++ b/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) diff --git a/glean-core/android/src/test/java/mozilla/telemetry/glean/GleanTest.kt b/glean-core/android/src/test/java/mozilla/telemetry/glean/GleanTest.kt index e440c2e6f..e080b4409 100644 --- a/glean-core/android/src/test/java/mozilla/telemetry/glean/GleanTest.kt +++ b/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)) + } } diff --git a/glean-core/ffi/examples/glean.h b/glean-core/ffi/examples/glean.h index 6fa39a6dc..3048d3fca 100644 --- a/glean-core/ffi/examples/glean.h +++ b/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); diff --git a/glean-core/ffi/src/lib.rs b/glean-core/ffi/src/lib.rs index f3cdaf929..6e54e5d50 100755 --- a/glean-core/ffi/src/lib.rs +++ b/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) }) } diff --git a/glean-core/src/lib.rs b/glean-core/src/lib.rs index b78239e7f..47f5f4688 100755 --- a/glean-core/src/lib.rs +++ b/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 { 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) } } diff --git a/glean-core/tests/simple.rs b/glean-core/tests/simple.rs index 6f45545d2..f96857ecc 100644 --- a/glean-core/tests/simple.rs +++ b/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");