Change 362429 had a related patch set uploaded (by Pmiazga; owner: Pmiazga):
[mediawiki/extensions/Popups@master] Send disabled event from settings windows

https://gerrit.wikimedia.org/r/362429

JS part is done, PHP part requires creating a patch in Core.

Currently, there are three events

• documented one - UserSaveOptions and UserSaveSettings
• undocumented - PreferencesFormPreSave

But all are fired after applying new options to a user. It means there is no possibility to detect switch Page previews options on the User:Preferences page.

As I mentioned above there is no possibility to track changes in user preferences form. I created a T169365 and it has to be solved first.

The only thing that comes to my mind is a huge hack. First, we can listen to GetPreferences, store it in a temporary variable, then on UserSaveOptions and compare new option value to the stored one but IMHO it's pretty big hack and it may introduce data inconsistency.

Change 362557 had a related patch set uploaded (by Pmiazga; owner: Pmiazga):
[mediawiki/extensions/Popups@master] POC: Hack user preferences and trigger event on user prefs save

https://gerrit.wikimedia.org/r/362557

@phuedx could you check the POC patch?

@pmiazga: On it!

Done. I've reviewed rEPOP3a42a3c5faec: POC: Hack user preferences and trigger event on user prefs save as if it were the solution that we were going to adopt. However, I think that, since there's no train running this week, we have ample time to tackle the problem properly, i.e. bring T169365: There is no possibility to check changes on user options save. into the sprint and fix it.

What do you think? /cc @ovasileva

In T167365#3403969, @phuedx wrote:

Done. I've reviewed rEPOP3a42a3c5faec: POC: Hack user preferences and trigger event on user prefs save as if it were the solution that we were going to adopt. However, I think that, since there's no train running this week, we have ample time to tackle the problem properly, i.e. bring T169365: There is no possibility to check changes on user options save. into the sprint and fix it.

What do you think? /cc @ovasileva

I think this makes sense

Quoting myself from my review of rEPOP7e2c79ae0db7: Send disabled event from settings windows:

Symptom:
When I disable page previews via the settings menu, re-enable it, then disable it again, I don't see a second "disabled" event.

Cause:
The second disabled event is considered a duplicate event by the de-duplicator.

Notes:
This is an edge-case and I'm not convinced that we should take it into account. Regardless, I see two ways of moving forward:

1. Remove the isDuplicateEvent code from the de-duplicator – there haven't been duplicate events for weeks now. OTOH having visibility into this would still be nice to have.
2. Special-case the disabled event in the de-duplicator. This ain't pretty but we do have form for this already as we've special-cased events without link interaction tokens in isDuplicateToken.

FTR we decided to go with option #1 with the constraint that the instrumentation should remain.

Change 363377 had a related patch set uploaded (by Pmiazga; owner: Pmiazga):
[mediawiki/extensions/Popups@master] Remove duplicate events filtering

https://gerrit.wikimedia.org/r/363377

Change 362557 abandoned by Phuedx:
POC: Hack user preferences and trigger event on user prefs save

Reason:
Abandoning this in favour of using the newly added parameter to the PreferencesFormPreSave hook.

https://gerrit.wikimedia.org/r/362557

Change 362429 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] Send disabled event from settings windows

https://gerrit.wikimedia.org/r/362429

Change 363377 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] Remove duplicate events filtering

https://gerrit.wikimedia.org/r/363377

Change 363642 had a related patch set uploaded (by Pmiazga; owner: Pmiazga):
[mediawiki/extensions/Popups@master] Send disabled event when user disables Page Preview

https://gerrit.wikimedia.org/r/363642

@Tbayer - https://meta.wikimedia.org/wiki/Schema:Popups specifies set of required event properties, most of those properties are available only in frontend (js). Logged-in user can change Page Previews settings only on Special:Preferences page, in that scenario the PHP server is responsible for triggering the event, not the browser. Because of that we do not have access to following properties:

• pageTitleSource (user is on Special:Preferences page)
• namespaceIdSource (same as above)
• pageIdSource (same as above)
• previewCountBucket
• pageToken (we can generate a random hash)
• sessionToken (available only in local storage in the browser)

What should we do in that case? It's pretty difficult to pass those variables to PHP server, without bigger changes in MWCore I would say it's not possible - @phuedx - correct me if I'm wrong.

If Nav_Popups gadget is enabled there is no way to enable Page Previews, because of that hovercardsSuppressedByGadget would be always false.

The following values seem sensible:

