More reliable statistics.js: retry after 2 s when request fails

1. Need the ready since you can't be sure Drupal.settings will be properly populated before the ready event (that bit me a few times).
2. I'm not sure how useful the error handler would be, what do you mean? You want a way to specify an error handler, is that it?
(3. POST won't have cache issues in case some proxy in the middle is caching GET requests.)

makes sense, why not.

I made the following changes:
1. Changed request type to get and added timestamp parameter to avoid cached by malformed proxies
2. made "use strict" global
3. made statistics.php a variable populated during page load
4. changed ready event to be jquery ready. Drupal.settings is ready at that moment.
5. added basic error handling. So far it checks for response code, and I think there's no reason to retry if we have server-related problems (4xx/5xx)

+ddd ? shouldn't be here.

I'm not sure about the amount of changes in here, seems less readable than before.

By the way, I think ts parameter is not necessary, since jQuery adds it's '_' parameter, so probably we can just skip that

yes, sure about ddd - placed this in statistics.php to simulate server error, and submitted by mistake

Switched back to the syntax found in other D8 JavaScript files.

And... slightly more readable indents

Tests fail, since there are 2 writes to database during AJAX call - in page handler and in hook_exit()
I'm new in D8, so please excuse me: what's the "standard" way to determine execution context in hook_exit()?
(i.e. what's the current menu_item, or if statistics was already called etc.)

As an alternative, maybe it's possible to add another setting to the settings form - either to use AJAX for statistics or traditional hook_exit(). I see no reason to use both methods simultaneously

tag

1. Changed tests to issue GET requests
2. Added error handler to $.ajax() call

Turns out search tests also call statistics.php, fixed failing test.
(By the way, isn't that a sign of unneeded intimacy between 2 modules? Shouldn't search module call API function of statistics module instead?)

#15: 1684968-15.patch queued for re-testing.

Found this issue... I'm actually working on that search module test right now so maybe you can remove it from this issue. And that section of the search module test is not working anyway due to #893302: Search ranking based on the factor "number of comments" & "No of Page Views" is broken

So in comment #15, you suggest that a statistics API call should be used instead of trying to simulate viewing the node. I looked in statistics.module and did not see any API function to call... Can you enlighten me? And meanwhile I'll try taking the modifications you made to the search test over to that other issue.

Actually, I can't really try that patch, as it looks like statistics.php is using POST not GET... not sure what to do.

1. Switching to a GET request seems pointless. There's no performance benefit at all. Instead you now have to work around HTTP semantics.

2. +++ b/core/modules/statistics/statistics.js
   @@ -2,14 +2,19 @@
   -  $(document).ready(function() {
   ...
   +  Drupal.stats = function(){
   ...
   +      error: function(jqXHR, textStatus, errorThrown){
   +       if(jqXHR.status<400){
   +        window.setTimeout(Drupal.stats, Drupal.settings.statistics.retry);
   +       }
   +      }
   ...
   +  Drupal.stats();
   

   These are the truly essential bits … except that It can be inline code after Drupal.settings (or add a onContentReady function) is neither implemented nor sensible to implement — it'd require us to move JS code for the Statistics module in the base module.

3. +++ b/core/modules/statistics/statistics.module
   @@ -104,7 +104,13 @@ function statistics_permission() {
   +      'logger' => base_path() . drupal_get_path('module', 'statistics') . '/statistics.php',
   

   This is already implemented in HEAD.

4. +++ b/core/modules/statistics/statistics.module
   @@ -104,7 +104,13 @@ function statistics_permission() {
   +      // Timeout between logging attempts, in milliseconds.
   +      // @todo: add this to settings screen?
   +      'retry' => 100,
   

   There's no point in making this configurable (who's ever going to customize it?), it just means more data to send with every HTML response.

AFAICT that means there's basically nothing else to be done here. The only thing this would gain, is retries.

+++ b/core/modules/statistics/statistics.js
@@ -2,14 +2,19 @@
+       if(jqXHR.status<400){
+        window.setTimeout(Drupal.stats, Drupal.settings.statistics.retry);
+       }

Retrying after a <400 response makes no sense. <400 implies 2xx or 3xx. 3xx is a redirect, which browsers would automatically follow. 2xx would mean it'd have succeeded.

Implemented just the retrying, and made it much simpler.

Is it possible, or even needed to write a test for this? Basically overwrite drupalSettings.statistics.url to a new controller that implements something like a this and extends statistics.php for the rest of the code.

if (!$_COOKIE['already_visited_statistics']) {
setcookie('already_visited_statistics', TRUE);
sleep(5);
}

include 'statistics.php';

I love how much simpler #25 is compared to the previous approaches, great work Wim!

- clean up code-style issues.

Clarifying issue title.

I'm also not sure about the need for explicit test coverage for this, it seems overkill.

@drpal, thoughts?

@Wim Leers The only thing I could think of would be to ensure that the request will retry after 2 seconds, however that is probably not needed since that behavior is part of jQuery.

Ok, I was doutbful about if such a test was useful anyway, setting to RTBC based on @drpal's feedback.

I'm not 100% convinced about this fix.
Let's assume someone would have configured the webserver to just have index.php as single front controller (This is security best practise), and then you decide to ship with statistics module on your website.

As your dev environment is not your life environment you start hitting your main index.php with this statistics script, and you end up increasing your load by 5x.

Possible solutions:

• Have a strict limit of the amount of retries (Let's say 3)
• Use an exponential retry time, see https://github.com/guzzle/guzzle/blob/master/src/RetryMiddleware.php as an implementation.

@dawehner: Yes, I agree we shouldn‘t retry every 2 seconds indefinitely.

An exponential or maybe cubic growth function seems to be the best fit in every respect. This way we could, IMHO should, even start with an interval considerably smaller than 2s.

+++ b/core/modules/statistics/statistics.es6.js
@@ -11,8 +11,18 @@
+            this.retryDelay = this.retryDelay * (2 * 1000);

Wait, so we increase it from 1 to 2000 to 4000000, this feels quite a steep increase.

📈 does seem a bit excessive. This dials it back to 2, 4, and 8 seconds.

Thank you @drpal for the quick iteration cycle!

True, let’s try it this way. RTBC.

Nice, @drpal, and great remark, @dawehner!

Should we implement backoff as per Google's suggestion? See https://developers.google.com/analytics/devguides/reporting/core/v3/erro...

Also a standard way of backing off feels useful rather than coding something special for statistics

Ah, so we are almost implementing what google suggests :)
Do we understand the random amount of milliseconds bit? (I guess this ensures equal distribution of requests to the server)

This issue is being reviewed by the kind folks in Slack, #need-reveiw-queue. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge require as a guide.

At this time we would need a D10.1.x patch or MR for this issue.

Patch for 10.1.x.
Please review.

I think this is very nearly ready - we should be adding in some jitter as described in https://aws.amazon.com/blogs/architecture/exponential-backoff-and-jitter/ - I also think we should be adding a follow-up to generalise this type of functionality as it is useful.