I do not know how many sites out there use core's statistics module, but there are certainly some that do. There are a lot of 3rd party services that do the same thing (probably even better), but I've met webmasters that prefer integrated statistics, since it fits better into their workflow.

Statistics module is, from the other hand, not very flexible and not really performance friendly. There is no way to change or extend it's behaviour at all. It can also become a performance issue on high-traffic websites since it performs a DB write on every page request (webkenny have spoken about that topic in his session @ DrupalCon Chicago).

Simple performance optimisation could be done via cache. Statistics module could track visits in Drupal's cache and keep it there until first cron, when data will be written to DB. This wouldn't offer any benefit for default DB cache, but it will be much better when memcached or APC are used, which are, frankly, used on most high-traffic Drupal sites. This is currently, unfortunately, impossible to do without patching core.

Another possibility would be to move this module more towards a simple framework for statistics, that would log statistics via custom "engines". Each engine would be responsible for all "dirty work" behind the scenes. Core would in that case implement just default implementation, that can be similar than current one. Other engines could be implemented by contrib modules. This implementation could be similar to core's cache, where we have default DB cache in core and contrib modules that implement other cache backends.

Are there any plans in this direction? Are there any other ideas about this module?

CommentFileSizeAuthor
#247 1209532-ajax-stats-not-working-IIS.patch667 bytesdavid_garcia
#228 drupal-1209532-statistics_via_ajax-228.patch6.66 KBsdrycroft
#217 drupal-1209532-statistics_via_ajax-217.patch6.56 KBiamEAP
#215 drupal-1209532-statistics_via_ajax-215.patch6.38 KBiamEAP
#207 1209532-206.patch1.53 KBtimmillwood
#203 1209532-203.patch1.53 KBtimmillwood
#199 1209532-199.patch1.76 KBtimmillwood
#197 drupal-1209532-speedup-statistics.patch787 bytesmikeytown2
#196 1209532-196.patch1.74 KBtimmillwood
#192 1209532-192.patch1.69 KBtimmillwood
#188 1209532-188.patch1.67 KBtimmillwood
#186 1209532-186.patch1.67 KBtimmillwood
#170 statistics-ajax-1209532-170.patch13.16 KBwiifm
#168 statistics-ajax-1209532-168.patch12.72 KBwiifm
#160 statistics-ajax-1209532-160.patch11.77 KBwiifm
#152 statistics-ajax-1209532-152.patch11.27 KBtimmillwood
#150 statistics-ajax-1209532-150.patch11.27 KBtimmillwood
#147 statistics-ajax-1209532-147.patch11.09 KBtimmillwood
#135 statistics-ajax-1209532-135.patch12.55 KBtimmillwood
#133 statistics-ajax-1209532-133.patch12.55 KBtimmillwood
#128 statistics-ajax-1209532-128.patch12.44 KBtimmillwood
#118 statistics-ajax-1209532-118.patch11.49 KBtimmillwood
#115 statistics-ajax-1209532-115.patch11.49 KBlucascaro
#113 statistics-ajax-1209532-113.patch10.49 KBlucascaro
#112 statistics-ajax-1209532-112.patch11.55 KBtimmillwood
#111 statistics-ajax-1209532-111.patch11.47 KBtimmillwood
#109 statistics-ajax-1209532-109.patch11.47 KBtimmillwood
#102 statistics-ajax-1209532-102.patch11.92 KBlucascaro
#100 statistics-ajax-1209532-99-2.patch10.84 KBlucascaro
#91 statistics-ajax-1209532-91.patch10.9 KBtimmillwood
#87 statistics-ajax-1209532-86.patch6.46 KBtimmillwood
#86 statistics-ajax-1209532-84.patch0 bytestimmillwood
#85 statistics-ajax-1209532-84.patch0 bytestimmillwood
#84 statistics-ajax-1209532-84.patch0 bytestimmillwood
#78 statistics-ajax-1209532-77.patch3.17 KBtimmillwood
#77 statistics-ajax-1209532-77.patch1.67 KBtimmillwood
#75 statistics-ajax-1209532-75.patch1.67 KBtimmillwood
#67 statistics-addajax-1209532-67.patch3.23 KBtimmillwood
#58 statistics-addcache-1209532-58.patch5.16 KBtimmillwood
#55 statistics-addcache-1209532-54.patch4.52 KBtimmillwood
#54 statistics-addcache-1209532-54.patch4.52 KBtimmillwood
#51 statistics-addcache-1209532-51.patch5.38 KBtimmillwood
#45 statistics-addajax-1209532-45.patch3.21 KBtimmillwood
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

timmillwood’s picture

EvanDonovan’s picture

I like the idea of making it a pluggable framework; I think that's been proposed for Search module as well?

RobLoach’s picture

Seriously? There's already so many solutions in the contrib world that already put the core Statistics module to shame. What would a Statistics framework provide? Drupal already has quite an amazing framework for retrieving information from itself. Not to mention all this doesn't work if you're running your website correctly on top of Varnish.

I'd suggest setting this to won't fix, #1018716: Remove Statistics module from D8 core, and recommending people look at contrib solutions via Open Web Analytics, Piwik or Statistics Pro. I'm sure there are more too.

Looking through the Statistics commit log, it doesn't look like any Statistics-related features haven't been added in quite a while. If it hasn't been improved in that long, do you really foresee it improving within the next year when there are already better contrib solutions available? Most people who use Drupal would just end up wanting Google Analytics anyway.... This is also avoiding the issue of it not even working behind Varnish/Boost anyway.

droplet’s picture