$pageTitleSource = SpecialPageFactory::getLocalNameFor( 'Preferences' ); // pageTitleSource should be the localized name of Special:Preferences, right?
$namespaceIdSource = -1;
$pageIdSource = -1;
$hovercardsSuppressedByGadget = false;
$pageToken = getRandomHash();

You're right about the sessionToken property, however.

One option would be to pass the session token as part of the URL, e.g. ?pagePreviewsSessionToken=X and grabbing it in the PreferencesFormPreSave hook. This idea is shamelessly stolen from Discovery, who add a searchSessionToken (or something similar, anyway) parameter to the URL when a search session starts.

In T167365#3414931, @phuedx wrote:

One option would be to pass the session token as part of the URL, e.g. ?pagePreviewsSessionToken=X and grabbing it in the PreferencesFormPreSave hook. This idea is shamelessly stolen from Discovery, who add a searchSessionToken (or something similar, anyway) parameter to the URL when a search session starts.

This is easier said than done. There's no hook that runs immediately before the PreferenceForm object is constructed, so there's no clean – relatively speaking! – way to add information to the form for later processing. I guess that if we absolutely require the sessionToken property, then there's always the session… i.e. if the user is on Special:Preferences and there's a pagePreviewsSessionToken query parameter, then persist it in the session; if we're saving the user's preferences, then grab the value out of the session. Bleh.

@Tbayer, @phuedx - perhaps we can make this a bit easier and restrict to anons only?

In T167365#3414984, @ovasileva wrote:

@Tbayer, @phuedx - perhaps we can make this a bit easier and restrict to anons only?

How would that affect our ability to test our most important hypothesis?

IMO, a bit but not significantly. We're using disabling as a sign that users like the feature and continue using it. Also, we're using it as a sign that the feature is not annoying and getting in the way. Given that logged in users have to make the conscious choice to turn the feature on and anons will be receiving it by default, I would say that measuring disabled for logged-in users is very valuable, but measuring disabled for anons is crucial.

In T167365#3415108, @ovasileva wrote:

IMO, a bit but not significantly. We're using disabling as a sign that users like the feature and continue using it. Also, we're using it as a sign that the feature is not annoying and getting in the way. Given that logged in users have to make the conscious choice to turn the feature on and anons will be receiving it by default, I would say that measuring disabled for logged-in users is very valuable, but measuring disabled for anons is crucial.

Agreed, but have we already made the decision to permanently keep the feature disabled by default for logged-in users?

Regarding the field values, @phuedx's suggestion in T167365#3414931 makes sense to me. (Actually I wouldn't mind recording "Special:Preferences" untranslated, because it would be easier to query across languages.) Not having previewCountBucket sucks, because we wanted to understand if people who deactivate the feature do so right after seeing it for the first time or after using it for a while. sessionToken would also be good to have, in particular for vetting/debugging purposes. But it sounds that these two would be too complicated.

@phuedx there is no possibility to grab stuff from Preferences form submit as <form /> always submits to /wiki/Special:Preferences - it doesn't keep $_GET params in the form action. If we pass another token it will be available only on form view, not form save.

@Tbayer - I'll do bit more research but both previewCountBucket and sessionToken stays in local storage in the browser and currently they are not available in the backend.

In T167365#3415734, @pmiazga wrote:

@phuedx there is no possibility to grab stuff from Preferences form submit as <form /> always submits to /wiki/Special:Preferences - it doesn't keep $_GET params in the form action. If we pass another token it will be available only on form view, not form save.

See T167365#3414960. I backtracked on manipulating the PreferencesForm object but there's still the possibility of setting data in the session for the duration of the Special:Preferences pageview. I'm not happy with that solution though!

Agreed, but have we already made the decision to permanently keep the feature disabled by default for logged-in users?

yup. It will be automatically enabled for the users who have it enabled in beta, but that's a fairly small number, not a priority for analysis imo

Change 364455 had a related patch set uploaded (by Pmiazga; owner: Pmiazga):
[mediawiki/extensions/Popups@master] POC: Send disabled event from JS after Preferences save

https://gerrit.wikimedia.org/r/364455

To be reviewed:

• rEPOP64ee22fbc21d: Send disabled event when user disables Page Preview
• rEPOP1dbb88a27851: POC: Send disabled event from JS after Preferences save

I've mentioned to @pmiazga and I just want to document it here:

To date, we've been blocked on analysing the Page Previews EventLogging data due to a serious issue with duplication of EL events in Firefox. We're now close to eliminating that blocker. disabled events are the next blocker (that we know of!). With that in mind, merging the server-side instrumentation in rEPOP64ee22fbc21d: Send disabled event when user disables Page Preview should be the priority. Tackling the client-side approach should be done later. We'll also have time to tidy up a part of the Page Previews codebase that makes rEPOP1dbb88a27851: POC: Send disabled event from JS after Preferences save complicated –the service definition in index.js. I'll write a follow-on task ASAP /cc @ovasileva

Change 363642 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] Send disabled event when user disables Page Preview

https://gerrit.wikimedia.org/r/363642

Following up on T167365#3431528, instrumentation can be tested end-to-end on the Beta Cluster! See https://wikitech.wikimedia.org/wiki/Analytics/Systems/EventLogging/TestingOnBetaCluster

To verify in production, unless I'm mistaken we'd need 1000 people to disable the beta feature. Given we only have 69,475 users on English Wikipedia we'd need 1.5% of those users to turn it off to verify. This event is going to be low traffic (if we do our job correctly) and unless it has special treatment so even with these changes implemented we'd not likely to see many.

@pmiazga correct me if I'm wrong but the server side bucketing uses wfRandom to bucket so we're not logging consistently with client side event logging by user session. It's worth pointing out that it won't be possible to link events in the client with server.

I'm not quite sure what the original intent of this task was here. I'm sorry I wasn't more involved in this from the start but my feeling is that if we care about disable rates a separate schema would be better here - not piggy backing off the existing one.

What is the end goal here? A rough idea of how many people disable or are we trying to capture the user's journey e.g. clicks setting cog, goes to preferences, disables ?

cc @phuedx @Tbayer @ovasileva

In T167365#3438016, @Jdlrobson wrote:

...

I'm not quite sure what the original intent of this task was here.

The intent was to have the code send the events that had been specified in the schema 15 months ago and had already been implemented in the 2016 version of the code back then, but then were apparently forgotten in the rewrite.

What is the end goal here? A rough idea of how many people disable or are we trying to capture the user's journey e.g. clicks setting cog, goes to preferences, disables ?

The former, as mentioned many times previously, e.g. in last year's notes that gave rise to the instrumentation back then, or in the later report on the results from the previous instrumentation, or the overview Google doc that @ovasileva sent around some weeks ago.
Regarding the user's journey, we had already decided above that we can do without logging the settings cog click in case the user disables later.

As I indicated above, the data from logged-in users is mostly import for the case that we eventually decide to roll the feature to everyone by default; I think this was the assumption originally.

To verify in production, unless I'm mistaken we'd need 1000 people to disable the beta feature.

Or we can just bucket ourselves and verify directly that the event is logged. But I think what you're getting at is that the current, decreased sampling rate is quite low for smaller user groups like this one (logged-in users on enwiki). That's a good point, it however just means that the statistical uncertainties grow (technically: the confidence interval for the estimated percentage of disabling sessions). Even if won't have enough data to determine whether the true rate was, say, 0% or 1%, it can still be useful to be able to say (with sufficient statistical confidence) that it was <30%. That said, a separate schema with a much higher sampling rate would give better data, but I don't know how important the logged-in users case is at this point. One reason to use the existing schema is that it already logs preview count and edit count, two data pieces that are relevant for disable events.

