Page MenuHomePhabricator

Update page performance tests to use null banner parameter
Open, LowPublic

Description

User Story

Changes in CentralNotice active campaigns affect performance. These changes are usually intentional and independent of Readers Web's mandate. Disable banner presentation for Readers Web performance tests by passing the banner=null query parameter. This should improve graph stability.

Acceptance Criteria

  • CentralNotice banners are disabled for performance tests
  • Graphs are stable

Event Timeline

Restricted Application changed the subtype of this task from "Deadline" to "Task". · View Herald TranscriptFeb 2 2019, 2:46 AM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
Jdlrobson moved this task from Incoming to Triaged but Future on the Readers-Web-Backlog board.
Jdlrobson added a subscriber: Jdlrobson.

Note: I assume this is for webpagetest

Is it not useful to know when a banner test is occuring?
(Note we could also do two jobs).

What does the performance team think of this proposal?

@phuedx has said that @Jdrewniak may have done this on Friday....

If its ok to make it possible to disable it, then do it.

I think we should run two tests, it's useful for us to also catch the banner timings. Let me know which URLs you are using and I can fix it.

@Peter, we monitor the Facebook and Barack Obama pages on both desktop and mobile.

https://grafana.wikimedia.org/d/000000205/mobile-2g?orgId=1

Cool, thanks @Niedzielski FYI I added one annotation to that dashboard:

Screen Shot 2019-02-07 at 6.53.36 AM.png (156×546 px, 17 KB)

I post changes there about upgrades to the environment. For example this week we switched to a AWS larger instance and I also try to make an annotation when browsers automatically gets upgraded.

Back to the issue: Sign it over to me when it's ready and I can fix it it. I want to do the same with the WebPageReplay tests we run too.

Thanks, @Peter! I'll assign this to you when the dependent patch, T215086, is merged!

T215086 just merged into CentralNotice master. It should go out with the train next week.

Note: I assume this is for webpagetest

Is it not useful to know when a banner test is occuring?
(Note we could also do two jobs).

Also related (probably quite useful, but involves a bit more work): T215087: Add automatic performance testing for all live banners

Thanks, @AndyRussG!! I'll assign this to @Peter on Thursday when the train arrives on enwiki.

I believe that the addition of that query parameter bypasses Varnish caching, which means that it's not that realistic anymore compared to normal pageloads, and is more subject to performance variations at the PHP level that would be normally be "hidden" by Varnish.

This might be fixable in VCL with a special case for that query parameter, though. Or maybe reproducing the query parameter's behaviour with an HTTP header, which might be cleaner?

@AndyRussG, do you think it would be possible to check the headers in CentralNotice.hooks.php? When a banner header with a value of null is set, maybe we don't call $out->addModules( 'ext.centralNotice.startUp' );. This would disable the CentralNotice banner JavaScript from executing.

@AndyRussG, do you think it would be possible to check the headers in CentralNotice.hooks.php? When a banner header with a value of null is set, maybe we don't call $out->addModules( 'ext.centralNotice.startUp' );. This would disable the CentralNotice banner JavaScript from executing.

Hi! That module is included on every pageview (other than Special, history, edit and diff pages) regardless of whether banners are available for the user. It is requested in the base HTML that most users only get from Varnish. If you're looking to measure typical performance, the client-side bow-out on banner=null is closer to what you'd get on a normal pageview with absolutely no banners available as determined by the server. Here is the code path for that. In fact, to better follow the normal code path of when no banners are available, we might want to move the call to cn.kvStoreMaintenance.doMaintenance() a little higher.

(In fact, the server can't usually determine that no banners are available for a user, so it's pretty rare to get choiceData.length===0. Usually there are a bunch of banners that the server thinks might be displayable to a user, and the client-side code has to figure out whether any are really displayable.)

What about cookies? While introducing new cookies is usually a no-no, in this case I think we could have CentralNotice frontend code disable itself when it sees a specific cookie, that the tests could set. This would allow us to leave the pageview being completely plain, served cached from Varnish.

If cookies don't work, could we set a header, check that server side, and pass a banner configuration to the JavaScript module?

You can't check things server side on a pageview coming from Varnish, that's the issue here. A realistic pageview from a reader is one that only hits our edge cache layer. It needs to be a signal that the CentralNotice client side code can see and act upon.

Jdlrobson lowered the priority of this task from Medium to Low.Mar 13 2019, 8:59 PM

Removing task assignee due to inactivity, as this open task has been assigned for more than two years (see emails sent to assignee on May26 and Jun17, and T270544). Please assign this task to yourself again if you still realistically [plan to] work on this task - it would be very welcome!

(See https://www.mediawiki.org/wiki/Bug_management/Assignee_cleanup for tips how to best manage your individual work in Phabricator.)