Malware wanring for the OWA site :(
www.openwebanalytics.com contains malware. Your computer might catch a virus if you visit this site.

Dave Reid’s picture

@RobLoach the idea would be that the statistics framework provides the re-usable things like 'the user doesn't want to be tracked' or 'exclude tracking on these paths' that *EVERY SINGLE* module duplicates. Why should a site that switches from GA to Piwik have to have their users opt-out all over again? This is why we can provide something in core that can easily be used by contrib.

jcisio’s picture

A framework sounds interesting, but I'm not sure how to do it correctly, and have something that works behind a reverse proxy.

There is a long discussion on gdo about how to make statistics better for high traffic websites http://groups.drupal.org/node/94609. In brief, statistics module now can work as expected with two modifications:
- For better precision: use ajax callback that pass the reverse proxy. This callback url is not neccessary on the same Drupal installation, but to a simple Drupal install that reads/writes statistics.
- For better performance: as described in the issue description, we put every page stats record in a cache table, and aggregate/update our statistics on cron.

webkenny’s picture

A big +1 to a pluggable back end to statistics. I am more than willing to be active in that effort. What this should be is something akin to Views and Views UI, so it's hard to know if it should be in core or in contrib (as side issues state) - But I think the concept of doing this as a framework is huge.

When I land in the states, I'll address specifically some ideas. But want to toss in my support early.

slashrsm’s picture

I was playing a bit with "the Varnish problem". I implemented a draft layer around core statistics module, that sends stats data via AJAX request after page load. The code can be found in a sandbox: http://drupal.org/sandbox/slashrsm/1247306.

I understand that most of the sites use Google Analytics, but there are use cases, when you also need internal Drupal Statistics. I have some websites at the moment, where customer uses GA, but we still use Drupal's Statistics for internal statistics information, which they need for their editorial decisions. We also need Drupal's stats to build lists of most read/most popular content. You cannot do this using GA, though.

I am prepared to get involved in this (and also to co-maintan/maintain) if Statistics stays in core or if it is moved into contrib. At the end of the day it is basically the same, but I still think that we should have al least some kind of stats in standard profile.

meba’s picture

It's good but my issue is that Radioactivity module is just way better and already in contrib...

xmacinfo’s picture

Radioactivity is overkill. We need to simplify statistics to keep only the basic view counter. The rest will should go to contrib.

bojanz’s picture

+1 for empowering slashrsm in any way.

We need to simplify statistics to keep only the basic view counter.

From a programming perspective, the counter is anything but simple and pretty much requires relying on memcache and / or doing some JS magic like the node.js solution. A SQL INSERT per page view is murder. It can't scale.

cweagans’s picture

Honestly, I think the best improvement we can do is `rm -rf modules/statistics` and leave it to contrib. This is not something that should be in core.

jcisio’s picture

@bojanz:

A SQL INSERT per page view is murder. It can't scale.

I don't know why it can't scale. Every web server does the same thing for access log, every analytics tool does the same thing. And it scales. The problem is that how we do that INSERT.

cweagans’s picture

@jcisio, think about a scenario where a site is getting hundreds of thousands of page views per minute.

With the current setup, that's hundreds of thousands of queries per minute.

Does anybody really have the access log turned on in production? Analytics tools can be load balanced across multiple servers (Google Analytics is, and it's trivial to do the same with any other analytics setup).

That's why a bunch of people are saying that it's better offloaded to a 3rd party provider. 3rd party providers are going to be able to do analytics much better than we will ever be able to, and their tool will be able to evolve and grow all the time, unlike something that's locked into core for n years.

webkenny’s picture

I think it's important we distinguish here between traditional access_log writes and database writes. @cweagans makes a good point. Folks don't use the DB intensive access log capabilities on production sites. Instead, the file system gets those writes which will always be faster. It's the same reason people use syslog and not dblog on their production sites.

Bottom line. In support, daily, and at many performance chats folks recommend that the Statistics module be turned off. Way too much activity on higher traffic sites.

Now, I believe we still have to have some option for folks to have the most basic statistics on their sites but why not do this in a file on the file system (not necessarily in sites/default but somewhere) where they can parse these? It's an option for the lowest common denominator of sites. Much like an example which other modules could build from. You could have this as option that it writes from core to a faster backend or that it doesn't and some other module/service will handle the captures.

Let's get real though. Here's how I would see something like this working. Providing the API for modules to hook into and do their own processing would be excellent but if we were to look at this at a high level:

  • Provide a series of hooks in various places that, with the option to write turned on, do so. With it off, Statistics merely exposes the hooks for other modules to do its bidding
  • Take that one step further and allow administrators/privileged users to determine what Drupal should be exposing. On some sites, we may want to know page-level visits, on others it might only be important for us to know when someone hits a content type, and even on others when someone is viewing a piece of content which is tagged in a certain way. So settings like: Taxonomy Level, Node Level, Content Type Level, etc. This has the added benefit of allowing site admins to tune their statistical output for performance.
  • Provide some kind of mechanism that allows the tracking of authenticated users. Now, of course there are some services (Google Analytics) where this is verboten but it's not core's job to make sure the outside modules are meeting TOS. That's their job.
  • Provide settings (thank you @davereid) that are the basic layer for Opt-in/Opt-out tracking and other configurations at the user level.

Borrowing from the design patterns of our platform and basically "broadcasting" statistical events that modules could consume would be an excellent way to standardize and then allow all the fancy processing to take place somewhere else be it in a module or a module that connects to a third party service.

Drupal's core shouldn't be responsible for that. The less monolithic we can make the Drupal requests and the more we can plug into existing systems which already do the work well, the better IMO.

Love where this conversation is headed, btw. Great stuff in here.

jcisio’s picture

Category: feature » task

@cweagans I tested with my Atom-like server (2 GB RAM, VIA Nano processor U2250 = half of an Atom 330), and it can do 5000 inserts per second or 300.000 inserts per minute. If your site has hundreds of thousands of pageviews a minute, your site has more pageviews than all Wikipedia sites aggregated (6B pageviews/month or 140.000 pageviews per minute).

However, we even don't need to deal with all of that. What we want to do is to make sure that it can scale. And yes, db insert can scale.

If your production server does not log, it's a personal choice. Many servers have it enabled. It costs nearly nothing to write log, except when you want a single server to serve ten thousands of hits per second. Only in that case it may affect your performance.

About an API for statistics, it's great but it seems too hard. As you need to write record directly, or fallback to Ajax request when behind a reverse proxy, and that's the tricky part.

Bojhan’s picture

@davereid Is it not the role of a search API module to provide that kind of functionality? I am a bit split on the fact that all discussion here seems related to providing better API's and high-end website performance problems.

timmillwood’s picture

Title: Count node views via AJAX in the statistics module » Improve statistics module for D8?
Version: 7.x-dev » 8.x-dev
Assigned: timmillwood » Unassigned
Status: Needs work » Active
Issue tags: -Needs backport to D7

I really think this module needs to be deleted and rewritten from the ground up

http://www.millwoodonline.co.uk/blog/my-views-drupal-statistics-module

Here it what I would like to see:-

  • A core module for tracking many different aspects of a page request.
  • Javascript based interaction to work with varnish and similar caching.
  • Able to interact with local or remote data sources
  • Rules, settings and attributes to allow control of statistics gathering
  • Views integration to display statistics

My personal next step would be to remove the statistics module and start an initiative to rewrite the module from the ground up.

The first steps for the new module would be to define a new hook, maybe hook_statistics. This hook would be called at every page load and passed details / statistics about the page being loaded and machine loading it via Javascript / AJAX. Many modules could then use hook_statistics to save this data to the local database or any datasource.

fgm’s picture

One thing to consider is that some contrib modules may be using data from statistics.module (esp. content views) as it is the only reliably available source of content views on any drupal site, which can be required since it is part of core. Of course, larger sites will have professionally set up analytics from Google, Xiti or whatever, but this is only the tip of the iceberg when it comes to drupal deployments.

The problems with performance are very real, of course, but having "something" in core (or standard profile) to count content views seems difficult to avoid, and likely difficult to put outside of core (kernel) due to the performance implications. And if a "hook_statistics" exists, even these could be offered as a standard way to query for these data even when the core/kernel/standard module is replaced by something with a better performance profile on higher-level sites.

giorgio79’s picture

#fgm "One thing to consider is that some contrib modules may be using data from statistics.module (esp. content views) as it is the only reliably available source of content views on any drupal site, which can be required since it is part of core. "

Any contrib module can require another contrib module. Views requires CTools. This shouldn't be an argument for keeping it in core :)
What matters is whether there is a real need for it. If a module satisifes it, the webmaster will install it whether it is core or contrib.
It seems the point is Statistics needs to grow stronger and it is difficult to develop it as fast as needed in core...

#DaveReid "@RobLoach the idea would be that the statistics framework provides the re-usable things like 'the user doesn't want to be tracked' or 'exclude tracking on these paths' that *EVERY SINGLE* module duplicates. Why should a site that switches from GA to Piwik have to have their users opt-out all over again? This is why we can provide something in core that can easily be used by contrib."

This can be provided in contrib as well. I never switched from Google Analytics to anything else. :) Although I may not be representative of the average Drupal webmaster.

slashrsm’s picture

I totally agree with things pointed out in #15 and #18. Is there any interest for a sprint, where we'd plan this and start with initial implementation?

RobLoach’s picture

Instead of improving the Statistics module, what if we were to improve the Radioactivity module?

timmillwood’s picture

Great idea Rob, created another issue to track this idea. http://drupal.org/node/1277710

/me goes to install Radioactivity module.

fgm’s picture

Maybe it has improved since 2010 but at the time, Radioactivity just was not an option for high-traffic sites: too much of a hog on resources.

timmillwood’s picture

@fgm - maybe that's something we can help with when moving it to core. I have not extensively tried the module yet, but from what I hear it is better than the core statistics module.

jcisio’s picture

#22 I'm not sure if Radioactivity does the same thing as statistics.module does, i.e. it counts node views.

Even radioactivity can technically count node views (by using a very long lifetime), I don't think that it's something that could go in core.

fgm’s picture

One thing that could be improved would be to bring in counting/listing 403/404 if these are to be lost when evolving the statistics/dblog couple, as was suggested on the monster module dropping issue.

xmacinfo’s picture

Semantically, we do not need statistics in core. Statistics helps giving an overview, a dashboard-like view of the current statistics or past ones. With statistics we can do all types of reporting.

All statistics reporting engine should live outside core.

What we need in core, though, is a simple counter to at least duplicate what's in Drupal 6 and 7, ie. counts node views, counts 404 errors, etc. Hence, this is why we need to scale down the statistics module (and maybe rename it to simplecounter!).

As for radioactivity, this is overkill, and even if we take this and move it to core, at the end we would need to scale it down.

tcmug’s picture

Actually, perfect timing for this:

Radioactivity is heading towards a slimmer and lighter set of basic features that will be easy to extend: it will in its most basic form track "views" and have cache piercing and be as fast as drupal can (memcache storage, non-drupal-boostrap storage would all be extensions).

The current Radioactivity 2.x in core sounds way silly. I'd go for having the extendable basic functionality in core instead that is in the (unwritten) plans for 3.x.

fgm’s picture

In a way, this could be like the current tripled of includes/bootstrap.inc#watchdog() vs modules/dblog vs modules/syslog :

- a common API to record information
- one or more core implementations, at least one geared towards small sites and easy debugging, and possibly another one geared towards larger sites

xmacinfo’s picture

@tcmug: “…(unwritten) plans for 3.x”. Well, can someone actually write the plan? :-)

tcmug’s picture

Sort of what @fgm said on #30. I should point out that at the moment Radioactivity works pretty much solely on fields; a field that contains the number of views - or energy as it is in R - and that is then used in various manners to calculate popularity or just plain number of views. A field makes it useful on all entities and with a bit of Rules magic you can count views per week, per month etc. or as an example with Drupal commerce there is of course the number of sales that you could be interested in :)

