WIP: 1624695: Act on changes in upload enable state outside of application #786

mdboom · 2020-03-26T20:33:05Z

This is trickier than it needs to be because the handling of actions
based on the change in upload state is handled both on the Rust and
language wrapper side.

The solution I arrived at is to no longer pass in an initial value
for upload enabled when initializing the Rust side, but to instead let
Glean determine the upload enable state from the database -- it is
disabled if either there is no client_id or client_id == c0ffee.

Then the language bindings immediately change the upload state based
on the what the user has passed in, allowing the usual state flipping
operations to run as usual.

Even though the FfiConfiguration object loses an uploadEnabled field as
part of this change, that is purely internal API and the public API doesn't
change at all here.

Before merging:

Port new tests to Swift

This is trickier than it needs to be because the handling of actions based on the change in upload state is handled both on the Rust and language wrapper side. The solution I arrived at is to no longer pass in an initial value for upload enabled when initializing the Rust side, but to instead let Glean determine the upload enable state from the database -- it is disabled if either there is no client_id or client_id == c0ffee. Then the language bindings immediately change the upload state based on the what the user has passed in, allowing the usual state flipping operations to run as usual. Even though the FfiConfiguration object loses an uploadEnabled field as part of this change, that is purely internal API and the public API doesn't change at all here.

CHANGELOG.md

Dexterp37 · 2020-03-27T13:59:37Z

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

@@ -261,7 +262,7 @@ open class GleanInternalAPI internal constructor () {
            val originalEnabled = getUploadEnabled()

            @Suppress("EXPERIMENTAL_API_USAGE")
-            Dispatchers.API.launch {
+            Dispatchers.API.executeTask {


Mh, this means that whenever users call setUploadEnabled we immediately flip, rather than enqueuing the call. We should think thorougly about what this implies.

Why are you changing this? What's the specific problem you were seeing?

This needs to happen before any of the preinit tasks run. executeTask merely adds to the "real" queue rather than the preinit queue. After preinit time, it's basically identical to launch.

Dexterp37 · 2020-03-27T14:11:58Z

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

+    @Test
+    fun `no sending of deletion ping if unchanged outside of run`() {
+        val server = getMockWebServer()
+        resetGlean(


Ideally we'd also want to check that we're not mistakenly resetting the client id for extra safety. Unless that's done in some existing test already

It might be, but it's not a bad idea to do it here as well.

Dexterp37 · 2020-03-27T14:12:49Z

glean-core/ios/Glean/Glean.swift

@@ -199,7 +200,7 @@ public class Glean {
        if isInitialized() {
            let originalEnabled = getUploadEnabled()

-            Dispatchers.shared.launchAPI {
+            Dispatchers.shared.launchConcurrent {


Same concern here that I had from the Android version

Dexterp37 · 2020-03-27T14:14:22Z

glean-core/preview/src/configuration.rs

@@ -7,8 +7,6 @@
 /// Optional values will be filled in with default values.
 #[derive(Debug, Clone)]
 pub struct Configuration {
-    /// Whether upload should be enabled.
-    pub upload_enabled: bool,


Are you sure this is actually required, in this case? This is basically an external package/API, we're changing internals in other places.

Dexterp37 · 2020-03-27T14:14:56Z

glean-core/preview/src/lib.rs

 //!     max_events: None,
 //!     delay_ping_lifetime_io: false,
 //!     channel: None,
 //! };
 //! glean_preview::initialize(cfg, ClientInfoMetrics::unknown())?;
+//! glean_preview::set_upload_enabled(true);


I think that glen_preview::initialize, in this case, should take care of calling the right thing internally.

Good point.

Dexterp37 · 2020-03-27T14:16:30Z

glean-core/src/lib.rs

@@ -181,7 +179,7 @@ impl Glean {
        let event_data_store = EventDatabase::new(&cfg.data_path)?;

        let mut glean = Self {
-            upload_enabled: cfg.upload_enabled,
+            upload_enabled: false,


nit: let's add a comment to not scare our future selves out. This should not trigger a wipeout of the stored client id.

Dexterp37 · 2020-03-27T14:34:55Z

glean-core/src/lib.rs

+            .client_id
+            .get_value(&glean, "glean_client_info")
+        {
+            None => true,


What if we keep all the existing code and change this part slightly: let's only find if we have an existing non coffee client id. If so, forcefully trigger the deletion ping here, as done in there. This would require us to deteramine is_first_run a bit earlier.

Moreover, wouldn't the current approach generate a new deletion ping the first time a new profile starts the product with telemetry disabled?

What if we keep all the existing code and change this part slightly: let's only find if we have an existing non coffee client id. If so, forcefully trigger the deletion ping here, as done in there. This would require us to deteramine is_first_run a bit earlier.

I tried that initially, but the problem is that we need to both submit the deletion ping (which happens in Rust) and trigger the deletion ping uploader (from the language bindings).

I have some thoughts about refactoring this further to make the line between the Rust and language binding side less weird here.

Moreover, wouldn't the current approach generate a new deletion ping the first time a new profile starts the product with telemetry disabled?

Yes, that's true. Should work on that case as well, I guess.

Co-Authored-By: Alessio Placitelli <alessio.placitelli@gmail.com>

mdboom · 2020-03-30T22:55:19Z

Found a much simpler approach in #791

auto-assign bot requested a review from Dexterp37 March 26, 2020 20:33

badboy self-assigned this Mar 27, 2020

badboy self-requested a review March 27, 2020 10:57

badboy removed their assignment Mar 27, 2020

mdboom force-pushed the deletion-request-at-init branch from a7e8265 to 2b5ea92 Compare March 27, 2020 12:56

mdboom changed the title ~~1624695: Act on changes in upload enable state outside of application~~ WIP: 1624695: Act on changes in upload enable state outside of application Mar 27, 2020

Dexterp37 requested changes Mar 27, 2020

View reviewed changes

mdboom and others added 2 commits March 27, 2020 10:59

Update CHANGELOG.md

2f87683

Co-Authored-By: Alessio Placitelli <alessio.placitelli@gmail.com>

Fix CI

873a1c4

mdboom mentioned this pull request Mar 30, 2020

WIP: Handle upload enable change at startup using callbacks #790

Closed

mdboom closed this Mar 30, 2020

mdboom deleted the deletion-request-at-init branch April 14, 2020 19:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: 1624695: Act on changes in upload enable state outside of application #786

WIP: 1624695: Act on changes in upload enable state outside of application #786

mdboom commented Mar 26, 2020 •

edited

Loading

Dexterp37 Mar 27, 2020

mdboom Mar 27, 2020

Dexterp37 Mar 27, 2020

mdboom Mar 27, 2020

Dexterp37 Mar 27, 2020

Dexterp37 Mar 27, 2020

Dexterp37 Mar 27, 2020

mdboom Mar 27, 2020

Dexterp37 Mar 27, 2020

Dexterp37 Mar 27, 2020

mdboom Mar 27, 2020

mdboom commented Mar 30, 2020

WIP: 1624695: Act on changes in upload enable state outside of application #786

WIP: 1624695: Act on changes in upload enable state outside of application #786

Conversation

mdboom commented Mar 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdboom commented Mar 30, 2020

mdboom commented Mar 26, 2020 •

edited

Loading