From a2929ad748115ec32a19796d513dcf93983d95c2 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 8 Apr 2020 17:55:53 +0100 Subject: [PATCH] Delay AJAX calls if the server is slow to respond MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit By default our AJAX calls were 2 seconds. Then they were 5 seconds because someone reckoned 2 seconds was putting too much load on the system. Then we made them 10 seconds while we were having an incident. Then we made them 20 seconds for the heaviest pages, but back to 5 seconds or 2 seconds for the rest of the pages. This is not a good situation because: - it slows all services down equally, no matter how much traffic they have, or which features they have switched on - it slows everything down by the same amount, no matter how much load the platform is under - the values are set based on our worst performance, until we manually remember to switch them back - we spend time during incidents deploying changes to slow down the dashboard refresh time because it’s a nothing-to-lose change that might relieve some symptoms, when we could be spending time digging into the underlying cause This pull request makes the Javascript smarter about how long it waits until it makes another AJAX call. It bases the delay on how long the server takes to respond (as a proxy for how much load the server is under). It’s based on the square root of the response time, so is more sensitive to slow downs early on, and less sensitive to slow downs later on. This helps us give a more pronounced difference in delay between an AJAX call that is fast (for example the page for a single notification) and one that is slow (for example a dashboard for a service with lots of traffic). *Some examples of what this would mean for various pages* Page | Response time | Wait until next AJAX call ---|---|--- Check a reply to address | 130ms | 1,850ms Brand new service dashboard | 229ms | 2,783ms HM Passport Office dashboard | 634ms | 5,294ms NHS Coronavirus Service dashboard | 779ms | 5,977ms _Example of the kind of slowness we’ve seen during an incident_ | 6,000ms | 18,364ms GOV.UK email dashboard | `HTTP 504` | 😬 --- app/assets/javascripts/updateContent.js | 14 +++++- app/templates/components/ajax-block.html | 3 +- app/templates/views/dashboard/dashboard.html | 10 ++-- app/templates/views/notifications.html | 6 +-- .../views/notifications/notification.html | 4 +- package.json | 2 + tests/javascripts/updateContent.test.js | 50 +++++++++++-------- 7 files changed, 52 insertions(+), 37 deletions(-) diff --git a/app/assets/javascripts/updateContent.js b/app/assets/javascripts/updateContent.js index 6297e3854..f519a0f63 100644 --- a/app/assets/javascripts/updateContent.js +++ b/app/assets/javascripts/updateContent.js @@ -3,6 +3,12 @@ var queues = {}; var dd = new global.diffDOM(); + var interval = 0; + + var calculateBackoff = responseTime => parseInt(Math.max( + (250 * Math.sqrt(responseTime)) - 1000, + 1000 + )); var getRenderer = $component => response => dd.apply( $component.get(0), @@ -19,7 +25,9 @@ var clearQueue = queue => (queue.length = 0); - var poll = function(renderer, resource, queue, interval, form) { + var poll = function(renderer, resource, queue, form) { + + let startTime = Date.now(); if (document.visibilityState !== "hidden" && queue.push(renderer) === 1) $.ajax( resource, @@ -33,6 +41,7 @@ if (response.stop === 1) { poll = function(){}; } + interval = calculateBackoff(Date.now() - startTime); } ).fail( () => poll = function(){} @@ -49,10 +58,11 @@ getRenderer($(component)), $(component).data('resource'), getQueue($(component).data('resource')), - ($(component).data('interval-seconds') || 1.5) * 1000, $(component).data('form') ); }; + global.GOVUK.Modules.UpdateContent.calculateBackoff = calculateBackoff; + })(window); diff --git a/app/templates/components/ajax-block.html b/app/templates/components/ajax-block.html index c0aa403d8..3638e13d3 100644 --- a/app/templates/components/ajax-block.html +++ b/app/templates/components/ajax-block.html @@ -1,10 +1,9 @@ -{% macro ajax_block(partials, url, key, interval=5, finished=False, form='') %} +{% macro ajax_block(partials, url, key, finished=False, form='') %} {% if not finished %}
diff --git a/app/templates/views/dashboard/dashboard.html b/app/templates/views/dashboard/dashboard.html index e7332e9f5..fa8405690 100644 --- a/app/templates/views/dashboard/dashboard.html +++ b/app/templates/views/dashboard/dashboard.html @@ -19,25 +19,25 @@ {% include 'views/dashboard/write-first-messages.html' %} {% endif %} - {{ ajax_block(partials, updates_url, 'upcoming', interval=20) }} + {{ ajax_block(partials, updates_url, 'upcoming') }}

In the last 7 days

- {{ ajax_block(partials, updates_url, 'inbox', interval=20) }} + {{ ajax_block(partials, updates_url, 'inbox') }} - {{ ajax_block(partials, updates_url, 'totals', interval=20) }} + {{ ajax_block(partials, updates_url, 'totals') }} {{ show_more( url_for('.monthly', service_id=current_service.id), 'See messages sent per month' ) }} - {{ ajax_block(partials, updates_url, 'template-statistics', interval=20) }} + {{ ajax_block(partials, updates_url, 'template-statistics') }} {% if current_user.has_permissions('manage_service') %}

