-
Notifications
You must be signed in to change notification settings - Fork 123
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bug 1649926 - Always enqueue an async task to change upload and deprecate getUploadEnabled
#1046
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say this approach looks good to me (provided the iOS tests pass, which I assume they do). The one problem I see is that Lockwise-iOS does rely on getUploadEnabled in a weird way, that can be fixed if they ever update Glean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens in the following order of events?
Glean.setUploadEnabled(false)
Glean.initialize(uploadEnabled=true)
I would expect the state to be upload is enabled, but since the first call would be deferred until after the second, that might not happen (we should confirm with a test -- I could be wrong).
glean-core/android/src/main/java/mozilla/telemetry/glean/Glean.kt
Outdated
Show resolved
Hide resolved
// data after the upload has been disabled. | ||
PingUploadWorker.cancel(applicationContext) | ||
} | ||
// Changing upload enabled always happens asynchronous. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if it's important to call out or not somewhere, but consider this scenario:
Initialize(uploadEnabled: true)
is calledsomePing.submit()
is called right afterSetUploadEnabled(false)
is called.
The ping at (2) might still get through, and I believe it's fine-ish. What do you think @badboy ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's the behavior I would expect. From a user perspective uploadEnabled
was set to false only after the ping was submitted.
However ... (gosh this is complicated) ... because of the delay of the uploader picking it up it could still get cancelled :/
glean-core/android/src/main/java/mozilla/telemetry/glean/Glean.kt
Outdated
Show resolved
Hide resolved
Damn, you are probably right. This is the one case why we had to preserve the state in an instance variable. |
Ok, following up on what Mike said. The positive first: It's easy to write a test! I propose we add this to the top of
|
I spoke too soon. We can't differentiate between
and
right now. Do we really need more state? -_- |
So yeah, if we track that wdyt? |
I support this approach and like it. My only recommendation would be to have a more comprehensive message or add a link to our docs that explain the initial state should be provided with |
As I won't get this done today here's what's left:
|
b89154e
to
4a36a33
Compare
628276b
to
8f670ce
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r+ with a few optional nits! Great work on this one, thanks for your efforts. Please DO NOT MERGE just yet. I'd love to cut a release before this gets merged :)
glean-core/android/src/main/java/mozilla/telemetry/glean/Glean.kt
Outdated
Show resolved
Hide resolved
e9e7154
to
ba80541
Compare
Waiting with the rebase, given that there's a release incoming and thus I need to change the changelog anyway. |
ba80541
to
ca7d46f
Compare
That way it follows what a user expect when calling it inbetween other calls: It executes in the right order. Because the dispatch queue is halted until Glean is fully initialized we can safely enqueue here and it will execute after initialization.
Due to Glean's asynchronous initialization the return value can be incorrect. Applications should not rely on Glean's internal state. Upload enabled status should be tracked by the application and communicated to Glean if it changes. Note: The method was removed from the C# implementation.
After we destroy the Glean handle we should be conceptually in a clean state, just as if the app is just starting and loading Glean. And for Glean that means tasks should get queued up. Some tests explicitly toggle this already, but resetting it here does not seem to affect them.
Co-authored-by: Alessio Placitelli <alessio.placitelli@gmail.com>
ca7d46f
to
2717a62
Compare
Replicating the commit messages for information:
and
This probably deserves some extra eyes on it. Putting you 3 on it, but maybe one or 2 should be enough.
I'm now reasonably sure it does what we expect it to do and the tests capture that.
However the tests are a bit brittle and (at least in some cases) might pass just fine on the old code base too.
Getting rid of
getUploadEnabled
means we also don't need to additionally track that upload enabled flag anymore.Unfortunately I can't write a C# test because we don't have all the necessary utilities in place yet.
I did verify that adding
GleanInstance.SetUploadEnabled(false);
in the sample app just afterInitialize
does NOT send the custom ping, but DOES send the deletion-request ping.