We also use it to track user activity (login emits energy, commenting, blog posts etc.) and this is where the decay profiles come handy: since the energy decays over time only those who are continuously active have high energy and are thus rewarded somehow. So, its quite a versatile thing and I'd love to see something that would be the backbone in core.

@xmacinfo #31 sure, why not - I'll write em up :) But I'll of course make them bendy ones so that it'll be easy porting to D8 hehe

chx’s picture

I tested with my Atom-like server (2 GB RAM, VIA Nano processor U2250 = half of an Atom 330), and it can do 5000 inserts per second

Well, that must be MyISAM. Cos you wont commit more than 120 transactions per second unto a 7200RPM disc (cos you have 120 rotations a second and you can't do more than one fsync per rotation). And, if you happen to SELECT from other parts of the disk (ie need to seek) then you won't even get a commit for every rotation either. Or does this low-end server have a fast SSD? 5000 IOPS is not a small amount even for an SSD but def. doable.

joachim’s picture

jcisio’s picture

@chx: yes it was MyISAM with a low-end HDD (I guess, as this server costs 15 EUR/month). I don't think we need transaction support for this table, by the way.

timmillwood’s picture

Hi,

I would like to get back on to this issue. I really feel that a patch for the statistics module to do everything I would like to see would alter 99% of the lines. Therefore, can we remove the module, then work in contrib to make something better. If this is better module exists and works well for D7, we could then look at porting it to D8 and moving it to core.

Tim

timmillwood’s picture

Step 1) merge http://drupal.org/project/statistics_ajax into the statistics module so that it works with Varnish.
Step 2) reduce database writes.

jcisio’s picture

There is also a new module http://drupal.org/project/jstats that is known to work quite well, too. The problem is not only Varnish, the bigger issue is that a direct update to node_counter does not scale (each update requires a whole index rebuild).

timmillwood’s picture

jstats looks interesting, I had not seen it.

Maybe I will take the best from everything and try to get an AJAX / less database writes patch together.

slashrsm’s picture

I agree with #37. Apart from that I'd also add more flexibility. Currently we count node views just for one day and entire existence of a node. I see a powerful and flexible statistics framework not accepting this kind of assumptions. Why couldn't I track node views for last 3 days, 2 hours, ...

I've tried a lot of contrib modules offering statistics functionalitites, and none of them completely meets my requirements. The closest for now is Radiocativity, though. Mostly because of it's support for light bootstraps and Memcached support.

I think it is time to take a decision:
- should we remove statistics from core and create new, more powerfull and flexible, module in contrib (or extend one of existing ones to be really powerfull - even better)
- rewrite core's statistic to offer a sufficient degree of flexibility, scalability, plugability and extensibility.

This should, of course, also be affected by general orientation of core development in future, but I think that having a statistics module "as it is" in D8 is not wise at all.

timmillwood’s picture

My biggest concern with the statistics module is how many database writes it makes. I am trying find a place to store the data until cron runs, then write it to the table every hour.

Although, I am struggling to find somewhere to store it. Using another table would help a little as the indexes would not need to be cleared as often, but it would still require a lot of writes. I have also been thinking of how it could be stored in memory via php or MySQL.

I am open for other ideas.

slashrsm’s picture

Radioactivity uses Memcached, which works great (data is stored into memcache on emmit and agregated and stored in DB at cron). They also implement a simple script, that is used at JS emmits. Advantage of this script is ability to save data without bootstraping Drupal, which is much lighter.

There are some other options besides Memcached: APC user cache, MongoDB, Cassandra, any other NoSQL solution or even a txt file. It totally depends upon requirements and limitations you have. This is why I see storage as a pluggable framework, which lets user decide which technology to use.

jcisio’s picture

Store it in another table (maybe with Memory engine) without index would be a good solution. We can aggregate data periodically. Memcache does not allow that, at least without an ugly loop through every node.

tcmug’s picture

We wrote Radioactivity to be as light as possible with being flexible at the same time - you can do much more than register node views with it.

I don't see a valid reason why core should have a statistics module - however (and I think I stated it up there before) I see a need for adding simple tasks to drupal without drupal bootstrap.

Currently I'm in the process (but lacking the time) in separating the Radioactivity incident api to a module of its own - it would be a fairly small module and provide only the gears for sending an action to drupal for processing with multiple ways (ajax, nodejs, rules, etc.) (http://drupal.org/sandbox/tcmug/1364084 no commits yet though...)

timmillwood’s picture

Here's a patch which makes the node_counter use ajax.
I realise it's not a full solution to the statistics issues, but it's a step in the right direction.

timmillwood’s picture

Status: Active » Needs review

Adding need's review

cweagans’s picture

Status: Needs review » Needs work

Please don't add new standalone PHP files. In this case, there's no reason to do so: it can easily be added to statistics_menu().

timmillwood’s picture

Status: Needs work » Needs review

My reason for adding a standalone php file is so that I didn't have to fully bootstrap drupal every ajax call.
Is there a way to do this with hook_menu?

Status: Needs review » Needs work

The last submitted patch, statistics-addajax-1209532-45.patch, failed testing.

bojanz’s picture

@timmillwood
No. And there should be. Because without that the tiny ajax requests (registering a page visit, or a VBO checkbox selection) are almost useless because they are too slow.

EDIT: D6 contrib had sun's http://drupal.org/project/js

timmillwood’s picture

I have now added the caching of statistics get, the cache is then set to clear every cron run.

timmillwood’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, statistics-addcache-1209532-51.patch, failed testing.

timmillwood’s picture

Just noticed I managed to include my statistics and taxonomy patch into one.
Here's the real addcache patch.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
4.52 KB

Just noticed I managed to include my statistics and taxonomy patch into one.
Here's the real addcache patch.

Status: Needs review » Needs work

The last submitted patch, statistics-addcache-1209532-54.patch, failed testing.

timmillwood’s picture

urgh, just noticed that the cache is caching the same number of views for all nodes!

*goes back to work*

timmillwood’s picture

Assigned: Unassigned » timmillwood
Status: Needs work » Needs review
FileSize
5.16 KB

caching is a little better now.

Status: Needs review » Needs work

The last submitted patch, statistics-addcache-1209532-58.patch, failed testing.

timmillwood’s picture

I am wondering if the statistics module should count a node view every time a node is loaded even if it's in a view, panel or block.

xmacinfo’s picture

Node count should represent only the count where the complete node is displayed. If only the teaser or fields are shown, that would not apply for the core count. Panels, views and others could implement their own counters if they also want to count node load vs full node display.

So I'd like to keep a simple node count in core and leave the rest in contrib.

cweagans’s picture

I'd say that we should only track full node views and expose some API function that will allow other modules to modify that count easily.

That said, if we're down to the point where we only have a simple view tracker in core, is it really worth having in core anymore?

timmillwood’s picture

I think it is worth having a node_counter in core, many sites small, large and enterprise want to display "most popular" content. Therefore I don't think we should remove this feature, but it should also work when using external cache and also not be a performance issue.

I do however feel that the accesslog features in the statistics module are a little pointless with tools like Google Analytics offering a better alternative.

timmillwood’s picture

I would like to work on a better way of caching the node couter. On pages such as /node, taxonomy listings, and views, the node_counter table could be getting queried 10 times or more. Each of these queries are based on a different node id. Would it be better to store all the node counts in a serialised value as an object or array, then query that one value? Then when counting a node view would it be better to increment the cache directly rather than increment the database table? Does node_counter only really need to be a table as backup for the cache? therefore could the node_counter table just get updated from the cache each cron run?