@Jdlrobson - after we agreed to do EventLogging in the PHP (first time we did sth like that) and I started working on it I found out it is not possible to collect and send all user-data as this data is stored in the browser. With @Tbayer approval (https://phabricator.wikimedia.org/T167365#3415376) we decided to go with what we already have - send event without required user data (like userSessionToken or previewCountBucket) as having a logged event is more important than having disabled event with all data later.

Now, once we have disabled event and we can do analytics we will improve current solution and somehow pass userSessionToken and pagePreviewsCount. The requirement was to send the event ASAP, even with incoplete data ( random hash for userSessionToken and undefined for pagePreviewsCount).

When the backend part was ready (PHP sends events to EventLogging) with @phuedx we tried to find a better solution and we came with two possible solutions

1. keep the logging in the PHP, when user visits Special:Preferences page - inject required data to form a <input type="hidden"... and on PreferencesFormPreSave read those values
   • probably we would need to pass those values only when we want to log the event, with that approach we could still use user bucketing
   • it requires
2. create a small JS Module that listens to PagePreviews settings change (whats stored in LocalStorage vs what server sends to a user) and when oldState == enabled && newState == disabled use the existing. see https://phabricator.wikimedia.org/T167365#3426316

I did a Proof-of-concept for point 2 - T170575 -- gerrit patch: https://gerrit.wikimedia.org/r/#/c/364455/ - instead of logging events by PHP, always use JS.

Summary:

• we decided to log events without having a link between events sent from server and events sent from browser
• we're going to improve current solution

Testing

It is possible to easily test the JS events. In T168847 we implemented a mechanism, when ?debug=true is passed event logging instrumentation will skip bucketing and always log every event.
Currently, there is no possibility to easily test disabled event sent by PreferencesForm as it's done on the backend and there is no way to bucket yourself. We couldn't use the ?debug=true trick as Preferences form always submit to /wiki/Special:Preferences page - it ommits all $_GET parameters

Thanks @pmiazga for catching me up! So yes, it seems we've got what we need but we're not going to be able to sign this off easily since we can't bucket ourselves for this event. Per https://phabricator.wikimedia.org/T167365#3431528 we'll need to wait till Wednesday but it may be near impossible to sign off the preferences steps.. but we'll see!

In T167365#3439344, @Jdlrobson wrote:

Per T167365#3431528 we'll need to wait till Wednesday but it may be near impossible to sign off the preferences steps.. but we'll see!

How about using the staging server – where we can tweak wgPopupsSchemaSamplingRate at will – for this?

I guess we could set it to 1 on staging. Good call. @phuedx can you deploy a change to do that today or Monday? Labs config deploys can happen any time.

FTR this is done. I deployed rOMWCe170b8109139: Log all events for page previews in beta cluster to the Beta Cluster a couple of minutes ago

I did the following in this order:

• Visited https://en.wikipedia.beta.wmflabs.org/wiki/Dog
• Disabled the beta feature
• Visited https://en.wikipedia.beta.wmflabs.org/wiki/Special:Preferences
• Enabled the beta feature

I tried to sign this off and verify the events show up but I seem to have lost access to mysql on deployment-eventlogging03.eqiad.wmflabs.. will see if I can get that today.

I cannot test whether events are logged for the user preference when it's not enabled as a beta feature until we do a config change to turn page previews on for all page views.

@phuedx can you deploy that labs change when you get a chance so we can sign off the rest?

Change 365696 had a related patch set uploaded (by Phuedx; owner: Jdlrobson):
[operations/mediawiki-config@master] Enable page previews for everyone on labs

https://gerrit.wikimedia.org/r/365696

As a logged in user when I disabled the Page Previews beta feature I see the following event in the log:

{
    "event": {
        "action": "disabled",
        "hovercardsSuppressedByGadget": false,
        "isAnon": false,
        "namespaceIdSource": -1,
        "pageIdSource": -1,
        "pageTitleSource": "Special:Preferences",
        "pageToken": "cad0165407bbf461f8d2870164fe2eda",
        "popupEnabled": false,
        "previewCountBucket": "unknown",
        "sessionToken": "a3dbb6739368c02517dff2f8c51aecf0"
    },
    "recvFrom": "deployment-cache-text04.deployment-prep.eqiad.wmflabs",
    "revision": 16364296,
    "schema": "Popups",
    "seqId": 6164475,
    "timestamp": 1500370808,
    "userAgent": "{\"os_minor\": \"12\", \"is_bot\": false, \"os_major\": \"10\", \"device_family\": \"Other\", \"os_family\": \"Mac OS X\", \"browser_minor\": \"0\", \"wmf_app_version\": \"-\", \"browser_major\": \"59\", \"browser_family\": \"Chrome\", \"is_mediawiki\": false}",
    "uuid": "7f3ce40a4d9153cfa18b412b77f366a3",
    "webHost": "en.wikipedia.beta.wmflabs.org",
    "wiki": "enwiki"
}

AFAIK the event only appears in the log if it has been validated against the schema.

Change 365696 merged by jenkins-bot:
[operations/mediawiki-config@master] Enable page previews for everyone on labs

https://gerrit.wikimedia.org/r/365696

rOMWCf7ddfc88720f: Enable page previews for everyone on labs has been deployed to the Beta Cluster and I've updated the deployment host.

Change 365939 had a related patch set uploaded (by Phuedx; owner: Phuedx):
[operations/mediawiki-config@master] pagePreviews: Re-enable Popups extension on Beta Cluster

https://gerrit.wikimedia.org/r/365939

I had to revert the previous change as it disabled Page Previews for everyone. I'll update and deploy my change in a little while…

Edit

Page Previews were disabled for everyone on the Beta Cluster.

Can someone review/sanity check https://gerrit.wikimedia.org/r/#/c/365939/?

@phuedx - looks good, I added +1 only because I have no permission to add +2. IMHO you can self-merge.

Change 365939 merged by jenkins-bot:
[operations/mediawiki-config@master] pagePreviews: Enable for anons/as pref on Beta Cluster

https://gerrit.wikimedia.org/r/365939

With rOMWC4ec5aaa84c97: pagePreviews: Enable for anons/as pref on Beta Cluster deployed, I saw the following event when I disabled Page Previews via the Appearance tab on Special:Preferences:

{
    "event": {
        "action": "disabled",
        "hovercardsSuppressedByGadget": false,
        "isAnon": false,
        "namespaceIdSource": -1,
        "pageIdSource": -1,
        "pageTitleSource": "Special:Preferences",
        "pageToken": "581c6b38e8c81e3c77c19ac9ecf0b099",
        "popupEnabled": false,
        "previewCountBucket": "unknown",
        "sessionToken": "48f414535fba8e7235ca7ed10a63941c"
    },
    "recvFrom": "deployment-cache-text04.deployment-prep.eqiad.wmflabs",
    "revision": 16364296,
    "schema": "Popups",
    "seqId": 6367526,
    "timestamp": 1500472280,
    "userAgent": "{\"os_minor\": \"12\", \"is_bot\": false, \"os_major\": \"10\", \"device_family\": \"Other\", \"os_family\": \"Mac OS X\", \"browser_minor\": \"0\", \"wmf_app_version\": \"-\", \"browser_major\": \"59\", \"browser_family\": \"Chrome\", \"is_mediawiki\": false}",
    "uuid": "fd18f5ddca9251819d3cf193b1f7dca8",
    "webHost": "en.wikipedia.beta.wmflabs.org",
    "wiki": "enwiki"
}

Looks like we can safely sign this off...?

Following on from T167365#3447318 and T167365#3452499, I saw the following event when I disabled Page Preview via the settings menu (i.e. as an anon):

{
    "event": {
        "action": "disabled",
        "hovercardsSuppressedByGadget": false,
        "isAnon": true,
        "namespaceIdSource": 0,
        "pageIdSource": 1457,
        "pageTitleSource": "Dog",
        "pageToken": "d47e511e68c5d4df",
        "popupEnabled": true,
        "previewCountBucket": "1-4 previews",
        "sessionToken": "f14686dbbc7d40f9"
    },
    "recvFrom": "deployment-cache-text04.deployment-prep.eqiad.wmflabs",
    "revision": 16364296,
    "schema": "Popups",
    "seqId": 6386159,
    "timestamp": 1500481226,
    "userAgent": "{\"os_minor\": \"12\", \"is_bot\": false, \"os_major\": \"10\", \"device_family\": \"Other\", \"os_family\": \"Mac OS X\", \"browser_minor\": \"0\", \"wmf_app_version\": \"-\", \"browser_major\": \"59\", \"browser_family\": \"Chrome\", \"is_mediawiki\": false}",
    "uuid": "c22cc9c900765d1bbe759a46c0186399",
    "webHost": "en.wikipedia.beta.wmflabs.org",
    "wiki": "enwiki"
}

Change 366287 had a related patch set uploaded (by Phuedx; owner: Phuedx):
[mediawiki/extensions/Popups@master] i13n: popupEnabled = false for disabled event

https://gerrit.wikimedia.org/r/366287

I would note that the client is sending the disabled event with the popupEnabled property set to true whereas the server is sending the event with the property set to false. The client is reflecting the enabled state when the page loads and not what the user has just chosen. Is this acceptable @Tbayer? If not, then I'll move this back to Needs Code Review so that rEPOPbfcc3a490cda: i13n: popupEnabled = false for disabled event can be reviewed.

I'll follow up with @Tbayer during today's Page Previews sync meeting.

@Tbayer and I agreed rEPOPbfcc3a490cda: i13n: popupEnabled = false for disabled event should probably be reviewed to avoid future confusion.

Change 366287 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] i13n: popupEnabled = false for disabled event

https://gerrit.wikimedia.org/r/366287

I've added a note in the schema definition itself about the discrepancy between popupEnabled for disabled events sent for logged in/out users.

Change 364455 abandoned by Jhernandez:
POC: Send disabled event from JS after Preferences save

Reason:
Untouched and out of date for many months. Feel free to reopen if necessary

https://gerrit.wikimedia.org/r/364455