Conversation
`account_logout` only fired when logging out left the user with no account or sites, and carried no timing. Track it from `AccountHelper.logOutDefaultWordPressComAccount()` instead, on every WordPress.com logout, with a `duration_ms` property. The duration is taken on the next main queue turn, so it also covers main thread work that logging out starts but that only runs after the method returns. Removing the last self-hosted site still tracks the event as before.
Collaborator
Generated by 🚫 Danger |
Contributor
|
| App Name | WordPress | |
| Configuration | Release-Alpha | |
| Build Number | 34828 | |
| Version | PR #26118 | |
| Bundle ID | org.wordpress.alpha | |
| Commit | 6cb3272 | |
| Installation URL | 76162fekd3qe0 |
Contributor
|
| App Name | Jetpack | |
| Configuration | Release-Alpha | |
| Build Number | 34828 | |
| Version | PR #26118 | |
| Bundle ID | com.jetpack.alpha | |
| Commit | 6cb3272 | |
| Installation URL | 0mnj8vcjaldn0 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Tracks
account_logouton every WordPress.com logout and adds aduration_msproperty saying how long logout kept the main thread busy. Until now the event only fired when logging out left the user with no account and no sites, and it carried no timing.Nothing reports how long logout takes today: Sentry app-hang tracking and performance tracing are both off in this app. On trunk, logging out freezes the main thread for about 2.5 s when WebKit's cookie store holds at least one WordPress.com cookie, and that went unnoticed until it was measured by hand. #26106 removes that wait. This PR is stacked on it, touches no file it changes, and makes the next such freeze visible.
Changes
logOutDefaultWordPressComAccount()tracksaccount_logoutwithduration_mswhen there was a WordPress.com account to log out of.trackLogoutIfNeeded(), which would now double-count.trackLogoutIfNeeded()stays for its other caller: removing the last self-hosted site still tracksaccount_logout, without a duration.What
duration_msmeasuresThe time from the start of
logOutDefaultWordPressComAccount()until the main queue next runs a block. The event is tracked from aDispatchQueue.main.asyncat the end of the method, not inline.Timing only the method body would have missed the freeze this metric exists to catch: on trunk, the 2 s cookie wait runs in a WebKit completion handler after the method has returned. Measured next to a main-thread stall monitor,
duration_mswas within 28 ms of the stall in all three runs below.The event's meaning changes
account_logoutno longer means "fully signed out". It now fires when a WordPress.com account is logged out even if self-hosted sites remain, so counts will rise. A user who logs out of WordPress.com and later removes their last self-hosted site produces two events, and nothing on the event tells the two cases apart.Replacing the default account during sign-in no longer tracks a logout.
WordPressComSyncServiceremoves an existing default account when a different one signs in. That went through the notification handler and trackedaccount_logoutif no self-hosted sites were left. It is not a user logout and has no duration to report.The event is lost if the app exits before the next main queue turn. That is the cost of measuring past the end of the method.
Test plan
Measured on an iOS 27.0 simulator (Debug build) signed in to an account with 389 sites, using a temporary analytics tracker and main-thread stall monitor that are not part of this PR:
duration_msaccount_logouteventsBefore this PR no event fired in the third scenario. The self-hosted site there was a placeholder
Bloginserted into Core Data, not a site added through the UI.account_logout, withduration_mswithin 28 ms of the stall monitor.duration_msreported the 2 s main-thread wait that Change CookieJar behaviors ahead of the async conversion #26106 removes.🔵 Tracked: account_logout <duration_ms: …>line appears.There is no unit test. Running the real logout in the unit-test host resets process-wide state (the disk cache,
WordPressClientFactory.shared,JetpackSocialFactory.shared) in a host shared with other suites.