/me might organise a drupalcon sprint on this (although I won't be in denver).

timmillwood’s picture

queues vs cache?

moshe weitzman’s picture

I recommend splitting off any read optimizations into a new issue. I would focus here on the great logging improvements you have made.

I think that only node/n views should be logged here.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
3.23 KB

Good thinking Moshe.

Here's the patch again that added ajax. It still includes statistics.php rather than using hook_menu just because of the performance benefits.

The issue I now have is that simpletest fails because it doesn't do the ajax call.

I doubt it will get committed without passing simpletest?

Status: Needs review » Needs work

The last submitted patch, statistics-addajax-1209532-67.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review

See the new statistics performance issue #1446932: Improve statistics performance by adding a swappable backend for items relating to performance.
This issue should be used for discussion around the use of AJAX to count node views allowing the use of external caching such as varnish or a CDN
All other statistics items should have a separate issue.

nod_’s picture

About the js, as it is now it'll run multiple time if there are other drupal ajax calls on the page,

(function ($) {
  var run = false;
  Drupal.behaviors.statisticsModule = {
    attach: function (context, settings) {
      if (!run) {
        var basePath = settings.basePath
        $.ajax({
          type: "POST",
          cache: false,
          url: basePath+"core/modules/statistics/statistics.php",
          data: settings.statistics,
        });
        run = true;
      }
    }
  };
})(jQuery);
timmillwood’s picture

Well, that's a bit stupid. Whys that?
May be we should fix it rather than use the 'run' workaround.

nod_’s picture

Ahaha no, that's the way it was designed, it's used to run behaviors init on new content coming from Ajax.

You don't even need to use behaviors here, beside convention maybe. settings is really Drupal.settings and it should already be available, just make sure you include that in the footer.

joachim’s picture

+    drupal_add_js(array('statistics' => array('nid' => arg(1))), 'setting');

Just to check I've followed this correctly... the way for a module such as Views to tell statistics module that 'nodes 1, 2, 3 have been looked at' would be to add something to the View which outputs that but with an array of nids:

+    drupal_add_js(array('statistics' => array('nid' => array(1, 2, 3))), 'setting');
timmillwood’s picture

Joachim,
Currently the patch I did only accepts one value for a nid. Therefore will only count one node per page. Therefore by design it's to could the view of a /node/ page and not the nodes loaded in a view. Although I could allow it to accept an array of nids then a contrib module could add the multiple node counting feature.

timmillwood’s picture

Quite a few changes here.
Moved adding of the JS to hook_node_view.
Moved the javascript to the footer and out of a behaviour.

Please review (the simpletest will fail as it doesn't know much about javascript)

Status: Needs review » Needs work

The last submitted patch, statistics-ajax-1209532-75.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
1.67 KB

Forgot to add statistics.php and statistics.js

timmillwood’s picture

Sorry, did it again

Status: Needs review » Needs work

The last submitted patch, statistics-ajax-1209532-77.patch, failed testing.

xmacinfo’s picture

Don't forget blank like at end of files. :-)

joachim’s picture

> Although I could allow it to accept an array of nids then a contrib module could add the multiple node counting feature.

I think that would be really useful. It would allow lots of interesting possibilities in contrib.

timmillwood’s picture

Title: Improve statistics module for D8? » Improve statistics module for D7/D8?
Issue tags: +Needs backport to D7

I will work on allowing this to accept an arrary of NIDs.

I also need to get the tests sorted, any help on the tests would be great.

timmillwood’s picture

I have been delayed on this trying to get the tests to work. I hope to get a new patch out soon that passes the tests either by fixing or removing the tests related on my Ajax changes.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
0 bytes

This patch allows the option to count node views even if only the teaser is loaded. It uses the same ajax system as the last patch.

Please review.

timmillwood’s picture

This patch allows the option to count node views even if only the teaser is loaded. It uses the same ajax system as the last patch.

Please review.

timmillwood’s picture

The patch did not attach properly

timmillwood’s picture

Trying that again.

timmillwood’s picture

Please ignore the patches in 84, 85 and 86.

Patch in 87 has passed, would be good to get some reviewers.

scito’s picture

Status: Needs review » Needs work

The last submitted patch removes only tests. There are no functional changes. It might be a problem.

BTW: I'm really glad that you work on this module. Thanks for your efforts so far.

timmillwood’s picture

Aww man! I really need to check / test my patches better.

Thanks for looking.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
10.9 KB

I had committed changes so I could do a pull, and I was only diffing with the local repo rather than the origin.

Here is the full patch to test.

Status: Needs review » Needs work

The last submitted patch, statistics-ajax-1209532-91.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review

Fails due to search.test?
WTF?

xmacinfo’s picture

This patch allows the option to count node views even if only the teaser is loaded.

I hope this is configurable. Usually we want to add only full nodes to the counter.

timmillwood’s picture

Yes, the patch adds a new checkbox to the settings page.
https://skitch.com/timmillwood/8md5r/statistics-d8.localhost

This defaults to the current behaviour in statistics.module of only counting full node views.

xmacinfo’s picture

@timmillwood: awesome!

timmillwood’s picture

Title: Improve statistics module for D7/D8? » Count node views via AJAX in the statistics module
lucascaro’s picture

Issue tags: -Needs backport to D7

#91: statistics-ajax-1209532-91.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Needs backport to D7

The last submitted patch, statistics-ajax-1209532-91.patch, failed testing.

lucascaro’s picture

Status: Needs work » Needs review
FileSize
10.84 KB

Hi, after an initial review of the patch, I've encountered some small issues that I modified and I'm attaching a new patch with these simple issues corrected:

* Removed a console.log in statistics.js
* Removed a comma after data: "nids="+nids in statistics.js
* Removed trailing whitespaces
* Removed ?> at the end of statistics.php
* fixed indentation in statistics.php

Status: Needs review » Needs work

The last submitted patch, statistics-ajax-1209532-99-2.patch, failed testing.

lucascaro’s picture

Status: Needs work » Needs review
FileSize
11.92 KB

Corrected the test case for search.test since it was counting on a non-ajax statistics. Now it works using a post call to statistics.php.

All the changes are in the patch below. please review!

lucascaro’s picture

Status: Needs review » Needs work

Hi guys, I think this patch could have a security vulnerability. The new file, settings.php doesn't check anything about the request and I think it could be vulnerable to a DOS attack (since it makes two database writes per request and the only validation is that $_POST['nids'] is set.

--- /dev/null
+++ b/core/modules/statistics/statistics.php
@@ -0,0 +1,34 @@
+<?php
+
+// Change the directory to the Drupal root.
+chdir('../../..');
+if (empty($_POST['nids'])) {
+  exit();
+}
+/**
+ * Root directory of Drupal installation.
+ */
+define('DRUPAL_ROOT', getcwd());
+
+include_once DRUPAL_ROOT . '/core/includes/bootstrap.inc';
+drupal_bootstrap(DRUPAL_BOOTSTRAP_VARIABLES);
+if (variable_get('statistics_count_content_views', 0)) {
+  $nids = json_decode($_POST['nids']);
+  watchdog('statistics', $_POST);
+  if (is_array($nids)) {
+    foreach ($nids as $nid) {
+      if (is_numeric($nid)) {
+        db_merge('node_counter')
+            ->key(array('nid' => $nid))
+            ->fields(array(
+              'daycount' => 1,
+              'totalcount' => 1,
+              'timestamp' => REQUEST_TIME,
+            ))
+            ->expression('daycount', 'daycount + 1')
+            ->expression('totalcount', 'totalcount + 1')
+            ->execute();
+      }
+    }
+  }
+}

Maybe these two lines fix that?

+++ b/core/modules/statistics/statistics.php
@@ -0,0 +1,34 @@
+include_once DRUPAL_ROOT . '/core/includes/bootstrap.inc';
+drupal_bootstrap(DRUPAL_BOOTSTRAP_VARIABLES);

I just wanted to have someone else look at this just in case.

Also, what would happen when nodes are loaded via ajax?

Cheers!

timmillwood’s picture

It checks if $nids is an array, it then checks if $nid is numeric. I am not sure of a way to exploit this, but others may know better. (/me pings greggles)

chx’s picture

I am less sure there is a security problem but from a performance standpoint it's silly to run one query / nid, just collect them nids in an array and run one query. IN is your friend. However, despite what the doxygen says db_merge does not allow multivalue inserts. Commit this as it is, file a database issue to fix this and link a followup issue to convert to a single db_merge.

lucascaro’s picture

Status: Needs work » Reviewed & tested by the community

@timmillwood, @chx great. RTBC then?

timmillwood’s picture

I would like to get the tests I took out back in, but only having partial success. I am happy for RTBC as long as I start a new issue to work on tests.

Chx: I have have been thinking about performance and was thinking about adding a cache table for statistics. Writing to then within statistics.php then writing to the node_counter table on cron, although not sure if this is the best option.

lucascaro’s picture

Status: Reviewed & tested by the community » Needs work

(for the tests)

timmillwood’s picture

Status: Needs work » Needs review
FileSize
11.47 KB

Added the tests back in, which are working better, but not perfectly.

Status: Needs review » Needs work

The last submitted patch, statistics-ajax-1209532-109.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
11.47 KB

Hope this one works better for you.

timmillwood’s picture

fixing path variable in tests.

lucascaro’s picture

Status: Needs review » Needs work
FileSize
10.49 KB

reroll with minor spacing corrections and patching search.test

Lars Toomre’s picture

Status: Needs work » Needs review

There are some formatting issues in statistics.php, including:

1) Incorrect indentation...
2) 'if(' needs to be 'if ('. (needs a space after if)
3) 'foreach(' needs a space after 'foreach'.

lucascaro’s picture

Status: Needs review » Needs work
FileSize
11.49 KB

