From 3456803129714f4c9166f1a6d9aaf4d2921067b9 Mon Sep 17 00:00:00 2001 From: brizental Date: Fri, 10 Jul 2020 14:02:32 +0200 Subject: [PATCH 01/10] Add functionality to manage log_pings from the core --- glean-core/ffi/examples/glean_app.c | 5 ++-- glean-core/ffi/glean.h | 4 ++- glean-core/ffi/src/lib.rs | 9 ++++-- glean-core/ios/Glean/GleanFfi.h | 4 ++- glean-core/src/lib.rs | 26 ++++++++++++++-- glean-core/src/lib_unit_tests.rs | 14 +++++++++ glean-core/src/upload/mod.rs | 46 ++++++++++++++--------------- 7 files changed, 77 insertions(+), 31 deletions(-) diff --git a/glean-core/ffi/examples/glean_app.c b/glean-core/ffi/examples/glean_app.c index 445c5e1ad7..fcf222c2c0 100644 --- a/glean-core/ffi/examples/glean_app.c +++ b/glean-core/ffi/examples/glean_app.c @@ -45,14 +45,15 @@ int main(void) // NOTE: If, there are other ping files inside tmp/glean_data directory // they will also be consumed here by `glean_process_ping_upload_response`. FfiPingUploadTask task; - glean_get_upload_task(&task, 1); + glean_set_log_pings(1); + glean_get_upload_task(&task); while (task.tag != FfiPingUploadTask_Done) { printf("tag: %d\n", task.tag); if (task.tag == FfiPingUploadTask_Upload) { printf("path: %s\n", task.upload.path); - printf("body length: %lld\n", task.upload.body.len); + printf("body length: %d\n", task.upload.body.len); glean_process_ping_upload_response(&task, UPLOAD_RESULT_HTTP_STATUS | 200); } diff --git a/glean-core/ffi/glean.h b/glean-core/ffi/glean.h index 87bf31c1fe..70a59aea42 100644 --- a/glean-core/ffi/glean.h +++ b/glean-core/ffi/glean.h @@ -418,7 +418,7 @@ char *glean_experiment_test_get_data(FfiStr experiment_id); uint8_t glean_experiment_test_is_active(FfiStr experiment_id); -void glean_get_upload_task(FfiPingUploadTask *result, uint8_t log_ping); +void glean_get_upload_task(FfiPingUploadTask *result); /** * # Safety @@ -660,6 +660,8 @@ void glean_set_experiment_active(FfiStr experiment_id, void glean_set_experiment_inactive(FfiStr experiment_id); +void glean_set_log_pings(uint8_t value); + void glean_set_upload_enabled(uint8_t flag); /** diff --git a/glean-core/ffi/src/lib.rs b/glean-core/ffi/src/lib.rs index d0371d6203..f4c1ab017a 100644 --- a/glean-core/ffi/src/lib.rs +++ b/glean-core/ffi/src/lib.rs @@ -358,9 +358,9 @@ pub extern "C" fn glean_is_first_run() -> u8 { // // * `result`: the object the output task will be written to. #[no_mangle] -pub extern "C" fn glean_get_upload_task(result: *mut FfiPingUploadTask, log_ping: u8) { +pub extern "C" fn glean_get_upload_task(result: *mut FfiPingUploadTask) { with_glean_value(|glean| { - let ffi_task = FfiPingUploadTask::from(glean.get_upload_task(log_ping != 0)); + let ffi_task = FfiPingUploadTask::from(glean.get_upload_task()); unsafe { std::ptr::write(result, ffi_task); } @@ -435,4 +435,9 @@ pub extern "C" fn glean_set_debug_view_tag(tag: FfiStr) -> u8 { }) } +#[no_mangle] +pub extern "C" fn glean_set_log_pings(value: u8) { + with_glean_mut(|glean| Ok(glean.set_log_pings(value != 0))); +} + define_string_destructor!(glean_str_free); diff --git a/glean-core/ios/Glean/GleanFfi.h b/glean-core/ios/Glean/GleanFfi.h index 87bf31c1fe..70a59aea42 100644 --- a/glean-core/ios/Glean/GleanFfi.h +++ b/glean-core/ios/Glean/GleanFfi.h @@ -418,7 +418,7 @@ char *glean_experiment_test_get_data(FfiStr experiment_id); uint8_t glean_experiment_test_is_active(FfiStr experiment_id); -void glean_get_upload_task(FfiPingUploadTask *result, uint8_t log_ping); +void glean_get_upload_task(FfiPingUploadTask *result); /** * # Safety @@ -660,6 +660,8 @@ void glean_set_experiment_active(FfiStr experiment_id, void glean_set_experiment_inactive(FfiStr experiment_id); +void glean_set_log_pings(uint8_t value); + void glean_set_upload_enabled(uint8_t flag); /** diff --git a/glean-core/src/lib.rs b/glean-core/src/lib.rs index 2e3aa74f39..633dfedf93 100644 --- a/glean-core/src/lib.rs +++ b/glean-core/src/lib.rs @@ -499,8 +499,8 @@ impl Glean { /// # Return value /// /// `PingUploadTask` - an enum representing the possible tasks. - pub fn get_upload_task(&self, log_ping: bool) -> PingUploadTask { - self.upload_manager.get_upload_task(log_ping) + pub fn get_upload_task(&self) -> PingUploadTask { + self.upload_manager.get_upload_task(self.log_pings()) } /// Processes the response from an attempt to upload a ping. @@ -736,6 +736,28 @@ impl Glean { self.debug.debug_view_tag.get() } + /// Set the log pings debug option. + /// + /// This will return `false` in case we are unable to set the option. + /// + /// When the log pings debug option is `true`, + /// we log the payload of all succesfully assembled pings. + /// + /// ## Arguments + /// + /// * `value` - The value of the log pings option + pub fn set_log_pings(&mut self, value: bool) -> bool { + self.debug.log_pings.set(value) + } + + /// Return the value for the log pings debug option or `None` if it hasn't been set. + /// + /// The log_pings option may be set from an environment variable (GLEAN_LOG_PINGS) + /// or through the `set_log_pings` function. + pub(crate) fn log_pings(&self) -> bool { + self.debug.log_pings.get().copied().unwrap_or(false) + } + fn get_dirty_bit_metric(&self) -> metrics::BooleanMetric { metrics::BooleanMetric::new(CommonMetricData { name: "dirtybit".into(), diff --git a/glean-core/src/lib_unit_tests.rs b/glean-core/src/lib_unit_tests.rs index 0ad5ad17f5..7f46f2bdf9 100644 --- a/glean-core/src/lib_unit_tests.rs +++ b/glean-core/src/lib_unit_tests.rs @@ -779,6 +779,20 @@ fn test_setting_debug_view_tag() { assert_eq!(valid_tag, glean.debug_view_tag().unwrap()); } +#[test] +fn test_setting_log_pings() { + let dir = tempfile::tempdir().unwrap(); + + let (mut glean, _) = new_glean(Some(dir)); + assert!(!glean.log_pings()); + + glean.set_log_pings(true); + assert!(glean.log_pings()); + + glean.set_log_pings(false); + assert!(!glean.log_pings()); +} + #[test] #[should_panic] fn test_empty_application_id() { diff --git a/glean-core/src/upload/mod.rs b/glean-core/src/upload/mod.rs index 17202f6015..78c33d742f 100644 --- a/glean-core/src/upload/mod.rs +++ b/glean-core/src/upload/mod.rs @@ -627,7 +627,7 @@ mod test { let (mut glean, _) = new_glean(None); // Wait for processing of pending pings directory to finish. - while glean.get_upload_task(false) == PingUploadTask::Wait { + while glean.get_upload_task() == PingUploadTask::Wait { thread::sleep(Duration::from_millis(10)); } @@ -650,14 +650,14 @@ mod test { // Clear the queue drop(glean.upload_manager.clear_ping_queue()); - let upload_task = glean.get_upload_task(false); + let upload_task = glean.get_upload_task(); match upload_task { PingUploadTask::Upload(request) => assert!(request.is_deletion_request()), _ => panic!("Expected upload manager to return the next request!"), } // Verify there really isn't any other pings in the queue - assert_eq!(glean.get_upload_task(false), PingUploadTask::Done); + assert_eq!(glean.get_upload_task(), PingUploadTask::Done); } #[test] @@ -665,7 +665,7 @@ mod test { let (mut glean, _) = new_glean(None); // Wait for processing of pending pings directory to finish. - while glean.get_upload_task(false) == PingUploadTask::Wait { + while glean.get_upload_task() == PingUploadTask::Wait { thread::sleep(Duration::from_millis(10)); } @@ -680,10 +680,10 @@ mod test { } // Wait for processing of pending pings directory to finish. - let mut upload_task = glean.get_upload_task(false); + let mut upload_task = glean.get_upload_task(); while upload_task == PingUploadTask::Wait { thread::sleep(Duration::from_millis(10)); - upload_task = glean.get_upload_task(false); + upload_task = glean.get_upload_task(); } // Verify the requests were properly enqueued @@ -693,11 +693,11 @@ mod test { _ => panic!("Expected upload manager to return the next request!"), } - upload_task = glean.get_upload_task(false); + upload_task = glean.get_upload_task(); } // Verify that after all requests are returned, none are left - assert_eq!(glean.get_upload_task(false), PingUploadTask::Done); + assert_eq!(glean.get_upload_task(), PingUploadTask::Done); } #[test] @@ -705,7 +705,7 @@ mod test { let (mut glean, dir) = new_glean(None); // Wait for processing of pending pings directory to finish. - while glean.get_upload_task(false) == PingUploadTask::Wait { + while glean.get_upload_task() == PingUploadTask::Wait { thread::sleep(Duration::from_millis(10)); } @@ -720,7 +720,7 @@ mod test { let pending_pings_dir = dir.path().join(PENDING_PINGS_DIRECTORY); // Get the submitted PingRequest - match glean.get_upload_task(false) { + match glean.get_upload_task() { PingUploadTask::Upload(request) => { // Simulate the processing of a sucessfull request let document_id = request.document_id; @@ -732,7 +732,7 @@ mod test { } // Verify that after request is returned, none are left - assert_eq!(glean.get_upload_task(false), PingUploadTask::Done); + assert_eq!(glean.get_upload_task(), PingUploadTask::Done); } #[test] @@ -740,7 +740,7 @@ mod test { let (mut glean, dir) = new_glean(None); // Wait for processing of pending pings directory to finish. - while glean.get_upload_task(false) == PingUploadTask::Wait { + while glean.get_upload_task() == PingUploadTask::Wait { thread::sleep(Duration::from_millis(10)); } @@ -755,7 +755,7 @@ mod test { let pending_pings_dir = dir.path().join(PENDING_PINGS_DIRECTORY); // Get the submitted PingRequest - match glean.get_upload_task(false) { + match glean.get_upload_task() { PingUploadTask::Upload(request) => { // Simulate the processing of a client error let document_id = request.document_id; @@ -767,7 +767,7 @@ mod test { } // Verify that after request is returned, none are left - assert_eq!(glean.get_upload_task(false), PingUploadTask::Done); + assert_eq!(glean.get_upload_task(), PingUploadTask::Done); } #[test] @@ -775,7 +775,7 @@ mod test { let (mut glean, _) = new_glean(None); // Wait for processing of pending pings directory to finish. - while glean.get_upload_task(false) == PingUploadTask::Wait { + while glean.get_upload_task() == PingUploadTask::Wait { thread::sleep(Duration::from_millis(10)); } @@ -787,13 +787,13 @@ mod test { glean.submit_ping(&ping_type, None).unwrap(); // Get the submitted PingRequest - match glean.get_upload_task(false) { + match glean.get_upload_task() { PingUploadTask::Upload(request) => { // Simulate the processing of a client error let document_id = request.document_id; glean.process_ping_upload_response(&document_id, HttpStatus(500)); // Verify this ping was indeed re-enqueued - match glean.get_upload_task(false) { + match glean.get_upload_task() { PingUploadTask::Upload(request) => { assert_eq!(document_id, request.document_id); } @@ -804,7 +804,7 @@ mod test { } // Verify that after request is returned, none are left - assert_eq!(glean.get_upload_task(false), PingUploadTask::Done); + assert_eq!(glean.get_upload_task(), PingUploadTask::Done); } #[test] @@ -812,7 +812,7 @@ mod test { let (mut glean, dir) = new_glean(None); // Wait for processing of pending pings directory to finish. - while glean.get_upload_task(false) == PingUploadTask::Wait { + while glean.get_upload_task() == PingUploadTask::Wait { thread::sleep(Duration::from_millis(10)); } @@ -827,7 +827,7 @@ mod test { let pending_pings_dir = dir.path().join(PENDING_PINGS_DIRECTORY); // Get the submitted PingRequest - match glean.get_upload_task(false) { + match glean.get_upload_task() { PingUploadTask::Upload(request) => { // Simulate the processing of a client error let document_id = request.document_id; @@ -839,7 +839,7 @@ mod test { } // Verify that after request is returned, none are left - assert_eq!(glean.get_upload_task(false), PingUploadTask::Done); + assert_eq!(glean.get_upload_task(), PingUploadTask::Done); } #[test] @@ -905,7 +905,7 @@ mod test { let (mut glean, _) = new_glean(None); // Wait for processing of pending pings directory to finish. - while glean.get_upload_task(false) == PingUploadTask::Wait { + while glean.get_upload_task() == PingUploadTask::Wait { thread::sleep(Duration::from_millis(10)); } @@ -919,7 +919,7 @@ mod test { glean.submit_ping(&ping_type, None).unwrap(); // Get the submitted PingRequest - match glean.get_upload_task(false) { + match glean.get_upload_task() { PingUploadTask::Upload(request) => { assert_eq!(request.headers.get("X-Debug-ID").unwrap(), "valid-tag") } From 73f191aea7b41103e235737430bf8fa89cb6c690 Mon Sep 17 00:00:00 2001 From: brizental Date: Fri, 10 Jul 2020 14:03:29 +0200 Subject: [PATCH 02/10] Allow the core to manage log_pings in Kotlin --- .../java/mozilla/telemetry/glean/Glean.kt | 25 ++++++++++++ .../telemetry/glean/config/Configuration.kt | 40 +------------------ .../glean/debug/GleanDebugActivity.kt | 12 ++---- .../telemetry/glean/rust/LibGleanFFI.kt | 4 +- .../glean/scheduler/PingUploadWorker.kt | 4 +- .../java/mozilla/telemetry/glean/GleanTest.kt | 37 ++++++----------- .../java/mozilla/telemetry/glean/TestUtil.kt | 2 + .../glean/debug/GleanDebugActivityTest.kt | 37 ----------------- .../telemetry/glean/pings/CustomPingTest.kt | 6 +-- .../telemetry/glean/pings/DeletionPingTest.kt | 9 ++--- .../glean/private/EventMetricTypeTest.kt | 12 ++---- .../telemetry/glean/private/PingTypeTest.kt | 15 +++---- .../scheduler/MetricsPingSchedulerTest.kt | 9 ++--- .../glean/scheduler/PingUploadWorkerTest.kt | 2 +- 14 files changed, 68 insertions(+), 146 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 1baf2910e3..7063e86d7b 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 @@ -78,6 +78,9 @@ open class GleanInternalAPI internal constructor () { // Keep track of this value before Glean is initialized private var debugViewTag: String? = null + // Keep track of this value before Glean is initialized + private var logPings: Boolean? = null + // This object holds data related to any persistent information about the metrics ping, // such as the last time it was sent and the store name internal lateinit var metricsPingScheduler: MetricsPingScheduler @@ -177,6 +180,12 @@ open class GleanInternalAPI internal constructor () { setDebugViewTag(debugViewTag!!) } + // The log pings debug option might have been set before initialize, + // get the cached value and set it. + if (logPings != null) { + setLogPings(logPings!!) + } + // Get the current value of the dirty flag so we know whether to // send a dirty startup baseline ping below. Immediately set it to // `false` so that dirty startup pings won't be sent if Glean @@ -625,6 +634,22 @@ open class GleanInternalAPI internal constructor () { } } + /** + * Set the logPing debug option, when this is `true` + * the payload of assembled ping requests get logged. + * + * This is only meant to be used internally by the `GleanDebugActivity`. + * + * @param value The value of the option. + */ + fun setLogPings(value: Boolean) { + if (isInitialized()) { + return LibGleanFFI.INSTANCE.glean_set_log_pings(value.toByte()) + } else { + logPings = value + } + } + /** * TEST ONLY FUNCTION. * This is called by the GleanTestRule, to enable test mode. diff --git a/glean-core/android/src/main/java/mozilla/telemetry/glean/config/Configuration.kt b/glean-core/android/src/main/java/mozilla/telemetry/glean/config/Configuration.kt index b54c7a6346..e27c524b19 100644 --- a/glean-core/android/src/main/java/mozilla/telemetry/glean/config/Configuration.kt +++ b/glean-core/android/src/main/java/mozilla/telemetry/glean/config/Configuration.kt @@ -62,59 +62,23 @@ internal class FfiConfiguration( * @property serverEndpoint the server pings are sent to. Please note that this is * is only meant to be changed for tests. * @property maxEvents the number of events to store before the events ping is sent - * @property logPings whether to log ping contents to the console. This is only meant to be used - * internally by the `GleanDebugActivity`. * @property httpClient The HTTP client implementation to use for uploading pings. * @property channel the release channel the application is on, if known. This will be * sent along with all the pings, in the `client_info` section. */ -data class Configuration internal constructor( - val serverEndpoint: String, +data class Configuration @JvmOverloads internal constructor( + val serverEndpoint: String = DEFAULT_TELEMETRY_ENDPOINT, val channel: String? = null, val maxEvents: Int? = null, - val logPings: Boolean = DEFAULT_LOG_PINGS, // NOTE: since only simple object or strings can be made `const val`s, if the // default values for the lines below are ever changed, they are required // to change in the public constructor below. val httpClient: PingUploader = HttpURLConnectionUploader() ) { - /** - * Configuration for Glean. - * - * @property serverEndpoint the server pings are sent to. Please note that this is - * is only meant to be changed for tests. - * @param channel the release channel the application is on, if known. This will be - * sent along with all the pings, in the `client_info` section. - * @param maxEvents the number of events to store before the events ping is sent - * @param httpClient The HTTP client implementation to use for uploading pings. - */ - // This is the only public constructor this class should have. It should only - // expose things we want to allow external applications to change. Every test - // only or internal configuration option should be added to the above primary internal - // constructor and only initialized with a proper default when calling the primary - // constructor from the secondary, public one, below. - @JvmOverloads - constructor( - serverEndpoint: String = DEFAULT_TELEMETRY_ENDPOINT, - channel: String? = null, - maxEvents: Int? = null, - httpClient: PingUploader = HttpURLConnectionUploader() - ) : this ( - serverEndpoint = serverEndpoint, - maxEvents = maxEvents, - logPings = DEFAULT_LOG_PINGS, - httpClient = httpClient, - channel = channel - ) - companion object { /** * The default server pings are sent to. */ const val DEFAULT_TELEMETRY_ENDPOINT = "https://incoming.telemetry.mozilla.org" - /** - * Whether to log pings by default. - */ - const val DEFAULT_LOG_PINGS = false } } diff --git a/glean-core/android/src/main/java/mozilla/telemetry/glean/debug/GleanDebugActivity.kt b/glean-core/android/src/main/java/mozilla/telemetry/glean/debug/GleanDebugActivity.kt index fdf87fcf91..f58eaa7cfd 100644 --- a/glean-core/android/src/main/java/mozilla/telemetry/glean/debug/GleanDebugActivity.kt +++ b/glean-core/android/src/main/java/mozilla/telemetry/glean/debug/GleanDebugActivity.kt @@ -87,14 +87,10 @@ class GleanDebugActivity : Activity() { Glean.setDebugViewTag(debugViewTag) } - val debugConfig = Glean.configuration.copy( - logPings = intent.getBooleanExtra(LOG_PINGS_EXTRA_KEY, Glean.configuration.logPings) - ) - - // Finally set the default configuration before starting - // the real product's activity. - Log.i(LOG_TAG, "Setting debug config $debugConfig") - Glean.configuration = debugConfig + var logPings: Boolean? = intent.getBooleanExtra(LOG_PINGS_EXTRA_KEY, false) + logPings?.let { + Glean.setLogPings(logPings) + } intent.getStringExtra(SEND_PING_EXTRA_KEY)?.let { Glean.submitPingByName(it) 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 acad48d21e..93899e405f 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 @@ -556,12 +556,14 @@ internal interface LibGleanFFI : Library { storage_name: String ): Int - fun glean_get_upload_task(task: FfiPingUploadTask.ByReference, logPing: Byte) + fun glean_get_upload_task(task: FfiPingUploadTask.ByReference) fun glean_process_ping_upload_response(task: FfiPingUploadTask.ByReference, status: Int) fun glean_set_debug_view_tag(value: String): Byte + fun glean_set_log_pings(value: Byte) + // Misc fun glean_str_free(ptr: Pointer) diff --git a/glean-core/android/src/main/java/mozilla/telemetry/glean/scheduler/PingUploadWorker.kt b/glean-core/android/src/main/java/mozilla/telemetry/glean/scheduler/PingUploadWorker.kt index c3085a6e10..501ea4566e 100644 --- a/glean-core/android/src/main/java/mozilla/telemetry/glean/scheduler/PingUploadWorker.kt +++ b/glean-core/android/src/main/java/mozilla/telemetry/glean/scheduler/PingUploadWorker.kt @@ -15,7 +15,6 @@ import androidx.work.WorkManager import androidx.work.Worker import androidx.work.WorkerParameters import mozilla.telemetry.glean.rust.LibGleanFFI -import mozilla.telemetry.glean.rust.toByte import mozilla.telemetry.glean.Glean import mozilla.telemetry.glean.net.FfiPingUploadTask import mozilla.telemetry.glean.utils.testFlushWorkManagerJob @@ -108,8 +107,7 @@ class PingUploadWorker(context: Context, params: WorkerParameters) : Worker(cont // Create a slot of memory for the task: glean-core will write data into // the allocated memory. val incomingTask = FfiPingUploadTask.ByReference() - val logPings = Glean.configuration.logPings.toByte() - LibGleanFFI.INSTANCE.glean_get_upload_task(incomingTask, logPings) + LibGleanFFI.INSTANCE.glean_get_upload_task(incomingTask) when (val action = incomingTask.toPingUploadTask()) { is PingUploadTask.Upload -> { // Upload the ping request. 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 7b7423bd8f..b1a69bb4ef 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 @@ -86,8 +86,7 @@ class GleanTest { fun `send a ping`() { val server = getMockWebServer() resetGlean(context, Glean.configuration.copy( - serverEndpoint = "http://" + server.hostName + ":" + server.port, - logPings = true + serverEndpoint = "http://" + server.hostName + ":" + server.port )) Glean.handleBackgroundEvent() @@ -107,8 +106,7 @@ class GleanTest { fun `X-Debug-ID header is correctly added when debug view tag is set`() { val server = getMockWebServer() resetGlean(context, Glean.configuration.copy( - serverEndpoint = "http://" + server.hostName + ":" + server.port, - logPings = true + serverEndpoint = "http://" + server.hostName + ":" + server.port )) Glean.setDebugViewTag("this-ping-is-tagged") @@ -199,8 +197,7 @@ class GleanTest { val context = getContextWithMockedInfo() resetGlean(context, Glean.configuration.copy( - serverEndpoint = "http://" + server.hostName + ":" + server.port, - logPings = true + serverEndpoint = "http://" + server.hostName + ":" + server.port )) // Fake calling the lifecycle observer. @@ -277,8 +274,7 @@ class GleanTest { val server = getMockWebServer() val context = getContextWithMockedInfo() resetGlean(context, Glean.configuration.copy( - serverEndpoint = "http://" + server.hostName + ":" + server.port, - logPings = true + serverEndpoint = "http://" + server.hostName + ":" + server.port ), false) try { @@ -452,8 +448,7 @@ class GleanTest { val context = getContextWithMockedInfo() resetGlean(context, Glean.configuration.copy( - serverEndpoint = "http://" + server.hostName + ":" + server.port, - logPings = true + serverEndpoint = "http://" + server.hostName + ":" + server.port )) val pingName = "custom_ping_1" @@ -610,8 +605,7 @@ class GleanTest { Glean.testDestroyGleanHandle() // Now trigger execution to ensure the tasks fired Glean.initialize(context, true, Glean.configuration.copy( - serverEndpoint = "http://" + server.hostName + ":" + server.port, - logPings = true + serverEndpoint = "http://" + server.hostName + ":" + server.port )) assertEquals(110, GleanError.preinitTasksOverflow.testGetValue()) @@ -642,8 +636,7 @@ class GleanTest { resetGlean( context, Glean.configuration.copy( - serverEndpoint = "http://" + server.hostName + ":" + server.port, - logPings = true + serverEndpoint = "http://" + server.hostName + ":" + server.port ), uploadEnabled = true ) @@ -651,8 +644,7 @@ class GleanTest { resetGlean( context, Glean.configuration.copy( - serverEndpoint = "http://" + server.hostName + ":" + server.port, - logPings = true + serverEndpoint = "http://" + server.hostName + ":" + server.port ), uploadEnabled = false, clearStores = false @@ -672,8 +664,7 @@ class GleanTest { resetGlean( context, Glean.configuration.copy( - serverEndpoint = "http://" + server.hostName + ":" + server.port, - logPings = true + serverEndpoint = "http://" + server.hostName + ":" + server.port ), uploadEnabled = false ) @@ -681,8 +672,7 @@ class GleanTest { resetGlean( context, Glean.configuration.copy( - serverEndpoint = "http://" + server.hostName + ":" + server.port, - logPings = true + serverEndpoint = "http://" + server.hostName + ":" + server.port ), uploadEnabled = false, clearStores = false @@ -709,8 +699,7 @@ class GleanTest { val server = getMockWebServer() val context = getContextWithMockedInfo() resetGlean(context, Glean.configuration.copy( - serverEndpoint = "http://" + server.hostName + ":" + server.port, - logPings = true + serverEndpoint = "http://" + server.hostName + ":" + server.port ), false) try { @@ -815,9 +804,7 @@ class GleanTest { // Set the dirty flag. LibGleanFFI.INSTANCE.glean_set_dirty_flag(true.toByte()) - resetGlean(context, Glean.configuration.copy( - logPings = true - ), false) + resetGlean(context, Glean.configuration, false) assertFalse(LibGleanFFI.INSTANCE.glean_is_dirty_flag_set().toBoolean()) } diff --git a/glean-core/android/src/test/java/mozilla/telemetry/glean/TestUtil.kt b/glean-core/android/src/test/java/mozilla/telemetry/glean/TestUtil.kt index 65a7c9c85d..64a2d064cc 100644 --- a/glean-core/android/src/test/java/mozilla/telemetry/glean/TestUtil.kt +++ b/glean-core/android/src/test/java/mozilla/telemetry/glean/TestUtil.kt @@ -127,6 +127,8 @@ internal fun resetGlean( // in tests without this line. Let's simply put it here. WorkManagerTestInitHelper.initializeTestWorkManager(context) Glean.resetGlean(context, config, clearStores, uploadEnabled = uploadEnabled) + // Always log pings for tests + Glean.setLogPings(true) } /** diff --git a/glean-core/android/src/test/java/mozilla/telemetry/glean/debug/GleanDebugActivityTest.kt b/glean-core/android/src/test/java/mozilla/telemetry/glean/debug/GleanDebugActivityTest.kt index 20e76a1b93..880270ec9b 100644 --- a/glean-core/android/src/test/java/mozilla/telemetry/glean/debug/GleanDebugActivityTest.kt +++ b/glean-core/android/src/test/java/mozilla/telemetry/glean/debug/GleanDebugActivityTest.kt @@ -12,9 +12,7 @@ import androidx.test.core.app.ApplicationProvider import mozilla.telemetry.glean.Glean import mozilla.telemetry.glean.config.Configuration import org.junit.Assert.assertEquals -import org.junit.Assert.assertFalse import org.junit.Assert.assertNotEquals -import org.junit.Assert.assertNull import org.junit.Assert.assertTrue import org.junit.Before import android.content.pm.ActivityInfo @@ -76,41 +74,6 @@ class GleanDebugActivityTest { shadowOf(pm).addResolveInfoForIntent(launchIntent, resolveInfo) } - @Test - fun `the default configuration is not changed if no extras are provided`() { - val originalConfig = Configuration() - Glean.configuration = originalConfig - - // Build the intent that will call our debug activity, with no extra. - val intent = Intent(ApplicationProvider.getApplicationContext(), - GleanDebugActivity::class.java) - assertNull(intent.extras) - - // Start the activity through our intent. - launch(intent) - - // Verify that the original configuration and the one after init took place - // are the same. - assertEquals(originalConfig, Glean.configuration) - } - - @Test - fun `command line extra arguments are correctly parsed`() { - // Make sure to set a baseline configuration to check against. - val originalConfig = Configuration() - Glean.configuration = originalConfig - assertFalse(originalConfig.logPings) - - // Set the extra values and start the intent. - val intent = Intent(ApplicationProvider.getApplicationContext(), - GleanDebugActivity::class.java) - intent.putExtra(GleanDebugActivity.LOG_PINGS_EXTRA_KEY, true) - launch(intent) - - // Check that the configuration option was correctly flipped. - assertTrue(Glean.configuration.logPings) - } - @Test fun `the main activity is correctly started`() { // Build the intent that will call our debug activity, with no extra. diff --git a/glean-core/android/src/test/java/mozilla/telemetry/glean/pings/CustomPingTest.kt b/glean-core/android/src/test/java/mozilla/telemetry/glean/pings/CustomPingTest.kt index 791b1f1839..ffaeddf2c2 100644 --- a/glean-core/android/src/test/java/mozilla/telemetry/glean/pings/CustomPingTest.kt +++ b/glean-core/android/src/test/java/mozilla/telemetry/glean/pings/CustomPingTest.kt @@ -44,8 +44,7 @@ class CustomPingTest { val server = getMockWebServer() resetGlean(context, Glean.configuration.copy( - serverEndpoint = "http://" + server.hostName + ":" + server.port, - logPings = true + serverEndpoint = "http://" + server.hostName + ":" + server.port ), clearStores = true, uploadEnabled = true) // Define a new custom ping inline. @@ -69,8 +68,7 @@ class CustomPingTest { val server = getMockWebServer() resetGlean(context, Glean.configuration.copy( - serverEndpoint = "http://" + server.hostName + ":" + server.port, - logPings = true + serverEndpoint = "http://" + server.hostName + ":" + server.port ), clearStores = true, uploadEnabled = true) // Define a new custom ping inline. diff --git a/glean-core/android/src/test/java/mozilla/telemetry/glean/pings/DeletionPingTest.kt b/glean-core/android/src/test/java/mozilla/telemetry/glean/pings/DeletionPingTest.kt index 344df03362..adcd61d541 100644 --- a/glean-core/android/src/test/java/mozilla/telemetry/glean/pings/DeletionPingTest.kt +++ b/glean-core/android/src/test/java/mozilla/telemetry/glean/pings/DeletionPingTest.kt @@ -61,8 +61,7 @@ class DeletionPingTest { val context = getContextWithMockedInfo() resetGlean(context, Glean.configuration.copy( - serverEndpoint = "http://" + server.hostName + ":" + server.port, - logPings = true + serverEndpoint = "http://" + server.hostName + ":" + server.port ), clearStores = true, uploadEnabled = false) triggerWorkManager(context) @@ -77,8 +76,7 @@ class DeletionPingTest { val context = getContextWithMockedInfo() resetGlean(context, Glean.configuration.copy( - serverEndpoint = "http://" + server.hostName + ":" + server.port, - logPings = true + serverEndpoint = "http://" + server.hostName + ":" + server.port ), clearStores = true, uploadEnabled = true) // Get directory for pending deletion-request pings @@ -132,8 +130,7 @@ class DeletionPingTest { val context = getContextWithMockedInfo() resetGlean(context, Glean.configuration.copy( - serverEndpoint = "http://" + server.hostName + ":" + server.port, - logPings = true + serverEndpoint = "http://" + server.hostName + ":" + server.port ), clearStores = true, uploadEnabled = false) triggerWorkManager(context) diff --git a/glean-core/android/src/test/java/mozilla/telemetry/glean/private/EventMetricTypeTest.kt b/glean-core/android/src/test/java/mozilla/telemetry/glean/private/EventMetricTypeTest.kt index 66e28993b8..13e62ef3ba 100644 --- a/glean-core/android/src/test/java/mozilla/telemetry/glean/private/EventMetricTypeTest.kt +++ b/glean-core/android/src/test/java/mozilla/telemetry/glean/private/EventMetricTypeTest.kt @@ -238,8 +238,7 @@ class EventMetricTypeTest { resetGlean( context, Glean.configuration.copy( - serverEndpoint = "http://" + server.hostName + ":" + server.port, - logPings = true + serverEndpoint = "http://" + server.hostName + ":" + server.port ), clearStores = true ) @@ -260,8 +259,7 @@ class EventMetricTypeTest { resetGlean( context, Glean.configuration.copy( - serverEndpoint = "http://" + server.hostName + ":" + server.port, - logPings = true + serverEndpoint = "http://" + server.hostName + ":" + server.port ), clearStores = false ) @@ -299,8 +297,7 @@ class EventMetricTypeTest { resetGlean( context, Glean.configuration.copy( - serverEndpoint = "http://" + server.hostName + ":" + server.port, - logPings = true + serverEndpoint = "http://" + server.hostName + ":" + server.port ), clearStores = true ) @@ -323,8 +320,7 @@ class EventMetricTypeTest { resetGlean( context, Glean.configuration.copy( - serverEndpoint = "http://" + server.hostName + ":" + server.port, - logPings = true + serverEndpoint = "http://" + server.hostName + ":" + server.port ), clearStores = false ) diff --git a/glean-core/android/src/test/java/mozilla/telemetry/glean/private/PingTypeTest.kt b/glean-core/android/src/test/java/mozilla/telemetry/glean/private/PingTypeTest.kt index 07441ea174..6ba0f04732 100644 --- a/glean-core/android/src/test/java/mozilla/telemetry/glean/private/PingTypeTest.kt +++ b/glean-core/android/src/test/java/mozilla/telemetry/glean/private/PingTypeTest.kt @@ -39,8 +39,7 @@ class PingTypeTest { val context = getContextWithMockedInfo() resetGlean(context, Glean.configuration.copy( - serverEndpoint = "http://" + server.hostName + ":" + server.port, - logPings = true + serverEndpoint = "http://" + server.hostName + ":" + server.port )) val customPing = PingType( @@ -80,8 +79,7 @@ class PingTypeTest { val context = getContextWithMockedInfo() resetGlean(context, Glean.configuration.copy( - serverEndpoint = "http://" + server.hostName + ":" + server.port, - logPings = true + serverEndpoint = "http://" + server.hostName + ":" + server.port )) val customPing = PingType( @@ -121,8 +119,7 @@ class PingTypeTest { val context = getContextWithMockedInfo() resetGlean(context, Glean.configuration.copy( - serverEndpoint = "http://" + server.hostName + ":" + server.port, - logPings = true + serverEndpoint = "http://" + server.hostName + ":" + server.port )) val customPing = PingType( @@ -162,8 +159,7 @@ class PingTypeTest { val context = getContextWithMockedInfo() resetGlean(context, Glean.configuration.copy( - serverEndpoint = "http://" + server.hostName + ":" + server.port, - logPings = true + serverEndpoint = "http://" + server.hostName + ":" + server.port )) val customPing = PingType( @@ -211,8 +207,7 @@ class PingTypeTest { val context = getContextWithMockedInfo() resetGlean(context, Glean.configuration.copy( - serverEndpoint = "http://" + server.hostName + ":" + server.port, - logPings = true + serverEndpoint = "http://" + server.hostName + ":" + server.port )) counter.add() diff --git a/glean-core/android/src/test/java/mozilla/telemetry/glean/scheduler/MetricsPingSchedulerTest.kt b/glean-core/android/src/test/java/mozilla/telemetry/glean/scheduler/MetricsPingSchedulerTest.kt index 028aa75dbb..1965f33e44 100644 --- a/glean-core/android/src/test/java/mozilla/telemetry/glean/scheduler/MetricsPingSchedulerTest.kt +++ b/glean-core/android/src/test/java/mozilla/telemetry/glean/scheduler/MetricsPingSchedulerTest.kt @@ -258,8 +258,7 @@ class MetricsPingSchedulerTest { val context = getContextWithMockedInfo() resetGlean(context, Configuration( - serverEndpoint = "http://" + server.hostName + ":" + server.port, - logPings = true + serverEndpoint = "http://" + server.hostName + ":" + server.port )) try { @@ -481,7 +480,7 @@ class MetricsPingSchedulerTest { // Start the web-server that will receive the metrics ping. val server = getMockWebServer() val configuration = Configuration( - serverEndpoint = "http://" + server.hostName + ":" + server.port, logPings = true + serverEndpoint = "http://" + server.hostName + ":" + server.port ) val oldVersion = "version.0" @@ -697,7 +696,7 @@ class MetricsPingSchedulerTest { context, true, Configuration( - serverEndpoint = "http://" + server.hostName + ":" + server.port, logPings = true + serverEndpoint = "http://" + server.hostName + ":" + server.port ) ) @@ -775,7 +774,7 @@ class MetricsPingSchedulerTest { resetGlean( context, Configuration( - serverEndpoint = "http://" + server.hostName + ":" + server.port, logPings = true + serverEndpoint = "http://" + server.hostName + ":" + server.port ), false ) diff --git a/glean-core/android/src/test/java/mozilla/telemetry/glean/scheduler/PingUploadWorkerTest.kt b/glean-core/android/src/test/java/mozilla/telemetry/glean/scheduler/PingUploadWorkerTest.kt index 406531ef4e..ef4b87e3d1 100644 --- a/glean-core/android/src/test/java/mozilla/telemetry/glean/scheduler/PingUploadWorkerTest.kt +++ b/glean-core/android/src/test/java/mozilla/telemetry/glean/scheduler/PingUploadWorkerTest.kt @@ -31,7 +31,7 @@ class PingUploadWorkerTest { @Throws(Exception::class) fun setUp() { MockitoAnnotations.initMocks(this) - resetGlean(context, config = Configuration().copy(logPings = true)) + resetGlean(context, config = Configuration()) pingUploadWorker = PingUploadWorker(context, workerParams!!) } From 53727f39fb906f1703c156518f8d7f281d2a0bc9 Mon Sep 17 00:00:00 2001 From: brizental Date: Fri, 10 Jul 2020 15:30:13 +0200 Subject: [PATCH 03/10] Allow the core to manage log_ping in Swift --- .../ios/Glean/Config/Configuration.swift | 25 ------------------- .../ios/Glean/Debug/GleanDebugTools.swift | 4 +-- glean-core/ios/Glean/Glean.swift | 21 +++++++++++++++- .../ios/Glean/Net/HttpPingUploader.swift | 2 +- .../Config/ConfigurationTests.swift | 5 ---- .../Debug/GleanDebugUtilityTests.swift | 23 ++++++++--------- .../Net/HttpPingUploaderTests.swift | 7 ++---- 7 files changed, 36 insertions(+), 51 deletions(-) diff --git a/glean-core/ios/Glean/Config/Configuration.swift b/glean-core/ios/Glean/Config/Configuration.swift index a3c2b6a415..f9736642e7 100644 --- a/glean-core/ios/Glean/Config/Configuration.swift +++ b/glean-core/ios/Glean/Config/Configuration.swift @@ -6,35 +6,11 @@ /// property for calculating the `FfiConfiguration` public struct Configuration { let serverEndpoint: String - var logPings: Bool let maxEvents: Int32? let channel: String? struct Constants { static let defaultTelemetryEndpoint = "https://incoming.telemetry.mozilla.org" - static let defaultLogPings = false - } - - /// This init is for internal testing setup only. - /// - /// - parameters: - /// * serverEndpoint: A `String` representing the server the pings are sent to. - /// This should only be changed for tests. - /// * logPings: whether to log ping contents to the console. - /// This is only meant to be used internally by the `GleanDebugActivity`. - /// * maxEvents: the number of events to store before the events ping is sent. - /// * channel: the release channel the application is on, if known. - /// This will be sent along with all the pings, in the `client_info` section. - internal init( - serverEndpoint: String = Constants.defaultTelemetryEndpoint, - logPings: Bool = Constants.defaultLogPings, - maxEvents: Int32? = nil, - channel: String? = nil - ) { - self.serverEndpoint = serverEndpoint - self.logPings = logPings - self.maxEvents = maxEvents - self.channel = channel } /// Create a new Glean `Configuration` object @@ -49,7 +25,6 @@ public struct Configuration { serverEndpoint: String? = nil ) { self.serverEndpoint = serverEndpoint ?? Constants.defaultTelemetryEndpoint - self.logPings = Constants.defaultLogPings self.maxEvents = maxEvents self.channel = channel } diff --git a/glean-core/ios/Glean/Debug/GleanDebugTools.swift b/glean-core/ios/Glean/Debug/GleanDebugTools.swift index d9b28cd819..f9c7b2a1f6 100644 --- a/glean-core/ios/Glean/Debug/GleanDebugTools.swift +++ b/glean-core/ios/Glean/Debug/GleanDebugTools.swift @@ -21,7 +21,7 @@ class GleanDebugUtility { /// /// There are 3 available commands that you can use with the Glean SDK debug tools /// - /// - `logPings`: If "true", will cause pings that are submitted to also be echoed to the device's log + /// - `logPings`: If "true", will cause pings that are submitted successfully to also be echoed to the device's log /// - `tagPings`: This command expects a string to tag the pings with and redirects them to the Glean Debug View /// - `sendPing`: This command expects a string name of a ping to force immediate collection and submission of. /// @@ -63,7 +63,7 @@ class GleanDebugUtility { } if let logPings = parsedCommands.logPings { - Glean.shared.configuration?.logPings = logPings + Glean.shared.setLogPings(logPings) logger.debug("Log pings set to: \(logPings)") } diff --git a/glean-core/ios/Glean/Glean.swift b/glean-core/ios/Glean/Glean.swift index 1d6a7c204c..ecce833497 100644 --- a/glean-core/ios/Glean/Glean.swift +++ b/glean-core/ios/Glean/Glean.swift @@ -28,6 +28,7 @@ public class Glean { var initialized: Bool = false private var uploadEnabled: Bool = true private var debugViewTag: String? + var logPings: Bool? var configuration: Configuration? private var observer: GleanLifecycleObserver? @@ -119,6 +120,10 @@ public class Glean { _ = self.setDebugViewTag(self.debugViewTag!) } + if self.logPings != nil { + _ = self.setLogPings(self.logPings!) + } + // If any pings were registered before initializing, do so now for ping in self.pingTypeQueue { self.registerPingType(ping) @@ -465,6 +470,19 @@ public class Glean { } } + /// Set the log_pings debug option, + /// when this option is `true` the pings that are successfully submitted get logged. + /// + /// - parameters: + /// * value: The value of the option. + public func setLogPings(_ value: Bool) { + if self.isInitialized() { + glean_set_log_pings(value.toByte()) + } else { + logPings = value + } + } + /// When applications are launched using the custom URL scheme, this helper function will process /// the URL and parse the debug commands /// @@ -473,7 +491,6 @@ public class Glean { /// /// There are 3 available commands that you can use with the Glean SDK debug tools /// - /// - `logPings`: If "true", will cause pings that are submitted to also be echoed to the device's log /// - `tagPings`: This command expects a string to tag the pings with and redirects them to the Glean Debug View /// - `sendPing`: This command expects a string name of a ping to force immediate collection and submission of. /// @@ -570,5 +587,7 @@ public class Glean { // Init Glean. testDestroyGleanHandle() initialize(uploadEnabled: uploadEnabled, configuration: configuration) + // Enable ping logging for all tests + setLogPings(true) } } diff --git a/glean-core/ios/Glean/Net/HttpPingUploader.swift b/glean-core/ios/Glean/Net/HttpPingUploader.swift index d0e7c016c4..5b22b6ee02 100644 --- a/glean-core/ios/Glean/Net/HttpPingUploader.swift +++ b/glean-core/ios/Glean/Net/HttpPingUploader.swift @@ -101,7 +101,7 @@ public class HttpPingUploader { var uploadFailures = 0 while uploadFailures < Constants.maxRetries { var incomingTask = FfiPingUploadTask() - glean_get_upload_task(&incomingTask, config.logPings.toByte()) + glean_get_upload_task(&incomingTask) let task = incomingTask.toPingUploadTask() switch task { diff --git a/glean-core/ios/GleanTests/Config/ConfigurationTests.swift b/glean-core/ios/GleanTests/Config/ConfigurationTests.swift index 1740d9b5fd..4623b17c3b 100644 --- a/glean-core/ios/GleanTests/Config/ConfigurationTests.swift +++ b/glean-core/ios/GleanTests/Config/ConfigurationTests.swift @@ -22,11 +22,6 @@ class ConfigurationTests: XCTestCase { Configuration.Constants.defaultTelemetryEndpoint, "Default endpoint is set" ) - XCTAssertEqual( - config?.logPings, - Configuration.Constants.defaultLogPings, - "Default log pings is set" - ) XCTAssertNil( config?.maxEvents, "Default max events are set" diff --git a/glean-core/ios/GleanTests/Debug/GleanDebugUtilityTests.swift b/glean-core/ios/GleanTests/Debug/GleanDebugUtilityTests.swift index e94333e9ca..6ceb90a828 100644 --- a/glean-core/ios/GleanTests/Debug/GleanDebugUtilityTests.swift +++ b/glean-core/ios/GleanTests/Debug/GleanDebugUtilityTests.swift @@ -20,38 +20,37 @@ class GleanDebugUtilityTests: XCTestCase { } func testHandleCustomUrlLogPings() { - // Test initial state - XCTAssertFalse(Glean.shared.configuration!.logPings) + // We destroy the Glean handle so that Glean in in an unitialized state, + // this way it will save the value of `logPings` in the `logPings` prop. + Glean.shared.testDestroyGleanHandle() // Test toggle true var url = URL(string: "test://glean?logPings=true") Glean.shared.handleCustomUrl(url: url!) - XCTAssertTrue(Glean.shared.configuration!.logPings) + XCTAssertTrue(Glean.shared.logPings!) // Test invalid value doesn't cause setting to toggle - var previousValue = Glean.shared.configuration?.logPings + var previousValue = Glean.shared.logPings url = URL(string: "test://glean?logPings=Not-a-bool") Glean.shared.handleCustomUrl(url: url!) - XCTAssertEqual(previousValue, Glean.shared.configuration!.logPings) + XCTAssertEqual(previousValue, Glean.shared.logPings) // Test toggle false url = URL(string: "test://glean?logPings=false") Glean.shared.handleCustomUrl(url: url!) - XCTAssertFalse(Glean.shared.configuration!.logPings) + XCTAssertFalse(Glean.shared.logPings!) // Test invalid value doesn't cause setting to toggle - previousValue = Glean.shared.configuration?.logPings + previousValue = Glean.shared.logPings url = URL(string: "test://glean?logPings=Not-a-bool") Glean.shared.handleCustomUrl(url: url!) - XCTAssertEqual(previousValue, Glean.shared.configuration!.logPings) - } + XCTAssertEqual(previousValue, Glean.shared.logPings) - func testHandleCustomUrlWrongHost() { // This should NOT set the logPings to true or false because it doesn't // match the required host "glean". - let url = URL(string: "test://not-glean?logPings=true") + url = URL(string: "test://not-glean?logPings=true") Glean.shared.handleCustomUrl(url: url!) - XCTAssertEqual(false, Glean.shared.configuration?.logPings) + XCTAssertEqual(previousValue, Glean.shared.logPings) } func testHandleCustomUrlSendPing() { diff --git a/glean-core/ios/GleanTests/Net/HttpPingUploaderTests.swift b/glean-core/ios/GleanTests/Net/HttpPingUploaderTests.swift index 240fc5a664..40f6cd5886 100644 --- a/glean-core/ios/GleanTests/Net/HttpPingUploaderTests.swift +++ b/glean-core/ios/GleanTests/Net/HttpPingUploaderTests.swift @@ -11,9 +11,6 @@ class HttpPingUploaderTests: XCTestCase { var expectation: XCTestExpectation? private let testPath = "/some/random/path/not/important" private let testPing = "{ \"ping\": \"test\" }" - private let testConfig = Configuration( - logPings: true - ) override func tearDown() { // Reset expectations @@ -30,7 +27,7 @@ class HttpPingUploaderTests: XCTestCase { expectation = expectation(description: "Completed upload") - let httpPingUploader = HttpPingUploader(configuration: testConfig) + let httpPingUploader = HttpPingUploader(configuration: Configuration()) httpPingUploader.upload(path: testPath, data: Data(testPing.utf8), headers: [:]) { result in testValue = result self.expectation?.fulfill() @@ -51,7 +48,7 @@ class HttpPingUploaderTests: XCTestCase { // Build a request. // We specify a single additional header here. // In usual code they are generated on the Rust side. - let request = HttpPingUploader(configuration: testConfig) + let request = HttpPingUploader(configuration: Configuration()) .buildRequest(path: testPath, data: Data(testPing.utf8), headers: ["X-Client-Type": "Glean"]) XCTAssertEqual( From 28b0fc06cb81acf9d40db6177126df2477553b7d Mon Sep 17 00:00:00 2001 From: brizental Date: Fri, 10 Jul 2020 17:24:07 +0200 Subject: [PATCH 04/10] Allow the core to manage log_ping in C# --- glean-core/csharp/Glean/Configuration/Configuration.cs | 4 ---- glean-core/csharp/Glean/LibGleanFFI.cs | 2 +- glean-core/csharp/Glean/Net/BaseUploader.cs | 2 +- 3 files changed, 2 insertions(+), 6 deletions(-) diff --git a/glean-core/csharp/Glean/Configuration/Configuration.cs b/glean-core/csharp/Glean/Configuration/Configuration.cs index d4c3155f1a..485a257109 100644 --- a/glean-core/csharp/Glean/Configuration/Configuration.cs +++ b/glean-core/csharp/Glean/Configuration/Configuration.cs @@ -17,7 +17,6 @@ public sealed class Configuration public string channel; public string buildId; public int? maxEvents; - public bool logPings; public string pingTag; public IPingUploader httpClient; @@ -32,7 +31,6 @@ public sealed class Configuration /// a build identifier generated by the CI system /// the number of events to store before the events ping is sent. /// The HTTP client implementation to use for uploading pings. - /// Whether to log ping contents to the console /// String tag to be applied to headers when uploading pings for debug view. public Configuration( string serverEndpoint = DefaultTelemetryEndpoint, @@ -40,7 +38,6 @@ public Configuration( string buildId = null, int? maxEvents = null, IPingUploader httpClient = null, - bool logPings = false, string pingTag = null) { this.serverEndpoint = serverEndpoint; @@ -48,7 +45,6 @@ public Configuration( this.buildId = buildId; this.maxEvents = maxEvents; this.httpClient = httpClient ?? new HttpClientUploader(); - this.logPings = logPings; this.pingTag = pingTag; } } diff --git a/glean-core/csharp/Glean/LibGleanFFI.cs b/glean-core/csharp/Glean/LibGleanFFI.cs index a8cdf5c7ab..6e3ab2e214 100644 --- a/glean-core/csharp/Glean/LibGleanFFI.cs +++ b/glean-core/csharp/Glean/LibGleanFFI.cs @@ -295,7 +295,7 @@ Int32 reason_codes_len // Upload API [DllImport(SharedGleanLibrary, ExactSpelling = true, CallingConvention = CallingConvention.Cdecl)] - internal static extern void glean_get_upload_task(ref FfiUploadTask result, bool logPing); + internal static extern void glean_get_upload_task(ref FfiUploadTask result); [DllImport(SharedGleanLibrary, ExactSpelling = true, CallingConvention = CallingConvention.Cdecl)] internal static extern void glean_process_ping_upload_response(IntPtr task, int status); diff --git a/glean-core/csharp/Glean/Net/BaseUploader.cs b/glean-core/csharp/Glean/Net/BaseUploader.cs index d948674aca..2191e9c47f 100644 --- a/glean-core/csharp/Glean/Net/BaseUploader.cs +++ b/glean-core/csharp/Glean/Net/BaseUploader.cs @@ -105,7 +105,7 @@ internal void TriggerUpload(Configuration config) while (uploadFailures < MAX_RETRIES) { FfiUploadTask incomingTask = new FfiUploadTask(); - LibGleanFFI.glean_get_upload_task(ref incomingTask, config.logPings); + LibGleanFFI.glean_get_upload_task(ref incomingTask); UploadTaskTag tag = (UploadTaskTag)incomingTask.tag; switch (tag) From 9e8f1b2739e8546b37b4147509dfb48d50139d3e Mon Sep 17 00:00:00 2001 From: brizental Date: Fri, 10 Jul 2020 17:39:21 +0200 Subject: [PATCH 05/10] Allow the core to manage log_ping in Python --- glean-core/python/glean/config.py | 13 ------------- glean-core/python/glean/net/ping_upload_worker.py | 2 +- glean-core/python/tests/metrics/test_event.py | 8 ++------ glean-core/python/tests/test_glean.py | 4 ---- 4 files changed, 3 insertions(+), 24 deletions(-) diff --git a/glean-core/python/glean/config.py b/glean-core/python/glean/config.py index b321975fb6..a56238cbb2 100644 --- a/glean-core/python/glean/config.py +++ b/glean-core/python/glean/config.py @@ -31,7 +31,6 @@ def __init__( server_endpoint: str = DEFAULT_TELEMETRY_ENDPOINT, channel: Optional[str] = None, max_events: int = DEFAULT_MAX_EVENTS, - log_pings: bool = False, ping_tag: Optional[str] = None, ping_uploader: Optional[net.BaseUploader] = None, allow_multiprocessing: bool = True, @@ -44,8 +43,6 @@ def __init__( if known. max_events (int): Optional.The number of events to store before force-sending. Defaults to `DEFAULT_MAX_EVENTS`. - log_pings (bool): Optional. Whether to log ping contents to the - console. Defaults to `False`. ping_tag (str): Optional. String tag to be applied to headers when uploading pings for debug view. ping_uploader (glean.net.BaseUploader): Optional. The ping uploader @@ -56,7 +53,6 @@ def __init__( self._server_endpoint = server_endpoint self._channel = channel self._max_events = max_events - self._log_pings = log_pings self._ping_tag = ping_tag if ping_uploader is None: ping_uploader = net.HttpClientUploader() @@ -92,15 +88,6 @@ def max_events(self) -> int: # max_events can't be changed after Glean is initialized - @property - def log_pings(self) -> bool: - """Whether to log ping contents to the console.""" - return self._log_pings - - @log_pings.setter - def log_pings(self, value: bool): - self._log_pings = value - @property def ping_tag(self) -> Optional[str]: """String tag to be applied to headers when uploading pings for debug view.""" diff --git a/glean-core/python/glean/net/ping_upload_worker.py b/glean-core/python/glean/net/ping_upload_worker.py index c3d8e556dd..7a4022ba3e 100644 --- a/glean-core/python/glean/net/ping_upload_worker.py +++ b/glean-core/python/glean/net/ping_upload_worker.py @@ -124,7 +124,7 @@ def _process(data_dir: Path, application_id: str, configuration) -> bool: while upload_failures < MAX_RETRIES: incoming_task = ffi_support.new("FfiPingUploadTask *") - _ffi.lib.glean_get_upload_task(incoming_task, configuration.log_pings) + _ffi.lib.glean_get_upload_task(incoming_task) tag = incoming_task.tag if tag == UploadTaskTag.UPLOAD: diff --git a/glean-core/python/tests/metrics/test_event.py b/glean-core/python/tests/metrics/test_event.py index 2d66ff7532..b4c7414835 100644 --- a/glean-core/python/tests/metrics/test_event.py +++ b/glean-core/python/tests/metrics/test_event.py @@ -201,7 +201,6 @@ def test_flush_queued_events_on_startup(safe_httpserver): safe_httpserver.serve_content(b"", code=200) Glean._configuration.server_endpoint = safe_httpserver.url - Glean._configuration.log_pings = True class EventKeys(enum.Enum): SOME_EXTRA = 0 @@ -222,9 +221,7 @@ class EventKeys(enum.Enum): application_id="glean-python-test", application_version=glean_version, clear_stores=False, - configuration=Configuration( - server_endpoint=safe_httpserver.url, log_pings=True - ), + configuration=Configuration(server_endpoint=safe_httpserver.url), ) assert 1 == len(safe_httpserver.requests) @@ -239,7 +236,6 @@ def test_flush_queued_events_on_startup_and_correctly_handle_preinit_events( safe_httpserver.serve_content(b"", code=200) Glean._configuration.server_endpoint = safe_httpserver.url - Glean._configuration.log_pings = True class EventKeys(enum.Enum): SOME_EXTRA = 0 @@ -264,7 +260,7 @@ class EventKeys(enum.Enum): application_version=glean_version, clear_stores=False, configuration=Configuration( - server_endpoint=safe_httpserver.url, log_pings=True + server_endpoint=safe_httpserver.url ), ) diff --git a/glean-core/python/tests/test_glean.py b/glean-core/python/tests/test_glean.py index ee5208c82a..b315bb21a0 100644 --- a/glean-core/python/tests/test_glean.py +++ b/glean-core/python/tests/test_glean.py @@ -72,7 +72,6 @@ def test_submit_a_ping(safe_httpserver): safe_httpserver.serve_content(b"", code=200) Glean._configuration.server_endpoint = safe_httpserver.url - Glean._configuration.log_pings = True counter_metric = CounterMetricType( disabled=False, @@ -314,8 +313,6 @@ def test_ping_collection_must_happen_after_currently_scheduled_metrics_recording Glean._configuration, "ping_uploader", _RecordingUploader(info_path) ) - Glean._configuration.log_pings = True - ping_name = "custom_ping_1" ping = PingType( name=ping_name, include_client_id=True, send_if_empty=False, reason_codes=[] @@ -531,7 +528,6 @@ def test_configuration_property(safe_httpserver): safe_httpserver.serve_content(b"", code=200) Glean._configuration.server_endpoint = safe_httpserver.url - Glean._configuration.log_pings = True counter_metric = CounterMetricType( disabled=False, From b429d78bef0d7056cb28594d16e308d3e72ef8da Mon Sep 17 00:00:00 2001 From: brizental Date: Mon, 13 Jul 2020 15:55:06 +0200 Subject: [PATCH 06/10] Add some more logging to the debug module --- glean-core/src/debug.rs | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/glean-core/src/debug.rs b/glean-core/src/debug.rs index 4c65858bf5..88aa969a3f 100644 --- a/glean-core/src/debug.rs +++ b/glean-core/src/debug.rs @@ -48,6 +48,8 @@ impl DebugOptions { /// where the value can be set programmatically or come from an environment variable. #[derive(Debug)] pub struct DebugOption Option> { + /// The name of the environment variable related to this debug option. + env: String, /// The actual value of this option. value: Option, /// Optional function to validade the value parsed from the environment @@ -64,11 +66,12 @@ where /// tries to get the initial value of the option from the environment. pub fn new(env: &str, validation: Option) -> Self { let mut option = Self { + env: env.into(), value: None, validation, }; - option.set_from_env(env); + option.set_from_env(); option } @@ -80,29 +83,23 @@ where } } - fn set_from_env(&mut self, env: &str) { - match env::var(&env) { + fn set_from_env(&mut self) { + match env::var(&self.env) { Ok(env_value) => match T::from_str(&env_value) { Ok(v) => { - if !self.set(v) { - log::error!( - "Invalid value for debug option {}={}. Ignoring.", - &env, - env_value, - ); - } + self.set(v); } Err(_) => { log::error!( "Unable to parse debug option {}={} into {}. Ignoring.", - &env, + self.env, env_value, std::any::type_name::() ); } }, Err(env::VarError::NotUnicode(_)) => { - log::error!("The value of {} is not valid unicode. Ignoring.", &env) + log::error!("The value of {} is not valid unicode. Ignoring.", self.env) } // The other possible error is that the env var is not set, // which is not an error for us and can safely be ignored. @@ -117,9 +114,11 @@ where pub fn set(&mut self, value: T) -> bool { let validated = self.validate(value); if validated.is_some() { + log::info!("Setting the debug option {}.", self.env); self.value = validated; return true; } + log::info!("Invalid value for debug option {}.", self.env); false } From cc8db05773784f01a729c770107e06c342abf3a1 Mon Sep 17 00:00:00 2001 From: brizental Date: Tue, 14 Jul 2020 12:30:40 +0200 Subject: [PATCH 07/10] Add docs about debug features through env vars --- docs/user/debugging/android.md | 6 ++++++ docs/user/debugging/index.md | 15 ++++++++++++++- docs/user/debugging/ios.md | 8 ++++++++ docs/user/debugging/python.md | 33 ++++++++++----------------------- 4 files changed, 38 insertions(+), 24 deletions(-) diff --git a/docs/user/debugging/android.md b/docs/user/debugging/android.md index a88f681145..661d17df2b 100644 --- a/docs/user/debugging/android.md +++ b/docs/user/debugging/android.md @@ -54,6 +54,12 @@ persist until the application is closed or manually reset. > --es tagPings test-metrics-ping > ``` +Debugging features in Android can also be enabled using environment variables. +For more information on the available features accessible through this method +and how to enable them, see [Enabling debugging features through environment variables](./index.md). + +These environment variables must be set on the device that is running the application. + ### Glean Log messages When running a Glean-powered app in the Android emulator or on a device connected to your computer via cable, there are several ways to read the log output. diff --git a/docs/user/debugging/index.md b/docs/user/debugging/index.md index 25f7798cb2..70233bced2 100644 --- a/docs/user/debugging/index.md +++ b/docs/user/debugging/index.md @@ -12,12 +12,25 @@ There are 3 available commands that you can use with the Glean SDK debug tools -- `logPings`: This is either true or false and will cause pings that are submitted to also be echoed to the device's log +- `logPings`: This is either true or false and will cause pings that are submitted to also be echoed to the device's log. - `tagPings`: This command will tag outgoing pings with the provided value, in order to identify them in the Glean Debug View. Tags need to be string with upper and lower case letters, numbers and dashes, with a max length of 20 characters. - `sendPing`: This command expects a string name of a ping to force immediate collection and submission of. Different platforms have different ways to send these commands. +### Enabling debugging features through environment variables + +Some of the debugging features described above may also be enabled using environment variables: + +- `logPings`: May be set by the `GLEAN_LOG_PINGS` environment variable. The accepted values are +`true` or `false`. Any other value will be ignored. +- `tagPings`: May be set by the `GLEAN_DEBUG_VIEW_TAG` environment variable. Any valid HTTP header value maybe set here +(e.g. any value that matches the regex `[a-zA-Z0-9-]{1,20}`). Invalid values will be ignored. + +These variables must be set at runtime, not at compile time. They will be checked upon Glean initialization. + +Enabling debugging features using environment variables is available for all supported platforms. + ### Important considerations when using Glean SDK debug tools - Options that are set using the flags are not immediately reset and will persist until the application is closed or manually reset. diff --git a/docs/user/debugging/ios.md b/docs/user/debugging/ios.md index 133b897f7a..8329d31cfd 100644 --- a/docs/user/debugging/ios.md +++ b/docs/user/debugging/ios.md @@ -2,6 +2,14 @@ For debugging and validation purposes on iOS, Glean makes use of a custom URL scheme which is implemented _within the application_ that is consuming Glean. Glean provides some convenience functions to facilitate this, but it's up to the consuming application to enable this functionality. Applications that enable this Glean SDK feature will be able to launch the application from a URL with the Glean debug commands embedded in the URL itself. +Debugging features in iOS can also be enabled using environment variables. +For more information on the available features accessible through this method +and how to enable them, see [Enabling debugging features through environment variables](./index.md). + +These environment variables must be set on the device that is running the application. + +> **Note** To set environment variables to the process running your app in an iOS device or emulator you need to edit the scheme for your app. In the Xcode IDE, you can use the shortcut `Cmd + <` to open the scheme editor popup. The environment variables editor is under the `Arguments` tab on this popup. + ### Available commands and query format There are 3 available commands that you can use with the Glean SDK debug tools diff --git a/docs/user/debugging/python.md b/docs/user/debugging/python.md index f2cde1d709..5dff863000 100644 --- a/docs/user/debugging/python.md +++ b/docs/user/debugging/python.md @@ -1,37 +1,24 @@ # Debugging Python applications using the Glean SDK -Glean provides a couple of configuration flags to assist with debugging Python applications. +Debugging features in Python can be enabled using environment variables. +For more information on the available features and how to enable them, +see [Enabling debugging features through environment variables](./index.md). -## Tagging pings +## Sending pings -The `Glean.configuration.ping_tag` property can be used to add a special flag to the HTTP header so that the ping will end up in the [Glean Debug View](./debug-ping-view.md). +Unlike other platforms, Python doesn't expose convenience methods to send pings on demand. -You can set it after `Glean.initialize` is called: - -```py -from Glean import Glean, Configuration -Glean.initialize( - application_id="my-app-id", - application_version="0.1.0", - upload_enabled=True, -) - -# ... - -Glean.configuration.ping_tag = "my-ping-tag" -``` - -After doing so, something like `pings.custom_ping.submit()` will send the custom ping to the Glean Debug View. +In case that is necessary, calling the `submit` function for a given ping, +such as `pings.custom_ping.submit()`, will send it. ## Logging pings -If the `Glean.configuration.log_pings` property is set to `True`, pings are -logged to the console on `DEBUG` level whenever they are submitted. You can set -this property in a similar way as the `ping_tag` property above. +If the `GLEAN_LOG_PINGS` environment variable is set to `true`, pings are +logged to the console on `DEBUG` level whenever they are submitted. Make sure that when you configure logging in your application, you set the level for the `glean` logger to `DEBUG` or higher. Otherwise pings won't be -logged even if `log_pings` is set to `True`. +logged even if `GLEAN_LOG_PINGS` is set to `true`. You can set the logging level for Glean to `DEBUG` as follows: From 18f80af527fdeb6eaf6f75b51e3ce378bbb378e1 Mon Sep 17 00:00:00 2001 From: brizental Date: Tue, 14 Jul 2020 13:01:36 +0200 Subject: [PATCH 08/10] Set different env vars for each test in debug.rs This is important because test run in parallel, if both tests are changing the same env var at the same time, we get intermittent failures. --- glean-core/src/debug.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/glean-core/src/debug.rs b/glean-core/src/debug.rs index 88aa969a3f..e866e3525f 100644 --- a/glean-core/src/debug.rs +++ b/glean-core/src/debug.rs @@ -171,8 +171,8 @@ mod test { #[test] fn debug_option_is_correctly_loaded_from_env() { - env::set_var("GLEAN_TEST", "test"); - let option: DebugOption = DebugOption::new("GLEAN_TEST", None); + env::set_var("GLEAN_TEST_1", "test"); + let option: DebugOption = DebugOption::new("GLEAN_TEST_1", None); assert_eq!(option.get().unwrap(), "test"); } @@ -187,8 +187,8 @@ mod test { } // Invalid values from the env are not set - env::set_var("GLEAN_TEST", "invalid"); - let mut option: DebugOption = DebugOption::new("GLEAN_TEST", Some(validate)); + env::set_var("GLEAN_TEST_2", "invalid"); + let mut option: DebugOption = DebugOption::new("GLEAN_TEST_2", Some(validate)); assert!(option.get().is_none()); // Valid values are set using the `set` function From b509811095ae36a7192795196eeaa8b0f8850b16 Mon Sep 17 00:00:00 2001 From: brizental Date: Wed, 15 Jul 2020 10:25:02 +0200 Subject: [PATCH 09/10] Attend to review comments * Separate env vars text in it's own section on iOS docs * Remove env vars text from Android docs and add note about it * Correct exposure of methods in kt and swift * Set default value of `false` to logPings in kt and swift * Set logPings to true before init for tests setup in kt and swift --- docs/user/debugging/android.md | 6 ------ docs/user/debugging/index.md | 2 ++ docs/user/debugging/ios.md | 10 ++++++---- .../main/java/mozilla/telemetry/glean/Glean.kt | 12 +++++++----- .../telemetry/glean/config/Configuration.kt | 2 +- .../java/mozilla/telemetry/glean/TestUtil.kt | 2 +- glean-core/ios/Glean/Glean.swift | 16 ++++++++-------- .../Debug/GleanDebugUtilityTests.swift | 4 ++-- 8 files changed, 27 insertions(+), 27 deletions(-) diff --git a/docs/user/debugging/android.md b/docs/user/debugging/android.md index 661d17df2b..a88f681145 100644 --- a/docs/user/debugging/android.md +++ b/docs/user/debugging/android.md @@ -54,12 +54,6 @@ persist until the application is closed or manually reset. > --es tagPings test-metrics-ping > ``` -Debugging features in Android can also be enabled using environment variables. -For more information on the available features accessible through this method -and how to enable them, see [Enabling debugging features through environment variables](./index.md). - -These environment variables must be set on the device that is running the application. - ### Glean Log messages When running a Glean-powered app in the Android emulator or on a device connected to your computer via cable, there are several ways to read the log output. diff --git a/docs/user/debugging/index.md b/docs/user/debugging/index.md index 70233bced2..5ab2955338 100644 --- a/docs/user/debugging/index.md +++ b/docs/user/debugging/index.md @@ -31,6 +31,8 @@ These variables must be set at runtime, not at compile time. They will be checke Enabling debugging features using environment variables is available for all supported platforms. +> **Note** Although it is technically possible to use the environment variables described here to enable debugging features in Android. The Glean team is not currently aware of a proper way to set environment variables in Android devices or emulators. + ### Important considerations when using Glean SDK debug tools - Options that are set using the flags are not immediately reset and will persist until the application is closed or manually reset. diff --git a/docs/user/debugging/ios.md b/docs/user/debugging/ios.md index 8329d31cfd..f63383cb4b 100644 --- a/docs/user/debugging/ios.md +++ b/docs/user/debugging/ios.md @@ -1,8 +1,6 @@ -# Debugging iOS applications using the Glean SDK - -For debugging and validation purposes on iOS, Glean makes use of a custom URL scheme which is implemented _within the application_ that is consuming Glean. Glean provides some convenience functions to facilitate this, but it's up to the consuming application to enable this functionality. Applications that enable this Glean SDK feature will be able to launch the application from a URL with the Glean debug commands embedded in the URL itself. +# Enabling debugging features in iOS through environment variables -Debugging features in iOS can also be enabled using environment variables. +Debugging features in iOS can be enabled using environment variables. For more information on the available features accessible through this method and how to enable them, see [Enabling debugging features through environment variables](./index.md). @@ -10,6 +8,10 @@ These environment variables must be set on the device that is running the applic > **Note** To set environment variables to the process running your app in an iOS device or emulator you need to edit the scheme for your app. In the Xcode IDE, you can use the shortcut `Cmd + <` to open the scheme editor popup. The environment variables editor is under the `Arguments` tab on this popup. +# Debugging iOS applications using the Glean SDK + +For debugging and validation purposes on iOS, Glean makes use of a custom URL scheme which is implemented _within the application_ that is consuming Glean. Glean provides some convenience functions to facilitate this, but it's up to the consuming application to enable this functionality. Applications that enable this Glean SDK feature will be able to launch the application from a URL with the Glean debug commands embedded in the URL itself. + ### Available commands and query format There are 3 available commands that you can use with the Glean SDK debug tools 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 7063e86d7b..46a29f311b 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 @@ -79,7 +79,7 @@ open class GleanInternalAPI internal constructor () { private var debugViewTag: String? = null // Keep track of this value before Glean is initialized - private var logPings: Boolean? = null + private var logPings: Boolean = false // This object holds data related to any persistent information about the metrics ping, // such as the last time it was sent and the store name @@ -182,8 +182,8 @@ open class GleanInternalAPI internal constructor () { // The log pings debug option might have been set before initialize, // get the cached value and set it. - if (logPings != null) { - setLogPings(logPings!!) + if (logPings) { + setLogPings(logPings) } // Get the current value of the dirty flag so we know whether to @@ -623,7 +623,7 @@ open class GleanInternalAPI internal constructor () { * * @param value The value of the tag, which must be a valid HTTP header value. */ - fun setDebugViewTag(value: String): Boolean { + internal fun setDebugViewTag(value: String): Boolean { if (isInitialized()) { return LibGleanFFI.INSTANCE.glean_set_debug_view_tag(value).toBoolean() } else { @@ -642,7 +642,7 @@ open class GleanInternalAPI internal constructor () { * * @param value The value of the option. */ - fun setLogPings(value: Boolean) { + internal fun setLogPings(value: Boolean) { if (isInitialized()) { return LibGleanFFI.INSTANCE.glean_set_log_pings(value.toByte()) } else { @@ -689,6 +689,8 @@ open class GleanInternalAPI internal constructor () { // Init Glean. Glean.testDestroyGleanHandle() + // Always log pings for tests + Glean.setLogPings(true) Glean.initialize(context, uploadEnabled, config) } diff --git a/glean-core/android/src/main/java/mozilla/telemetry/glean/config/Configuration.kt b/glean-core/android/src/main/java/mozilla/telemetry/glean/config/Configuration.kt index e27c524b19..74afd2e62d 100644 --- a/glean-core/android/src/main/java/mozilla/telemetry/glean/config/Configuration.kt +++ b/glean-core/android/src/main/java/mozilla/telemetry/glean/config/Configuration.kt @@ -66,7 +66,7 @@ internal class FfiConfiguration( * @property channel the release channel the application is on, if known. This will be * sent along with all the pings, in the `client_info` section. */ -data class Configuration @JvmOverloads internal constructor( +data class Configuration @JvmOverloads constructor( val serverEndpoint: String = DEFAULT_TELEMETRY_ENDPOINT, val channel: String? = null, val maxEvents: Int? = null, diff --git a/glean-core/android/src/test/java/mozilla/telemetry/glean/TestUtil.kt b/glean-core/android/src/test/java/mozilla/telemetry/glean/TestUtil.kt index 64a2d064cc..a1559edae2 100644 --- a/glean-core/android/src/test/java/mozilla/telemetry/glean/TestUtil.kt +++ b/glean-core/android/src/test/java/mozilla/telemetry/glean/TestUtil.kt @@ -126,9 +126,9 @@ internal fun resetGlean( // We're using the WorkManager in a bunch of places, and Glean will crash // in tests without this line. Let's simply put it here. WorkManagerTestInitHelper.initializeTestWorkManager(context) - Glean.resetGlean(context, config, clearStores, uploadEnabled = uploadEnabled) // Always log pings for tests Glean.setLogPings(true) + Glean.resetGlean(context, config, clearStores, uploadEnabled = uploadEnabled) } /** diff --git a/glean-core/ios/Glean/Glean.swift b/glean-core/ios/Glean/Glean.swift index ecce833497..22b24d15f3 100644 --- a/glean-core/ios/Glean/Glean.swift +++ b/glean-core/ios/Glean/Glean.swift @@ -28,7 +28,7 @@ public class Glean { var initialized: Bool = false private var uploadEnabled: Bool = true private var debugViewTag: String? - var logPings: Bool? + var logPings: Bool = false var configuration: Configuration? private var observer: GleanLifecycleObserver? @@ -116,12 +116,12 @@ public class Glean { return } - if self.debugViewTag != nil { - _ = self.setDebugViewTag(self.debugViewTag!) + if let debugViewTag = self.debugViewTag { + self.setDebugViewTag(debugViewTag) } - if self.logPings != nil { - _ = self.setLogPings(self.logPings!) + if self.logPings { + self.setLogPings(self.logPings) } // If any pings were registered before initializing, do so now @@ -461,7 +461,7 @@ public class Glean { /// /// - parameters: /// * value: The value of the tag, which must be a valid HTTP header value. - public func setDebugViewTag(_ value: String) -> Bool { + func setDebugViewTag(_ value: String) -> Bool { if self.isInitialized() { return glean_set_debug_view_tag(value).toBool() } else { @@ -475,7 +475,7 @@ public class Glean { /// /// - parameters: /// * value: The value of the option. - public func setLogPings(_ value: Bool) { + func setLogPings(_ value: Bool) { if self.isInitialized() { glean_set_log_pings(value.toByte()) } else { @@ -586,8 +586,8 @@ public class Glean { // Init Glean. testDestroyGleanHandle() - initialize(uploadEnabled: uploadEnabled, configuration: configuration) // Enable ping logging for all tests setLogPings(true) + initialize(uploadEnabled: uploadEnabled, configuration: configuration) } } diff --git a/glean-core/ios/GleanTests/Debug/GleanDebugUtilityTests.swift b/glean-core/ios/GleanTests/Debug/GleanDebugUtilityTests.swift index 6ceb90a828..228e342aae 100644 --- a/glean-core/ios/GleanTests/Debug/GleanDebugUtilityTests.swift +++ b/glean-core/ios/GleanTests/Debug/GleanDebugUtilityTests.swift @@ -27,7 +27,7 @@ class GleanDebugUtilityTests: XCTestCase { // Test toggle true var url = URL(string: "test://glean?logPings=true") Glean.shared.handleCustomUrl(url: url!) - XCTAssertTrue(Glean.shared.logPings!) + XCTAssertTrue(Glean.shared.logPings) // Test invalid value doesn't cause setting to toggle var previousValue = Glean.shared.logPings @@ -38,7 +38,7 @@ class GleanDebugUtilityTests: XCTestCase { // Test toggle false url = URL(string: "test://glean?logPings=false") Glean.shared.handleCustomUrl(url: url!) - XCTAssertFalse(Glean.shared.logPings!) + XCTAssertFalse(Glean.shared.logPings) // Test invalid value doesn't cause setting to toggle previousValue = Glean.shared.logPings From bf10bf19ec57e4893d15941a51546756982bb144 Mon Sep 17 00:00:00 2001 From: brizental Date: Wed, 15 Jul 2020 11:36:24 +0200 Subject: [PATCH 10/10] Add CHANGELOG.md entry --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index ac67861b12..65cdf1febb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,8 @@ # Unreleased changes +* General + * Enable debugging features through environment variables. ([#1058](https://github.com/mozilla/glean/pull/1058)) + [Full changelog](https://github.com/mozilla/glean/compare/v31.3.0...main) # v31.3.0 (2020-07-10)