Page MenuHomePhabricator

Instrumentation QA for new search widget
Open, HighPublic

Description

Description

Splitting off from T259798: Deploy the new Vue.js search experience to the Beta-Cluster and Test Wikipedia for clarity. This task will cover all beta cluster and production instrumentation QA for the new search widget

Acceptance Criteria

  • Ensure events recorded as expected. See T256100 for instrumentation details.
  • Ensure A/B bucketing functioning as expected. See T261647 for AB test set-up details.

Steps

  • Pre-Deployment Instrumentation Data QA on Testwiki
    • Ensure new search widget events are recorded as expected
    • Confirm all events needed to complete the AB analysis are recorded. See T275200
    • QA edit bucket instrumentation change to SearchSatisfaction [Pending Deployment]. T272991
  • Post-Deployment Instrumentation Data QA on Pilot Wikis
    • Confirm that AB buckets are balanced on each pilot wiki following deployment
    • Re-check that events are recorded as expected on all Pilot Wikis
  • Update QA Instrumentation QA and data checks document

Event Timeline

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)

search_typeskinVersionsearch_location# of events# of distinct sessions initiated
NULLlegacyheader-navigation8524
NULLlatestheader-moved3013
WVUIlatestheader-moved216
  • 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

search_typelogged-in statussearch location# of events# of distinct search sessions initiated
WVUIlogged-inheader-moved141
NULLlogged-inheader-moved3013
WVUIlogged-outheader-moved75

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)

wikiis_anonymousedit_bucketnum_eventsnum_sessions
mediawikiwikifalse0 edits7514
mediawikiwikifalse1-4 edits61
mediawikiwikifalse100-999 edits502
mediawikiwikifalse1000+ edits51
mediawikiwikifalse5-99 edits586
mediawikiwikitrue0 edits132199
testwikidatawikitrue0 edits21

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:

wikiinputLocationextraParamsnum_eventsnum_sessions
frwiktionaryheader-movedNULL56051261
frwiktionaryheader-movedWVUI356141
hewikiheader-movedNULL1715973
hewikiheader-movedWVUI786171
ptwikiversityheader-movedNULL14328
ptwikiversityheader-movedWVUI53

Stage 3 Deployment Pilot Wikis

wikiinputLocationextraParamsnum_eventsnum_sessions
euwikiheader-movedNULL3882680
euwikiheader-movedWVUI5419
fawikiheader-movedNULL89451496
fawikiheader-movedWVUI686191
frwikiheader-movedNULL7685414549
frwikiheader-movedWVUI3065834

Stage 4 Deployment Pilot Wikis

wikiinputLocationextraParamsnum_eventsnum_sessions
bnwikiheader-movedNULL889
bnwikiheader-movedWVUI113
dewikivoyageheader-movedNULL205
kowikiheader-movedNULL34298
kowikiheader-movedWVUI4320
ptwikiheader-movedNULL822216
ptwikiheader-movedWVUI12634
srwikiheader-movedNULL13532
srwikiheader-movedWVUI155
trwikiheader-movedNULL615122
trwikiheader-movedWVUI309

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)

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.

wikiinputLocationextraParamsnum_sessions
frwiktionaryheader-movedNULL24
frwiktionaryheader-movedWVUI12
hewikiheader-movedNULL15
hewikiheader-movedWVUI31
euwikiheader-movedNULL13
euwikiheader-movedWVUI7
fawikiheader-movedNULL20
fawikiheader-movedWVUI26
frwikiheader-movedNULL205
frwikiheader-movedWVUI166
kowikiheader-movedNULL4
kowikiheader-movedWVUI7
ptwikiheader-movedNULL42
ptwikiheader-movedWVUI40
srwikiheader-movedNULL5
srwikiheader-movedWVUI10
trwikiheader-movedNULL19
trwikiheader-movedWVUI14

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

[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?

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?

Update: Confirmed during the Web team standup today that this is expected as the new search widget was turned off to non-pilot wikis when the AB test started to not interfere with the test data. We should not expect to see new search widget session events from non-pilot wikis for the duration of the AB test.

@ovasileva - Going ahead and marking this task as resolved as all the post-deployment checks have been completed but let me know if we need to reopen or you have any questions.

Here is the QA notebook and tracking doc for reference.

@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

datesearch_typenum_sessions
2021-03-17old2949
2021-03-17new3287
2021-03-18old2924
2021-03-18new2359
2021-03-19old2750
2021-03-19new234
2021-03-20old2706
2021-03-20new196
2021-03-21old2997
2021-03-21new209
2021-03-22old2980
2021-03-22new225

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.

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.