rerolled again, this time includen the patch to search.test by @timmillwood and the fixes suggested by @Lars Toomre

lucascaro’s picture

Status: Needs work » Needs review
Lars Toomre’s picture

Hmmm... Not sure what is in #115 patch, but formatting of statistics.php appears to be the same.

Also noticed that there are some indent issues with statistics.js.

Perhaps wrong patch was attached?

timmillwood’s picture

Status: Needs review » Needs work
FileSize
11.49 KB

fixing the indentation and spacing issues.

lucascaro’s picture

Status: Needs work » Needs review

just for the test

lucascaro’s picture

Status: Needs review » Reviewed & tested by the community

marking as RTBC since it's been checked by various users, and the tests are back in.

timmillwood’s picture

So nice to finally see this RTBC!

Lars Toomre’s picture

Status: Reviewed & tested by the community » Needs review

Something funny is happening with testbot... Only passes 11 tests?

Setting back to needs review until all tests pass.

timmillwood’s picture

#118: statistics-ajax-1209532-118.patch queued for re-testing.

Lars Toomre’s picture

Status: Needs review » Needs work

Some more coding nits...

+++ b/core/modules/statistics/statistics.jsundefined
@@ -0,0 +1,12 @@
+     });
+  });

The indentation of the first set is one too much it seems.

+++ b/core/modules/statistics/statistics.moduleundefined
@@ -115,6 +98,18 @@ function statistics_permission() {
+    if(!empty($nids_json)) {

There needs to be a space between if and '('.

+++ b/core/modules/statistics/statistics.testundefined
@@ -104,6 +104,13 @@ class StatisticsLoggingTestCase extends DrupalWebTestCase {
+    drupal_http_request($stats_path, array('method' => 'POST','data' => $post,'headers' => $headers,'timeout' => 10000));

My understanding of coding standards is that there should be a space after each ',' in the array declaration. This is repeated several times throughout patch.

lucascaro’s picture

Status: Needs work » Needs review

#118: statistics-ajax-1209532-118.patch queued for re-testing.

timmillwood’s picture

When running the "STATISTICS REPORTS TESTS" using mod_fcgi I was getting 500 errors, now using mod_php I get fails.

https://skitch.com/timmillwood/8p8tp/test-result-stats.localhost

I am currently unsure why these are failing or why mod_fcgi / mod_php makes a difference.

Status: Needs review » Needs work

The last submitted patch, statistics-ajax-1209532-118.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
12.44 KB

Fixed the block test, forgot to call statistics.php, also tidied up some of the code.

timmillwood’s picture

Status: Needs review » Reviewed & tested by the community
timmillwood’s picture

Once committed to D8 I would like to re-roll this for D7.

lucascaro’s picture

Status: Reviewed & tested by the community » Needs work

Hey @timmillwood that's great! I have a few more comments, though. I don't know how much these are enforced but:
According to dreditor, there are still a few minor issues:

No newline at end of file:

+++ b/core/modules/statistics/statistics.php
@@ -0,0 +1,33 @@
\ No newline at end of file

two extra spaces:

+++ b/core/modules/statistics/statistics.php
@@ -0,0 +1,33 @@
+          'daycount' => 1, ¶
+          'totalcount' => 1, ¶

also here

+++ b/core/modules/statistics/statistics.js
@@ -0,0 +1,12 @@
+})(jQuery);
\ No newline at end of file

Also, according to http://drupal.org/coding-standards#phptags the closing ?> should be removed in statistics.php

+++ b/core/modules/statistics/statistics.php
@@ -0,0 +1,33 @@
+?>

what do you think?

xmacinfo’s picture

@lucascaro: You are right. All the points you highlight should be fixed. Thanks for checking these out.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
12.55 KB

Fixing @lucascaro's suggestions, but... also making the new checkbox only appear when the old one is checked using states.

Lars Toomre’s picture

Status: Needs review » Needs work

Thanks for the code clean up. It looks good except search.test which is missing spaces in the array definition there.

timmillwood’s picture

Re-roll for Lars

timmillwood’s picture

Status: Needs work » Needs review

changing status

timmillwood’s picture

Status: Needs review » Reviewed & tested by the community
catch’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/statistics/statistics.jsundefined
@@ -0,0 +1,12 @@
+(function ($) {
+  $(document).ready(function() {
+    var nids = Drupal.settings.statistics.nids;
+    var basePath = Drupal.settings.basePath
+    $.ajax({
+      type: "POST",
+      cache: false,
+      url: basePath+"core/modules/statistics/statistics.php",
+      data: "nids="+nids
+    });
+  });
+})(jQuery);

This needs an @file header. It should also use Drupal.behaviors rather than document ready.

Is there a particular reason to make the AJAX post request instead of putting a 1px gif on the page or similar?

+++ b/core/modules/statistics/statistics.admin.incundefined
@@ -269,6 +269,17 @@ function statistics_settings_form() {
+  $form['content']['statistics_count_only_full'] = array(
+    '#type' => 'checkbox',
+    '#title' => t('Count full nodes only'),
+    '#default_value' => variable_get('statistics_count_only_full', 1),
+    '#description' => t('Count views of only whole content and not as part of blocks, lists or teasers.'),
+    '#states' => array(
+      'visible' => array(   // action to take.
+        ':input[name=statistics_count_content_views]' => array('checked' => TRUE),
+      ),
+    ),
+  );
 
   return system_settings_form($form);
 }

We usually avoid the work 'node' in the UI. I don't necessarily agree with this and there's more than one issue trying to standardize, but it should be consistent with settings elsewhere in core.

"Count views of only whole content and not as part of blocks, lists or teasers." - this reads somewhat awkward to me as well, generally we don't have the 'view mode' concept fully embedded everywhere in core yet but referring to that might be better than the examples, or dropping the description altogether.

 'visible' => array(   // action to take.

Comment should be above the line.

+++ b/core/modules/statistics/statistics.jsundefined
@@ -0,0 +1,12 @@
+(function ($) {
+  $(document).ready(function() {
+    var nids = Drupal.settings.statistics.nids;
+    var basePath = Drupal.settings.basePath
+    $.ajax({
+      type: "POST",
+      cache: false,
+      url: basePath+"core/modules/statistics/statistics.php",
+      data: "nids="+nids
+    });
+  });
+})(jQuery);


This needs an @file header. It should also use Drupal.behaviors rather than document ready.

Is there a particular reason to make the AJAX post request instead of putting a 1px gif on the page or similar?

<code>
+    global $nids_json;
+    if (!empty($nids_json)) {
+      $nids = drupal_json_decode($nids_json);
+    }
+    $nids[] = $node->nid;
+    $nids_json = drupal_json_encode($nids);
+    drupal_add_js(drupal_get_path('module', 'statistics') . '/statistics.js', array('scope' => 'footer'));
+    drupal_add_js(array('statistics' => array('nids' => $nids_json)), 'setting');
+  }

If the only reason for the global is to avoid adding the same nids more than once during a request, then it should be a static variable. Also this is calling drupal_add_js() even when we otherwise might not be adding a new nid seems unnecessary - looks like it'd be OK to add the js file once with #attached and then just update the setting.

TBH I think it'd be easier to review this if there was one patch to switch from drupal_exit() to the ajax (or a 1px gif, I really don't know which is preferred at the moment), then a separate one about adding the feature to track statistics for node teasers - I'm not sure about adding that feature to core at all to be honest - it's adding extra complexity and configuration and generally I only want to know if a page was viewed directly, not if someone looked at a list of nodes where it happened to be.

timmillwood’s picture

Using a gif would be a good option, it could allow access_log and node_counter to work, it will also work for users who do not have Javascript enabled (unless the gif is added via JS, but I should be able to add it at the bottom of the page before some how.). The only concern I have is making sure it doesn't get cached, although headers could be set to stop it being cached.

I wanted to make statistics.php flexible enough so contrib modules could use it to could single and multiple node views, who while I was making sure it could take an array of nids I thought I might as well build in the checkbox to send multiple nids.

timmillwood’s picture

I am not sure using a gif is the cleanest way to do this as seems a little messy and could end up breaking site layouts. The AJAX route seems to be the most sensible and cleanest way to implement this.

I did not use behaviours under advise from nod_, as it means cleaner, simpler javascript that doesn't get run multiple times.

I will create a new patch removing the multiple nids feature, and tidying up the comments.

jcisio’s picture

@timmillwood: when a node is fetched (via AJAX) and display in, e.g., a modal frame, does it count?

nod_’s picture

The easiest way to address that is provide a JS function doing the actual ajax call. After it'll be the responsibility of whatever display the node in the modal form to integrate with stats JS API.

timmillwood’s picture

I'm not sure.