This year

- {{ ajax_block(partials, updates_url, 'usage', interval=20) }} + {{ ajax_block(partials, updates_url, 'usage') }} {{ show_more( url_for(".usage", service_id=current_service['id']), 'See usage' diff --git a/app/templates/views/notifications.html b/app/templates/views/notifications.html index d8bfa4bdd..7c9bbabc0 100644 --- a/app/templates/views/notifications.html +++ b/app/templates/views/notifications.html @@ -31,8 +31,7 @@ {{ ajax_block( partials, url_for('.get_notifications_as_json', service_id=current_service.id, message_type=message_type, status=status), - 'counts', - interval=20 + 'counts' ) }} {% call form_wrapper( @@ -83,8 +82,7 @@ partials, url_for('.get_notifications_as_json', service_id=current_service.id, message_type=message_type, status=status, page=page), 'notifications', - form='search-form', - interval=20 + form='search-form' ) }} {% endblock %} diff --git a/app/templates/views/notifications/notification.html b/app/templates/views/notifications/notification.html index 63bf4ceed..288cb7ee4 100644 --- a/app/templates/views/notifications/notification.html +++ b/app/templates/views/notifications/notification.html @@ -96,10 +96,10 @@
{% elif template.template_type == 'email' %}
- {{ ajax_block(partials, updates_url, 'status', interval=2, finished=finished) }} + {{ ajax_block(partials, updates_url, 'status', finished=finished) }}
{% elif template.template_type == 'sms' %} - {{ ajax_block(partials, updates_url, 'status', interval=2, finished=finished) }} + {{ ajax_block(partials, updates_url, 'status', finished=finished) }} {% endif %} {% if current_user.has_permissions('send_messages') and current_user.has_permissions('view_activity') and template.template_type == 'sms' and can_receive_inbound %} diff --git a/package.json b/package.json index c9005ed6a..866f2d3c5 100644 --- a/package.json +++ b/package.json @@ -50,6 +50,8 @@ "gulp-prettyerror": "1.2.1", "gulp-sass-lint": "1.4.0", "jest": "24.7.1", + "jest-date-mock": "^1.0.8", + "jest-each": "^25.3.0", "jshint": "2.10.2", "jshint-stylish": "2.2.1", "rollup-plugin-commonjs": "10.1.0", diff --git a/tests/javascripts/updateContent.test.js b/tests/javascripts/updateContent.test.js index b946480ec..bd1c92abb 100644 --- a/tests/javascripts/updateContent.test.js +++ b/tests/javascripts/updateContent.test.js @@ -1,3 +1,6 @@ +const each = require('jest-each').default; +const jestDateMock = require('jest-date-mock'); + const helpers = require('./support/helpers.js'); const serviceNumber = '6658542f-0cad-491f-bec8-ab8457700ead'; @@ -18,6 +21,8 @@ beforeAll(() => { // set up the object returned from $.ajax so it responds with whatever responseObj is set to jqueryAJAXReturnObj = { done: callback => { + // The server takes 1 second to respond + jestDateMock.advanceBy(1000); callback(responseObj); return jqueryAJAXReturnObj; }, @@ -100,7 +105,7 @@ describe('Update content', () => { jest.clearAllTimers(); }); - + test("It should make requests to the URL specified in the data-resource attribute", () => { // start the module @@ -136,24 +141,6 @@ describe('Update content', () => { }); - test("If an interval between requests is specified, using the data-interval-seconds attribute, requests should happen at that frequency", () => { - - document.querySelector('[data-module=update-content]').setAttribute('data-interval-seconds', '0.5'); - - // start the module - window.GOVUK.modules.start(); - - expect($.ajax).toHaveBeenCalledTimes(1); - - // units are milliseconds - jest.advanceTimersByTime(500); - jest.advanceTimersByTime(500); - jest.advanceTimersByTime(500); - - expect($.ajax).toHaveBeenCalledTimes(4); - - }); - describe("By default", () => { beforeEach(() => { @@ -175,17 +162,36 @@ describe('Update content', () => { }); - test("It should request updates every 1.5 seconds", () => { + test("It should request updates with a dynamic interval", () => { + // First call happens straight away expect($.ajax).toHaveBeenCalledTimes(1); - // units are milliseconds - jest.advanceTimersByTime(1500); + // It took the server 1000ms to respond to the first call so we + // will back off – the next call shouldn’t happen in the next 6904ms + jest.advanceTimersByTime(6904); + expect($.ajax).toHaveBeenCalledTimes(1); + // But it should happen after 6905ms + jest.advanceTimersByTime(1); expect($.ajax).toHaveBeenCalledTimes(2); }); + each([ + [1000, 0], + [1500, 100], + [4590, 500], + [6905, 1000], + [24000, 10000], + ]).test('It calculates a delay of %dms if the API responds in %dms', (waitTime, responseTime) => { + expect( + window.GOVUK.Modules.UpdateContent.calculateBackoff(responseTime) + ).toBe( + waitTime + ); + }); + }); describe("If a form is used as a source for data, referenced in the data-form attribute", () => {