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) diff --git a/docs/user/debugging/index.md b/docs/user/debugging/index.md index 25f7798cb2..5ab2955338 100644 --- a/docs/user/debugging/index.md +++ b/docs/user/debugging/index.md @@ -12,12 +12,27 @@ 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. + +> **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 133b897f7a..f63383cb4b 100644 --- a/docs/user/debugging/ios.md +++ b/docs/user/debugging/ios.md @@ -1,3 +1,13 @@ +# Enabling debugging features in iOS through 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). + +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. + # 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. 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: 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..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 @@ -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 = 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 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) { + 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 @@ -614,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 { @@ -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. + */ + internal 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. @@ -664,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 b54c7a6346..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 @@ -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 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..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,6 +126,8 @@ 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) + // Always log pings for tests + Glean.setLogPings(true) Glean.resetGlean(context, config, clearStores, uploadEnabled = uploadEnabled) } 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!!) } 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) 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/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..22b24d15f3 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 = false var configuration: Configuration? private var observer: GleanLifecycleObserver? @@ -115,8 +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 { + self.setLogPings(self.logPings) } // If any pings were registered before initializing, do so now @@ -456,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 { @@ -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. + 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. /// @@ -569,6 +586,8 @@ public class Glean { // Init Glean. testDestroyGleanHandle() + // Enable ping logging for all tests + setLogPings(true) initialize(uploadEnabled: uploadEnabled, configuration: configuration) } } 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/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..228e342aae 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( 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, diff --git a/glean-core/src/debug.rs b/glean-core/src/debug.rs index 4c65858bf5..e866e3525f 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 } @@ -172,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"); } @@ -188,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 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") }