If hook_node_view is called, then the nid gets added to the Drupal.settings ready to be counted, and the statistics.js file is added to the page. So if the node is loaded via AJAX, hook_node_view will get called, but I don't think Drupal.settings and statistics.js will get added correctly.

jcisio’s picture

We can use Drupal.behaviors and check for div with class/id to know if a node is rendered in full view mode and get the nid. Of course themer could change the tpl to remove id/class but it's their responsability.

I think it's universal and better if we plan to use JS.

nod_’s picture

data- attribute, much better than a class or whatever. there is an issue somewhere in the queue to remove as much stuff from Drupal.settings as possible and replace with data- attributes on the relevant element.

jcisio’s picture

@nod_: yes it's much better, it will also prevent other problem (duplicate ID etc.). Of course, it is not a blocking issue. Change to use data-nid is fairly easy.

timmillwood’s picture

I have taken Catch's comments into account and rolled another patch.

xmacinfo’s picture

Status: Needs work » Needs review

:-)

catch’s picture

Status: Needs review » Needs work
+  if (!empty($node->nid)) {
+    drupal_add_js(drupal_get_path('module', 'statistics') . '/statistics.js', array('scope' => 'footer'));
+    drupal_add_js(array('statistics' => array('nid' => $node->nid)), 'setting');
+  }

This should use #attached.

Also if it's only logging on full node pages now, shouldn't it check the view mode?

timmillwood’s picture

Status: Needs work » Needs review
FileSize
11.27 KB

I have made sure the JS only gets added for 'full' nodes and also moved js to #attached.

lucascaro’s picture

Status: Needs review » Needs work

Looking good!
So there is no admin settings anymore, right?

a few minor details:

+++ b/core/modules/statistics/statistics.php
@@ -0,0 +1,33 @@
+  }    ¶

There is trailing space in here.

+++ b/core/modules/statistics/statistics.js
@@ -0,0 +1,12 @@
+(function ($) {
+  $(document).ready(function() {
+    var nid = Drupal.settings.statistics.nid;
+    var basePath = Drupal.settings.basePath
+    $.ajax({
+      type: "POST",
+      cache: false,
+      url: basePath+"core/modules/statistics/statistics.php",
+      data: "nid="+nid
+    });
+  });
+})(jQuery);

And maybe we need a @file comment here (as per #138 )

cheers

timmillwood’s picture

Status: Needs work » Needs review
FileSize
11.27 KB

I have removed the extra white space in statistics.php.

lucascaro, nod_ and I discussed the JS @file comment on IRC and decided it was best not to add comments as it is not yet a standard and only adds to the size of the JS file being loaded.

lucascaro’s picture

Status: Needs review » Reviewed & tested by the community

looks good

catch’s picture

Status: Reviewed & tested by the community » Fixed

OK this looks good now. committed/pushed to 8.x, thanks!

tstoeckler’s picture

Status: Fixed » Needs review
+++ b/core/modules/statistics/statistics.js
@@ -0,0 +1,12 @@
+      url: basePath+"core/modules/statistics/statistics.php",

This makes it impossible to override statistics.module from contrib.

Marking "needs review" at least for that detail.

timmillwood’s picture

Status: Needs review » Fixed

@tstoeckler: because this patch has now been committed, please open a new issue about this.

timmillwood’s picture

Version: 8.x-dev » 7.x-dev
Status: Fixed » Needs work

I am new moving this issue to D7 to work on back porting this patch.

wiifm’s picture

As module maintainer for statistics_ajax, I applaud you @timmillwood for spending so much time getting this into core!

Are you okay with the backport of this code to D7? i can help out if you have not already started ;)

timmillwood’s picture

@wiifm: I am hoping this D8 patch will just work in Drupal 7, the only difference is the file paths. If you are able to test the patch with D7 before I can that would be great.

wiifm’s picture

Status: Needs work » Needs review
FileSize
11.77 KB

Attached is #152 ported to D7 (verbatim with path changes etc). Initial testing showed the statistics updating the {node_counter} table as expected. I just hope the test bot is happy as well

wiifm’s picture

It seems my editor also stripped a few bonus trailing spaces as well. Hope this is okay still

Status: Needs review » Needs work

The last submitted patch, statistics-ajax-1209532-160.patch, failed testing.

RobLoach’s picture

tstoeckler’s picture

While I find it weird to open a follow-up for a regression that was introduced ~2 sec ago :), I don't really care: #1549064: Statistics module is not overridable by contrib

timmillwood’s picture

The patch from #160 works fine for me, therefore there might be an issue in the test.
Looking into it.

timmillwood’s picture

It looks like the statistics.test patch doesn't work right, the rest of the patch is ok.

The D7 test should be ok, just needs the drupal_http_request() added.

wiifm’s picture

I must confess, the D8 patch did not apply cleanly to the D7 .test file so I did manually patch it, from the testbot failures they should be easy to fix up.

wiifm’s picture

Status: Needs work » Needs review
FileSize
12.72 KB

New patch that hopefully passes all the tests

Status: Needs review » Needs work

The last submitted patch, statistics-ajax-1209532-168.patch, failed testing.

wiifm’s picture

Okay, another try

wiifm’s picture

Status: Needs work » Needs review

setting status

timmillwood’s picture

Looks good, I will try to give it a test today.

timmillwood’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me!

timmillwood’s picture

#170: statistics-ajax-1209532-170.patch queued for re-testing.

webchick’s picture

this looks great!! however, i think i'm going to hold it until next release just to be on the safe side. ideally this would get more than 2 days' testing before being available in the wild.

timmillwood’s picture

Perfect, I would be a bit nervous putting it in with only 2 day to go as well.

RobLoach’s picture

We could open up http://drupal.org/project/statisticstng or something similar.

timmillwood’s picture

there are already contrib modules that do similar, would be great to have it in core, wouldn't it?

xmacinfo’s picture

@Rob Loach: There is no need to create a contrib project. webchick only suggests to wait until after the next Drupal point release before committing this patch. She left it at RTBC. :-)

timmillwood’s picture

Ready for a D7 commit? It'd be great to get some eyes on this before the next release.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs work

