Posted by timmillwood on February 20, 2012 at 3:29pm
20 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | statistics.module |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
The accesslog in the statistics module isn't the most elegant thing.
What it does is, each page view it writes to the database these items:
- Page title
- Path
- url
- hostname (IP addess)
- uid
- Session id
- Page load time via timer_read
- timestamp
Many of these can be gathered by free external tools such as google analytics. These services often offer custom variable tracking, so a contrib module could add this functionality.
I will work on a patch to strip out the accesslog features from statistics module.
Comments
#1
I would still like to see this make it's way into Drupal 8 as I really feel the 'access log' gives little value, most of the value from the statistics module comes from the 'node counter'.
#2
Agreed. Let's support the basic use case of counters for all entity types, but anything advanced should probably not be for core.
#3
The node counter functionality of statistics is great for site builders / administrators (for, e.g., building a View of the most "popular" content).
While the access log is not immediately useful for the same audience, it's extremely useful (at least in my opinion) for developers.
#4
@iamEAP - I don't feel these useful items you mentioned really need to be in core and by moving them to contrib allows for better expansion on features and functionality. I really hope you could be one of many to help lead this effort with your already widely used contrib module.
Also to cover your last point about javascript. The node counter has already moved to JavaScript in Drupal 8 to allow it to give accurate results when using reverse proxy caching. If we were to keep the accesslog I would like to do the same with that and move it to JavaScript.
#5
I suppose accesslog isn't necessarily appropriate for core, especially with the direction node/entity statistics is going / has gone.
If you remove the accesslog, I'm happy to adopt its schema/table in an 8.x branch of my contrib module.
#6
Starting with removing tests
#7
The last submitted patch, 1446956-6.patch, failed testing.
#8
#9
So that's the tests sorted, now to do the rest.
#10
Removed the access log schema and settings form.
Little by little!
#11
Bad patch in #10
also removed most things now.
#12
The last submitted patch, 1446956-11.patch, failed testing.
#13
Yes please. See also #1860026: Remove hook_exit() for cached page requests.
#14
I am really keen to remove the accesslog, but there is a lot of code to remove and by the time I find it all, and test it all, core has changed so much that my patch doesn't work.
Changing the access log to use ajax is near impossible because of the kind of data it gathers, it would need to have a php built gif on the page that gathers the information much like Google analytics or other such tools.
#15
Just out of curiosity Tim, what advantage do you have in mind for a PHP-built gif over a regular AJAX request?
#16
Kicking this back off again. Re-roll of #11.
#17
#18
The last submitted patch, drupal-remove_accesslog-1446956-16.patch, failed testing.
#19
Missed a merge conflict in the re-roll. Also getting rid of another test that's not relevant anymore.
#20
Un-assigning Tim for wider review.
#21
Hi, I tested and it looks ok to me.
I just wonder whether we should have an explicit dp_drop_table for accesslog in the update hook?
Cheers
#22
Eric,
Thanks for looking into this. All looks good to me, I am going to send as re-test to the testbot now before marking RTBC.
Tim
#23
#19: drupal-remove_accesslog-1446956-19.patch queued for re-testing.
#24
Looking good!
Really excited to get this committed.
#25
+++ b/core/modules/statistics/lib/Drupal/statistics/Tests/StatisticsAdminTest.phpundefined
@@ -62,19 +62,13 @@ function setUp() {
- $this->assertFalse($config->get('count_content_views'), 'Count content view log is disabled by default.');
+ $this->assertFalse($config->get('count_content_views'), t('Count content view log is disabled by default.'));
...
- $this->assertTrue($config->get('count_content_views'), 'Count content view log is enabled.');
+ $this->assertTrue($config->get('count_content_views'), t('Count content view log is enabled.'));
+++ b/core/modules/statistics/lib/Drupal/statistics/Tests/StatisticsLoggingTest.phpundefined
@@ -81,10 +79,7 @@ function testLogging() {
+ $this->assertIdentical($this->drupalGetHeader('X-Drupal-Cache'), 'MISS', t('Testing an uncached page.'));
@@ -92,10 +87,7 @@ function testLogging() {
+ $this->assertIdentical($this->drupalGetHeader('X-Drupal-Cache'), 'HIT', t('Testing a cached page.'));
These shouldn't use t()
#26
Good catch, Berdir. Fixing those here.
@mondrake In past upgrade paths, I believe accesslog has been truncated (maybe I'm imagining this); though there've also been cases (in Taxonomy, for example) where a table's been left behind for contrib to pick up. In any event, I think it's fine to leave it for now and address it when the upgrade path is being worked on.
#27
OK, thanks
Good to leave it there for contrib to pick up.
#28
Now those lines are all unchanged. Looks good, was RTBC.
#29
Hm. Assigning to Dries, since this seems to remove basically all of the usefulness of this module.
#30
node counter is the most useful part of this module, it counts all node views allowing for "Most viewed content" views to be generated etc. The access log just duplicates what any analytics or server access log does. It offers little or no functionality just statistical data that is better sourced else where. Access log doesn't work with reverse proxy cache and adds extra database load.
#31
Yes, but I don't run external analytics due to privacy concerns, it is much easier to simply go to admin/reports than grep my Apache logs or install a third-party log visualization script, and I don't use a reverse proxy cache (along with 90%+ of the rest of the Drupal websites out there) so I don't care about that. :)
Anyway, let's see what Dries says.
#32
I don't know if accesslog should remain in core or if it should be decoupled from node counter functionality but for me and many others provides invaluable information.
Right now it's one of the bests sources of information for server side performance monitoring and tuning... so, please, if it's finally removed provide a clean path to a contrib alternative.
Best regards
#33
I would suggest a tool such as New Relic for performance monitoring and tuning, the accesslog in statistics module is going to cause more performance issues. You should see a faster site by disabling it.
#34
I understand the concerns mentioned by webchick and NITEMAN; I expressed similar concerns in #3.
I maintain Better Statistics in contrib. In D7, it support the default accesslog functionality, but also an API to allow collection of arbitrary data. On the roadmap is pluggable storage (e.g. a syslog for Statistics).
I could honestly go in either direction:
But as Tim Mentions now, it's not currently doing its job very well.
#35
This patch doesn't look ready yet. Some quick things I noticed:
name = Statisticsdescription = Logs access statistics for your site.
***
Also, I have no strong opinion on whether this should be in core, but it doesn't sound like there's a clear plan yet for maintaining this in contrib .... Is someone planning to maintain it as a standalone module?
I don't think it's accurate to say that the accesslog duplicates what analytics or server access logs do; maybe that's partially the case for anonymous users, but definitely not for authenticated users. The module provides deep integration of those statistics within your Drupal site itself. Example from the patch...
- $items['user/%user/track/navigation'] = array(- 'title' => 'Page visits',
- 'page callback' => 'statistics_user_tracker',
...
- * Page callback: Displays statistics for a user.
#36
Here's a patch that..
I mentioned earlier that I'd be willing to, at a minimum, maintain the functionality that we're ripping out here in the 8.x version of the Better Statistics module (I'd attempt to improve upon it there as well, obviously).
#37
Every measurement tool has its performance impact (¿Heisenberg?), acceslog doesn't impact so much in a well configured dedicated InnoDB.
I know New relic... but IMHO we can't seriously solve everything using external black box services.
Best Regards
#38
Lets get this done, we have a critical waiting on this..anyone who wants this functionality in d8 will be able to find it in contrib in Better Statistics module
#39
NewRelic has nothing to do with this issue.
If you want performance measurement of page loads from within Drupal you can use http://drupal.org/project/performance http://drupal.org/project/xhprof and others.
#40
I agree that this no longer is core worthy so I committed this to 8.x. Not to mention it is often a source of problems. I do think we should add a 'CHANGELOG.txt' entry so I'm marking it 'needs work' on that basis.
#41
CHANGELOG.txt entry, per Dries' note. Also made a change record: http://drupal.org/node/1900384
#42
Don't we need a changelog entry for that?
#43
@dawehner: what do you mean in #42? Do we now have some other process for making changelog.txt changes other than submitting a patch as #41 has done?
#44
Yeah good question what I talked about, missed somehow the link on http://drupal.org/node/1900384 . I'm sorry.
#45
#41 looks good. Thanks.
#46
Committed/pushed the follow-up, thanks!
#47
Automatically closed -- issue fixed for 2 weeks with no activity.