I completed QA of the events logged on testwiki and confirmed that events appear to be firing as expected based on the instrumentation changes and AB test set-up. Please see summary of checks and initial findings below and report for further details.

@ovasileva - Let me know if you have any questions or further checks you'd like to look into before deployment to the pilot wikis.

Summary of checks and findings:

(Results below reflect data logged on testwiki and test2wiki in SearchSatisfaction from 17 February 2021 through 22 February 2021)

• There have been 6 new search sessions initiated (21 search events) using the new search widget event (as indicated by event.extraParams = 'WVUI') logged to date on the testwiki.
• The new search widget sessions and events are only logged for the new search location (event.inputLocation = 'header-moved) . We are also logging some old search widget sessions (as indicated by event.extraParams = NULL) and events from the new search location as expected.
• Confirmed that new search widget events are only logged for the latest vector skin.

Distinct Search Sessions and Events Initiated by Search Location and Search Type (includes both logged in and logged out users)

• The new search widget events are recorded for autocomplete search sessions initiated as expected. There were also 2 click events that were logged as coming from the new search widget.
• New search widget events have been recorded for both logged in (1 session) and logged out (5 sessions) users.
• On the latest vector version, logged-out users only see both the new search location and the new search widget as expected. Logged-in users see either the new search location and new search widget or the new search location and the old search widget depending on the bucket they are assigned to.

Latest Vector Distinct Search Sessions and Events Initiated by Search Widget Type and Logged-In Status

It's difficult to determine if the buckets are balanced using the data currently available in eventlogging. I will plan to check soon after deployment to the pilot wikis to confirm during post-deployment Data QA.

Next Up:

• Check editcount bucket data in eventlogging following planned deployment tomorrow 23 February 2021
• Post deployment Data QA following deployment to early adopter wikis

I checked data logged in SearchSatisfaction and confirmed that we are now receiving events for Group0 wikis for the new event.usereditcount field following the fix deployed on 24 Feb 2021.

All events for this field appear to be logging as expected.

See a quick summary of checks and findings below:

• We have logged search sessions and events with edit bucket info for mediawiki (121 search sessions) and testwikidata (1 session) so far.
• All logged out users are recorded as having 0 edits as expected.
• We have events logged for all 5 edit buckts types and the number of events per edit bucket seem reasonable.
• The edit bucket field is recorded for both vector versions and both search location types (header-moved, header-navigation)

Search Sessions and Events with User Edit Bucket Data Record (Since deployment on 24 Feb 2021)

Updated QA Notebook

@phuedx @ovasileva

I ran a check of the data logged following the deployment of the AB test to all of the pilot wikis.

Event fields are populating as expected; however, the difference in search sessions initiated between each AB group is much higher than would be expected with a 50/50 split across logged-in users. See summary of AB search session data below. Specifically, it looks like many of the new search widget sessions are not recorded or not recorded as expected. Any ideas into why this might be happening?

Data below reflects unique search sessions initiated by logged in users in each search group (new search widget sessions [inputLocation = 'header-moved', extraParams = 'WVUI'; old search widget sessions [inputLocation = 'header-moved', extraParams = NULL] following the AB test deployment date.

Stage 2 Deployment Pilot Wiki:

Stage 3 Deployment Pilot Wikis

Stage 4 Deployment Pilot Wikis

Daily search sessions for the old search widget decreased by half around the deployment dates, which is expected since 50% of users would now receive the new search; however, we're not seeing a proportionate increase in sessions using the new search widget around the same time.

Data Via:

SELECT
    COUNT( DISTINCT event.searchSessionId) AS search_session,
    COUNT(*) AS events,
    event.inputLocation AS search_location,
    event.extraParams AS search_type,
    wiki AS wiki
FROM event.searchSatisfaction 
    WHERE year = 2021
    AND month = 03
-- vary day based on deployment date
    AND day >= 01
    AND useragent.is_bot = false 
    -- deployed on modern vector
    AND event.skin = 'vector'
    AND event.skinVersion = 'latest'
    -- only deployed to logged-in users
    AND event.isAnon = FALSE
    -- action and source recoded when search session started
    AND event.action = 'searchResultPage'
    AND event.source = 'autocomplete'
    AND event.inputLocation = 'header-moved'
    -- pilot wikis
    AND wiki IN ('frwiktionary', 'hewiki', 'ptwikiversity', 'frwiki', 
    'euwiki', 'fawiki', 'ptwiki', 'kowiki', 'trwiki', 'srwiki', 'bnwiki', 'dewikivoyage', 'vecwiki' )
GROUP BY 
    event.inputLocation,
    event.extraParams,
    wiki"

Change 670459 had a related patch set uploaded (by Phuedx; owner: Phuedx):
[mediawiki/extensions/WikimediaEvents@master] searchSatisfaction: Allow for async initialisation

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

Change 670459 merged by jenkins-bot:
[mediawiki/extensions/WikimediaEvents@master] searchSatisfaction: Allow for async initialisation

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

Change 670365 had a related patch set uploaded (by Phuedx; owner: Phuedx):
[mediawiki/extensions/WikimediaEvents@wmf/1.36.0-wmf.33] searchSatisfaction: Allow for async initialisation

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

Change 670526 had a related patch set uploaded (by Phuedx; owner: Phuedx):
[mediawiki/extensions/WikimediaEvents@wmf/1.36.0-wmf.34] searchSatisfaction: Allow for async initialisation

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

Change 670365 merged by jenkins-bot:
[mediawiki/extensions/WikimediaEvents@wmf/1.36.0-wmf.33] searchSatisfaction: Allow for async initialisation

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

Change 670526 merged by jenkins-bot:
[mediawiki/extensions/WikimediaEvents@wmf/1.36.0-wmf.34] searchSatisfaction: Allow for async initialisation

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

Mentioned in SAL (#wikimedia-operations) [2021-03-10T19:16:52Z] <urbanecm@deploy1002> Synchronized php-1.36.0-wmf.33/extensions/WikimediaEvents/modules/ext.wikimediaEvents/searchSatisfaction.js: d9bad12cdb02e13517cecd1775162fde88af48eb: searchSatisfaction: Allow for async initialisation (T274869) (duration: 01m 08s)

Mentioned in SAL (#wikimedia-operations) [2021-03-10T19:18:45Z] <urbanecm@deploy1002> Synchronized php-1.36.0-wmf.34/extensions/WikimediaEvents/modules/ext.wikimediaEvents/searchSatisfaction.js: e998086f7cf7839d2c9aa917776509b3198c3142: searchSatisfaction: Allow for async initialisation (T274869) (duration: 01m 08s)

In T274869#6902041, @gerritbot wrote:

Change 670526 merged by jenkins-bot:
[mediawiki/extensions/WikimediaEvents@wmf/1.36.0-wmf.34] searchSatisfaction: Allow for async initialisation

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

This seems problematic for performance as this installs a global event listener, demanding synchronous eventloop syncing and interruption for almost all input events. This would negatively affect input-heavy applications like VisualEditor, as well as more generally when typing or entering text anywhere in anything. Basically blocks the UI thread until the callback can run, and the jQuery callback needs to run a Selector on the target node before being able to discard it.

This should imho be reverted and replaced with something much more limited. Two ideas come to mind:

1. Go back to the previous approach, but mitigate the issue with an mw.hook() for something like 'skin.search', which Vector can then fire for you once it is ready.
2. Keep the "delegate" apprach from this patch, but bind it much lower down in the document tree. E.g. bound to '#p-search' instead of document.body.

FYI - I rechecked the Search Vue.JS AB test events and the search sessions initiated look much more balanced between the two test groups. Here are the search sessions initiated for each wiki and search group recorded since the deployment of the fix.

In T274869#6902338, @Krinkle wrote:

In T274869#6902041, @gerritbot wrote:

Change 670526 merged by jenkins-bot:
[mediawiki/extensions/WikimediaEvents@wmf/1.36.0-wmf.34] searchSatisfaction: Allow for async initialisation

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

This seems problematic for performance as this installs a global event listener, demanding synchronous eventloop syncing and interruption for almost all input events.

… once as jQuery.one should unbind the event listener after its first invocation. However: this is the case for each individually-bound event listener; and the cost is still unnecessary.

This should imho be reverted and replaced with something much more limited. Two ideas come to mind:

1. Go back to the previous approach, but mitigate the issue with an mw.hook() for something like 'skin.search', which Vector can then fire for you once it is ready.
2. Keep the "delegate" apprach from this patch, but bind it much lower down in the document tree. E.g. bound to '#p-search' instead of document.body.

Reverting wouldn't be acceptable as it would stop the SearchSatisfaction instrument from working with the WVUI search autocomplete widget. I'm happy to submit a follow-on change and backport it. My preference is #2. I'd considered #1 for the original patch but I landed on a version of #2 as it doesn't require introducing any new APIs/protocols to bind to.

Change 670834 had a related patch set uploaded (by Phuedx; owner: Phuedx):
[mediawiki/extensions/WikimediaEvents@master] searchSatisfaction: Don't bind high-level delegated event handler

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

In T274869#6904051, @phuedx wrote:

In T274869#6902338, @Krinkle wrote:

In T274869#6902041, @gerritbot wrote:

[mediawiki/extensions/WikimediaEvents@wmf/1.36.0-wmf.34] searchSatisfaction: Allow for async initialisation
https://gerrit.wikimedia.org/r/670526

This seems problematic for performance as this installs a global event listener, demanding synchronous eventloop syncing and interruption for almost all input events.

… once as jQuery.one should unbind the event listener after its first invocation. However: this is the case for each individually-bound event listener; and the cost is still unnecessary. […]

Thanks, I did miss that!

Change 670834 merged by jenkins-bot:
[mediawiki/extensions/WikimediaEvents@master] searchSatisfaction: Don't bind high-level delegated event handler

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

AB Test Events

I ran another check of the AB test events today (15-March-2021) and the number of search sessions initiated still appear as expected based on a 50/50 user split following the deployment of the fix and all events are logged as expected. Everything looks good! We'll need to run the AB test through 25 March 2021 for two weeks of complete data.

Non AB Test Events
I noticed that from March 2nd to today, we have not recorded any Vue.Js search sessions initiated by logged-in users on wikis not included in the AB test. Vue.Js search events on non-pilot wikis were only recorded from Feb 25th through March 2nd. @phuedx @ovasileva Is that expected?

@phuedx @ovasileva
While collecting data needed for the AB analysis, I noticed a large imbalance again between new and old search widget sessions.

Based on a review of the daily search sessions initiated, it looks like this is due to a regression in SearchSatisfaction that occured around March 18th 2021. We have complete data from the date the 670526 fix was deployed on March 10th through March 17th and then we stop logging a large number of new search widget events. See chart and data below:

Total AB Test Search Sessions Initiated by Date and Search Type

The issue that @MNeisler documented above must be fixed moving forward. It should be considered a regression in the SearchSatisfaction instrument, though it might not need to be fixed there.

The patch that introduced the regression is rEWMV7cacd87d8f1f: searchSatisfaction: Don't bind high-level delegated event handler. The regression itself is due to Vue.js's behaviour of destroying the element it mounts on (see https://vuejs.org/v2/api/#el), which necessarily removes any event listeners already added to that element. Thus, when Vector's App component mounts on the #p-search element, it removes the delegated event listener bound to that element by the SearchSatisfaction instrument.

It's worth noting that the above patch was deployed the week after rEWMV550918e2a179: searchSatisfaction: Allow for async initialisation, which explains our week of data following by none whatsoever.

Now, there are a handful of ways that I would go about fixing this:

1. Revert rEWMV7cacd87d8f1f: searchSatisfaction: Don't bind high-level delegated event handler.
   • It's a more flexible solution, allowing for a search input to be anywhere within the document
     • Though, honestly, how likely are we to vary to the position of the search input again?!
   • It does have a negative performance implication (see T274869#6902338), which is somewhat lessened by the use of jQuery.one()
2. Increase the scope of the selector in the SearchSatisfaction instrument ever-so-slightly
   • Doesn't have the negative performance implication as 1
   • Unlike #p-search, there doesn't appear to be a contract for the ID of the header container element (if there is one), e.g.
     • Vector: #mw-head
     • Vector V2: .mw-header
     • Timeless: #mw-header or #mw-header-container
     • Monobook: #mw-sidebar (search is in the sidebar)
   • It doesn't have the negative performance implication as above
3. Make Vector's App component mount on a different element
   • Doesn't have the negative performance implication of 1
   • Allows other developers to rely on the #p-search element, which does appear to be part of the implicit contract of our HTML output
   • Requires a little more DOM manipulation in Vector

Myself, I'm inclined to go with 3.

In T274869#6905317, @Krinkle wrote:

In T274869#6904051, @phuedx wrote:

In T274869#6902338, @Krinkle wrote:

In T274869#6902041, @gerritbot wrote:

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

This seems problematic for performance as this installs a global event listener, demanding synchronous eventloop syncing and interruption for almost all input events.

… once as jQuery.one should unbind the event listener after its first invocation. However: this is the case for each individually-bound event listener; and the cost is still unnecessary. […]

Thanks, I did miss that!

Since you mentioned this in the last comment, figured I'd elaborate a little. The patch used $(document).one('keypress', 'input[type="search"]', fn). This does not incurr overhead once only. It incurrs it indefinitely for every keypress just like on() -- unless and until the user decides to use the search feature on that particular page load, taps there, and presses a key. After that particular combination of events, for that particular page load, then the "one" condition would be fulfulled and we unbind the event listener, thus no longer invoking the cost of jQuery internals matching the css selector against the event target during each of those key. For someone creating an account, editing articles, using a form, engaging in discussion etc,, they would retain this overhead throughout each of those page loads, on every keypress and other input event, up until they use search, and then the next page load it'd start again.

Change 681670 had a related patch set uploaded (by Phuedx; author: Phuedx):

[mediawiki/skins/Vector@master] [search] Don't destroy #p-search element

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

Change 681670 merged by jenkins-bot:

[mediawiki/skins/Vector@master] [search] Don't destroy #p-search element

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

The deployment for this change is scheduled for this Thursday, April 29th. I will plan to QA following deployment after data has been logged and available to query in SearchSatisfaction.

Search sessions initiated (PASSED)
Confirmed that the new search widget sessions are logging as expected following the deployment of the fix on 29 April 2021.

Here are the daily search session initiated by logged-in users on the pilot wikis on vector.

Some explanation of various decreases/increases depicted in the chart:
18 March: decrease in new search widget session when the regression occurred,
31 March: Decrease in old search widget sessions when the AB test ended.
29 April 2021: Increase in new search widget session when the fix was deployed.

Seach sessions completed
When working on the AB analysis for T275200, I noticed that the search sessions completed for the new widget seemed lower than expected. Search Sessions completed is currently calculated by looking at search sessions initiated in the new search widget and that includes at least 1 click to a search result.

I did a little digging and it looks like there might be an issue in the instrumentation of clicks made directly to the search button for the new search widget (done by either clicking the search button or hitting enter on the keyboard). For the new search widget, we record an expected number of clicks to the suggested autocomplete results (recorded in SearchSatisfaction as any event.position >= 0 but we record a much smaller than expected number of clicks to the search button (recorded in SearchSatisfaction as any event.position = -1).

See chart below comparing the number of daily search sessions completed by search click type for the new vs the old search widget. Only about 9% of all search sessions completed on the new search widget are completed by clicking the search button compared to almost 47.7% for the old search widget.

@phuedx - I have not been able to isolate this to a specific date or pilot wiki so I'm wondering if the fix deployed in https://gerrit.wikimedia.org/r/c/mediawiki/extensions/WikimediaEvents/+/670526/ did not address this particular part of the instrumentation.

Implication for the AB Test
This does not impact data and analysis on the KPI (search sessions initiated) but does impact data used to calculate search sessions completed (assuming we want to account for clicks to the search button). I'm currently looking into other possible metrics or workarounds using the data we have that might still give us some insight into completed searches. (cc @ovasileva)

Change 690527 had a related patch set uploaded (by Phuedx; author: Phuedx):

[mediawiki/skins/Vector@master] search: Wire up submit event handler

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

So… After puzzling over this for a while, I noticed that we hadn't wired up the function that handles the user clicking the search button…

Change 690527 merged by jenkins-bot:

[mediawiki/skins/Vector@master] search: Wire up submit event handler

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

QA instructions for the above patch only:

1. Navigate to https://en.wikipedia.beta.wmflabs.org/wiki/Main_Page?useskinversion=2
2. Run the following in your browser's JavaScript console:

mw.eventLog.logEvent = ( schemaName, eventData ) => console.log( schemaName, JSON.stringify( eventData, null, 2 ) );

3. Search for "A" with the WVUI search autocomplete widget
4. Click the "Search" button
5. Observe that a SearchSatisfaction event has been logged with:
   • action="click"
   • source="autocomplete"
   • position=-1

What patch(es) need code review (I don't see any associated)?