This looks like a great improvement, and I think we should get it into Drupal 7 (it's really a borderline bugfix, even).

However, for Drupal 7, I don't think we can make this the default behavior; rather, it should be opt-in. That is per the backport policy at http://drupal.org/node/1527558, but also, in this specific case, I'm imagining a live site running behind Varnish with the Statistics module already enabled (yes, it doesn't work correctly in this setup, but that doesn't mean people don't have it turned on). After the next version of Drupal is released with this patch, every request (to a node page) that hits Varnish is now suddenly going to trigger a direct AJAX call to the Drupal site also, right? That is the kind of thing that can bring down web servers, so I don't think we can spring it on people by default.

The good news is, I don't think it would be too hard to modify the patch to add a variable which controls whether the old or new behavior is used. And given that the statistics module admin page is not exactly a heavily-trafficked part of the Drupal UI, we could probably get away with adding a new checkbox on that page in Drupal 7. Does that make sense?

In addition, some other issues with the patch:

  1. +      url: basePath+"modules/statistics/statistics.php",
    

    I think we should address #1549064: Statistics module is not overridable by contrib before getting this patch in Drupal 7. I don't know if it's actually an important issue or not, but I don't want to find out later that it actually was :)

    And it's hopefully a really easy fix; can't we just call url() on the PHP side, then add a JavaScript setting that contains the final URL? I think that's the standard way of doing this, and it would make the code cleaner also.

  2. -    if (arg(0) == 'node' && is_numeric(arg(1)) && arg(2) == NULL) {
    -      // A node has been viewed, so update the node's counters.
    ...
     function statistics_node_view($node, $view_mode) {
    +  if (!empty($node->nid) && $view_mode == 'full') {
    

    This looks like an unintended (?) behavior change to me. The view mode can certainly be 'full' even if you aren't on the node/$nid page (think Views). I think you need to check node_is_page($node) also; see examples elsewhere in core. (And perhaps you need to check empty($node->in_preview) too, although I'm not sure about that one.)

  3.  function statistics_node_view($node, $view_mode) {
    ...
    +    $node->content['#attached']['js'] = array(
    +      drupal_get_path('module', 'statistics') . '/statistics.js' => array(
    +        'scope' => 'footer'
    +      ),
    +    );
    

    This looks like it will blow away any JavaScript that was attached by an earlier hook implementation.

Also, a couple more things that aren't quite bugs, but seemed curious:

  1. +  $(document).ready(function() {
    

    @catch asked this above and I'm not sure I saw a response; why isn't this using Drupal.behaviors?

  2. +    // Manually calling statistics.php, simulating ajax behavior.
    +    $nid = $node->nid;
    +    $post = http_build_query(array('nid' => $nid));
    +    $headers = array('Content-Type' => 'application/x-www-form-urlencoded');
    +    global $base_url;
    +    $stats_path = $base_url . '/' . drupal_get_path('module', 'statistics'). '/statistics.php';
    +    drupal_http_request($stats_path, array('method' => 'POST', 'data' => $post, 'headers' => $headers, 'timeout' => 10000));
    

    This type of code is found throughout the tests. Is there a reason it can't use $this->drupalPost()? If it can, that would be a lot simpler.

David_Rothstein’s picture

Version: 7.x-dev » 8.x-dev

Several of the issues above affect Drupal 8 also, so I'm moving this back there for now.

nod_’s picture

just replying for 4. It's because you want to log the view on page load, not on every ajax call that can be triggered afterwards (which is what Drupal.behaviors will do). Behaviors are not well suited for this kind of things: we'd be adding 4-something lines of code to work around what behaviors do, might as well not use it and make the code simpler.

droplet’s picture

- What is the point of use AJAX ? Better Permanence? If DB under high load, it adds more overhead. (to both PHP server and HTTP server)
- #1684976: Better validation for statistics.php
- should not use db_merge. It can fake the script to accept any NID. even node is not existence. (myself consider its a security issue)

nod_’s picture

AJAX if for having relevant stats when you're using something like varnish or even the drupal cache for anonymous pages. That's pretty much the only simple and reliable way to go about that. You can't really avoid the overhead, usually google analytics takes it.

The issues with the code are valid though.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
1.67 KB

Here's a bunch of changes based on David_Rothstein's suggestions.

tstoeckler’s picture

Status: Needs review » Needs work
+++ b/core/modules/statistics/statistics.module
@@ -102,13 +102,12 @@ function statistics_permission() {
+  if (!empty($node->nid) && $view_mode == 'full' && node_is_page($node) && empty($node->in_preview)) {

I think node_is_page() already contains a check for $view_mode == 'full' (not sure, though). Maybe we want to leave this check specifically anyway, if we specifically want to check for that view mode, but I don't think so.

+++ b/core/modules/statistics/statistics.module
@@ -102,13 +102,12 @@ function statistics_permission() {
+    $settings = array('nid' => $node->nid, 'url' => url('core/modules/statistics/statistics.php'));

Since we now moved the path to PHP can we use drupal_get_path() here? That would solve #1549064: Statistics module is not overridable by contrib

Marking needs work for the second item.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
1.67 KB

Makes sense!

tstoeckler’s picture

timmillwood’s picture

Status: Needs review » Reviewed & tested by the community
David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs work

I think the $view_mode == 'full' check is actually necessary here, since the same node can easily be displayed twice on a page (e.g., the primary view plus a sidebar view). Anyway, every other call to node_is_page() in core checks $view_mode also (yes, perhaps there should be a helper function for that), so no reason to be different here :)

+      'scope' => 'footer'

While rerolling, might as well add a comma at the end of this line (per coding standards).

timmillwood’s picture

Status: Needs work » Needs review
FileSize
1.69 KB

@david_rothstein, thanks for the feedback, rerolled.

nod_’s picture

Any reason to hardcode the nid parameter instead of just putting data: Drupal.settings.statistics? and filter the data in the PHP side of things?

timmillwood’s picture

Drupal.settings.statistics would make it more flexible wouldn't it?

nod_’s picture

yep, jquery takes care of building the post string from the object too.

timmillwood’s picture

FileSize
1.74 KB

Adding in suggestion from nod_

mikeytown2’s picture

Coming from #1407524-2: Front End Speed Tweak: Speed up $.post() callback by returning before booting up drupal we can speed up the code a little bit. Patch attached to do that.

jcisio’s picture

About #197: it is ok for D7, but I doubt it is suitable for D8. I think statistics.php will disappear, we'll use the kernel. However I don't know how we apply this trick into Drupal's HttpKernel.

timmillwood’s picture

FileSize
1.76 KB

moved the nid to a data in the JS rather than sending all Drupal.settings.statistics.

timmillwood’s picture

Status: Needs review » Reviewed & tested by the community
catch’s picture

Issue tags: -Needs backport to D7

#199: 1209532-199.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs backport to D7

The last submitted patch, 1209532-199.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
1.53 KB

nod_ has been making awesome JS changes again, but these caused my patch not to work, here's a new one.

nod_’s picture

haha sorry about that. Patch looks good :)

timmillwood’s picture

Status: Needs review » Reviewed & tested by the community

Marking as RTBC then!

nod_’s picture

Status: Reviewed & tested by the community » Needs work

There is just one extra "," after Drupal.settings.statistics.data in statistics.js though, beside that it's RTBC I agree :)

timmillwood’s picture

Status: Needs work » Needs review
FileSize
1.53 KB

Removed extra comma

timmillwood’s picture

Status: Needs review » Reviewed & tested by the community

Passed tests so marking at RTBC

webchick’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Gosh, I'm really sorry. I have NO idea why this was off my radar for so long. :(

I confess that I don't fully grok this, but it has the +1 of our JS maintainer, so I guess it's good to go. :)

Committed and pushed to 8.x. Thanks! Marking for D7 backport, although it's worth checking w/ David to find out how much of this is actually backportable.

timmillwood’s picture

@webchick - this patch is really just a few improvements over the original patch which had been committed. No problems with the delay getting this committed, thanks!

Time to work on the D7 version now.

David_Rothstein’s picture

Since @webchick asked, I'll just point out #181 again... In my opinion, this patch should be backportable as long as in Drupal 7 it's not the default behavior.

webchick’s picture

Oops. That'll learn me to read things before asking dumb questions. Thanks, David!

mikeytown2’s picture

What about getting #197 in D8 so we can get it in D7?

iamEAP’s picture

For those uninterested in reading through the full history of this ticket, a D7 backport needs to comprise both #152 and #207 with a checkbox and variable to opt-in to (enable) the new functionality (#181).

Mike, sounds like you might be better off creating a followup task.

iamEAP’s picture

Status: Patch (to be ported) » Needs review
FileSize
6.38 KB

Test changes made for D8 are largely irrelevant, since the AJAX setting is opt-in for D7. Instead, I just added additional tests:

  • Ensure that node hits aren't counted via AJAX when AJAX is disabled.
  • Ensure that node hits are counted via AJAX when AJAX is enabled.

As per #181, I added a variable and checkbox on the admin form. As David pointed out, this breaks string freeze, but I'll leave it up to him to decide if that's acceptable. Certainly open to feedback on the wording, there.

Added a variable delete in statistics.install for the new variable. Believe once this gets committed, we should add an update function in 8.x to remove the new variable, since it has no meaning there.

Status: Needs review » Needs work

The last submitted patch, drupal-1209532-statistics_via_ajax-215.patch, failed testing.

iamEAP’s picture

Status: Needs work » Needs review
FileSize
6.56 KB

Was not reading the tests very well when I wrote them.

Status: Needs review » Needs work
Issue tags: -Needs backport to D7

The last submitted patch, drupal-1209532-statistics_via_ajax-217.patch, failed testing.

iamEAP’s picture

Status: Needs work » Needs review
iamEAP’s picture

mikeytown2’s picture

@iamEAP
Checkout returning data to browser ASAP in #197. Reports of the browser taking 90ms to get URL without patch, 65ms with patch. I have this logic in a function httprl_background_processing(). The main difference between the patch in #197 and httprl_background_processing is the call to fastcgi_finish_request() which is available on php-fpm only.

David_Rothstein’s picture

@mikeytown2: Sounds like that should be a separate issue?

Also, I remember discussing that kind of technique extensively in #566494: Running cron in a shutdown function breaks the site a long time ago... it looked promising, but as I recall there were many webserver setups where it didn't work correctly. If it can be made to work widely, though, it's pretty neat.

tcmug’s picture

I wonder if this is kind approach is only of superficial on the server level as the speed is only visible to the user/browser; wont the child process left running the PHP after the connection is closed still take up a connection slot?

If the connection is closed in a totally awesome 10 ms (instead of the usual 500 ms) the server is still busy for the entire 500 ms and unable to process any requests -this sounds kind of bad to my ear.

jcisio’s picture

#223 but both (client and server) don't have to manage that connection, it will free more resource.

tcmug’s picture

Well the remaining PHP after connection closing still has to be executed so ofcourse it keeps the child process executing (atleast apache). nginx might free the actual nginx process but php-fpm process keeps executing and keepin it occupied.

sdrycroft’s picture

Status: Needs review » Needs work

As far as I can see, there are currently two issues with the patch from #217:

  1. When the function conf_path() attempts to calculate the path to the site's settings folder (usually something like "sites/example.com") it includes "modules/statistics", resulting in a path like "sites/example.com.modules.statistics".
  2. getcwd() when executed in the statistics.php file may not give the correct directory path if using symlinks. For example, we have a number of Drupal platforms on a single server. Each platform shares a common Drupal code base, but has a different sites folder. e.g.
    /usr/share/drupal-7
    index.php
    modules
    includes
    ...
    
    /var/www/web_app_1
    index.php -> /usr/share/drupal-7/index.php
    modules -> /usr/share/drupal-7/modules
    ...
    sites -> /var/lib/drupal-7/web_app_1
    
    /var/www/web_app_2
    index.php -> /usr/share/drupal-7/index.php
    ...
    sites -> /var/lib/drupal-7/web_app_2
    

    When executing getcwd() from /var/www/web_app_2/index.php /usr/share/drupal-7 is returned. When executing it from /var/www/web_app_2/modules/statistics/statistics.php /var/www/web_app_2/modules/statistics is returned and not /usr/share/drupal-7/modules/statistics.

I am aware that the second issue could easily be resolved by symlinking to files only, but it's still an issue that we need to be aware of.

mikeytown2’s picture

Title: Improve statistics module for D8? » Count node views via AJAX in the statistics module
Version: 8.x-dev » 7.x-dev
Assigned: Unassigned » timmillwood
Status: Active » Needs work
Issue tags: +Needs backport to D7

getcwd() could be worked around if using PWD. PWD doesn't work on windows but it's not an issue due to symlinks on windows being 100 times harder to setup.
Check both:
$_SERVER['PWD']
$_ENV['PWD']

Background info #1878454-22: When does something happen?

sdrycroft’s picture

Status: Needs work » Needs review
FileSize
6.66 KB

$_SERVER['PWD'] is only available when executing PHP from the command line, so that solution will not work. I have tweaked iamEAP's #217 patch to attempt to calculate the correct DRUPAL_ROOT using $_SERVER['SCRIPT_FILENAME'].

JeremyFrench’s picture

Patch in #228 applies and works for me behind varnish.

davidwhthomas’s picture

This is a great enhancement to the core statistics module, allowing statistics to work well on cached pages.

I'm successfully using the D7 patch in #228

Here's a code snippet for anyone looking how to get it working when using Panels for node_view.

There's probably a few ways to do it, I was hoping to use #attached somewhere here, but this works:

/**
 * Implements hook_ctools_render_alter().
 */
function MODULE_ctools_render_alter(&$info, &$page, &$context) {

  // Attach AJAX node count statistics to page if configured.
  if (isset($context['task']['name']) && $context['task']['name'] == 'node_view' && $page) {
    if (variable_get('statistics_count_content_views', 0) && variable_get('statistics_count_content_views_ajax', 0)) {
      if ($node = reset($context['contexts'])->data) {
        if (!empty($node->nid) && empty($node->in_preview)) {
          drupal_add_js(
              drupal_get_path('module', 'statistics') . '/statistics.js', 
              array('scope' => 'footer')
          );
          $settings = array('data' => 
                                array('nid' => $node->nid), 
                                'url' => url(drupal_get_path('module', 'statistics') . '/statistics.php')
                              );
          drupal_add_js(
              array('statistics' => $settings),
              array('type' => 'setting')
          );
        }
      }
    }
  }

}
Rosamunda’s picture

Is #228 commited?

JordanMagnuson’s picture

I'm also wondering if this has been/will be committed?

iamEAP’s picture

Assigned: timmillwood » Unassigned
Issue summary: View changes

Re #231, 232: This will only be considered for commit once it's marked "reviewed and tested by the community." If you've deployed this patch, or have otherwise tested, please feel free ti chime in and/or flip that issue status.

I see #229 and #230 have positive reviews.

Also, un-assigning Tim so this is seen more widely.

sdrycroft’s picture

We're currently using this patch on approximately 500 sites, and have not experienced any issues.

JordanMagnuson’s picture

Status: Needs review » Reviewed & tested by the community

@sdrycroft: That sounds pretty RTBC, then.

David_Rothstein’s picture

Title: Count node views via AJAX in the statistics module » Change notice: Count node views via AJAX in the statistics module
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active
Issue tags: +7.25 release notes, +7.25 release blocker, +Needs change record

Looks great now. Committed to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/1bcc0e3

I did change "AJAX" to "Ajax" (especially in the user-facing text) on commit, to match coding standards.

Ideally I wouldn't have committed this so close to the next release (due to the string change), but this patch sat for a while and it's new translatable strings only (not breaking any existing ones), plus buried deep on an administrative page, so I think it's OK.

I'm not sure how much the new checkbox description will really help people with existing sites decide whether to turn this on or not, but we need a change notification anyway so that'll help too.

SocialNicheGuru’s picture

I can only select Use 'AJAX to increment the counter' if I have 'Do not collect content hit statistics'.

do I do that then select one of the other options?

Edit: this is caused by better_statistics module.

iamEAP’s picture

Status: Active » Needs review

Added a change notice here: https://drupal.org/node/2164069 Feedback welcome.

@SocialNicheGuru, you should only be able to select the Ajax collection method when "Count content views" is also checked.

David_Rothstein’s picture

Looks great, thanks!

The one thing I thought was missing was information about how this affects sites using Varnish (or similar external caches). Because those are the sites that can get the greatest benefit from this new option, but also the greatest risk. So I added a paragraph about that now: https://drupal.org/node/2164069/revisions/view/6801571/6804873

If that looks good to others, I think we can close this issue.

iamEAP’s picture

Title: Change notice: Count node views via AJAX in the statistics module » Count node views via AJAX in the statistics module
Priority: Critical » Normal
Status: Needs review » Fixed
Issue tags: -Needs backport to D7, -Needs change record

Believe that covers it; marking fixed and reverting back to prior issue details. Keeping 7.25 release notes and 7.25 release blocker because I'm unsure of their meaning / intent.

iamEAP’s picture

Looks like this wasn't quite ready for prime-time; if you have multiple languages enabled and you're using path-based language negotiation, no statistics are collected for languages with configured path prefixes. It also generates a 404 log entry for each failed attempt.

The data collection callback should not be passed through url(). This issue exists in D8 too. I've created an issue here and tagged it as "needs backport." #2172871: Async node hit counter data collection is broken when path-based language negotiation is used

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

math_1048’s picture

That's Great, Thanks for you,
What about to displaying updated {node_count} value using ajax ... my cache module is "BOOST"?
can you help me please in this issue
Thanks and regards

Andre-B’s picture

I guess quiet a lot of people might stumble in this as well:

I am using Panels based setup, where I have page overrides for various node pages, I use a different view mode than "full" on these variants - for those view modes the statistics are not counted, since it's only added on the full view mode - is this intended behavior?

WillowDigit’s picture

I have encountered a new issue in statistics.php file.

My statistics counters do not increment. I am running on IIS and the define at the top

define('DRUPAL_ROOT', substr($_SERVER['SCRIPT_FILENAME'], 0, strpos($_SERVER['SCRIPT_FILENAME'], '/modules/statistics/statistics.php')));

is not platform directory separator aware.

It should be something like:

PHP_OS == "Windows" || PHP_OS == "WINNT" ?
  define('DRUPAL_ROOT', substr($_SERVER['SCRIPT_FILENAME'], 0, strpos($_SERVER['SCRIPT_FILENAME'], '\\modules\\statistics\\statistics.php'))) :
  define('DRUPAL_ROOT', substr($_SERVER['SCRIPT_FILENAME'], 0, strpos($_SERVER['SCRIPT_FILENAME'], '/modules/statistics/statistics.php')))
mikeytown2’s picture

Normalizing the variable is a better option.

  $script_filename = str_replace('\\', '/', realpath($_SERVER['SCRIPT_FILENAME']));

Using DIRECTORY_SEPARATOR is another way.

david_garcia’s picture

Current implementation not working on IIS, I made a patch and tested based on @mikeytown2 proposal.

mikeytown2’s picture

Category: Task » Bug report
Status: Closed (fixed) » Needs review

Re-opening the issue because of the windows bug.

Edit: Looks like #247 passes the test bot.

David_Rothstein’s picture

Status: Needs review » Fixed

Let's move that to the separate issue which was filed for it - #2327953: statistics.php is not platform directory separator aware - since this issue was fixed long ago.

(I'll leave a comment there pointing to the patch.)

David_Rothstein’s picture

Category: Bug report » Task
timmillwood’s picture

Is this a bug on both the D7 and D8 versions of the code?

David_Rothstein’s picture

I didn't test, but I don't think so because D8 isn't doing anything with $_SERVER['SCRIPT_FILENAME'] in this file.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

ruloweb’s picture

ruloweb’s picture