Move logging to init hook

Makes sense to me. +1.

Looks good. Committed.

uh, it was in exit for a reason. the page has been printed to the browser by that point (i think). if the DB is clogged up, the page does not slow down. the statistics query is an INSERT or UPDATE and not SELECT and thus has a much greater chance of slowing ... i don't ever enable statistics module so i don't care that much, but FYI.

I think the cahnce of slowing a page delivery down in an unacceptable manner is negligible.

It would be nice if we had a db_insert function with an optional "delayed" argument, so we could make use of this mysql specific feature for this (and other) queries.

also, we have now introduced an expensive query into the delivery of a cached page. the bootstrap takes great pains to avoid queries, especially writes. the only write during a cached page view is the one at the end of sess_write() [see http://drupal.org/node/40545], and i want to obsolete that one too someday.

one might argue that an admin who cares can just disable the stats module. thats true, but he loses ALL insight whereas before this patch he could comfortably collect stats for non cached pages without slowing down the cached ones.

enabling statistics module is now even more costly that it used to be.

This patch broke the accuracy of the page generation timer.

Now that we are doing the insert in the init hook, the timer column does not contain what it purports to contain, that is, the page generation time. Instead, it only records the time from the start of the page timer to the execution of the init hook. That is not helpful at all.

I propose we roll this patch back and continue to do logging in the exit hook until a better solution can be found.

The alternative, to update the page timer in the exit hook, is untenable because
- we have no handle on which row we inserted
- which means we need either an expensive query or another column with a db_next_id (even more expensive!)

Setting to critical because as it stands, statistics reporting is broken.

Here's a compromise solution. Node counter statistics are done in the init hook (so activity will be reflected even if the exit hook is not reached) but the accesslog is done in the exit hook, so that timers will be accurate.

Should be statisticS_exit(), no?

sigh - the compromise solution adds *another* query to each cached page. i think that rollback is the right move. that way we have one query that happens after the page is delivered. we should not sacraifice performance because someone somehow requested a lot of pages and drupal didn't notice. this abuse can be monitored at other layers of the stack.

Im going to have to +1 moshe's last comment.

I am not really interested in statistics that tell me a page was *HALF VISITED* by somebody. If it's not a full page load, don't bother logging it because it has no statistical relevance.

What you experienced on Drupal.org should be monitored with your server logs until a better solution can be engineered for this. However, the bottom line remains - only record full page loads for statistical relevancy.

I agree that this patch needs to be rolled back. If logging is critical for you, use logging at the Apache level or patch core. For the majority of the people, performance is more important than logging accuracy.

Rollback patch anyone?

We should also review (remove?) the bootstrap code in that _init hook.

The rollback patch is in #8.

The bootstrap code was introduced in 44828.

The execution of the bootstrap code involves include_once'ing path.inc and checking if $_GET['q'] is already defined. So, no big deal.

From http://us2.php.net/manual/en/features.connection-handling.php

If you do not tell PHP to ignore a user abort and the user aborts, your script will terminate. The one exception is if you have registered a shutdown function using register_shutdown_function().

I think this may be the way to go. Seems to always execute and is at the end.

Neil: maybe we should refactor the _exit hook to be called from such a function but for now, I'd go with a regular _exit().

jvandyk: if you change that to _exit(), we might no longer need the drupal_bootstrap(DRUPAL_BOOTSTRAP_PATH) code that is part of _init(). I believe that code was added to make the logging work on cached pages. Do we still need that in _exit()? Chances are we still need it, but I think we should double check. :)

http://drupal.org/node/102526 points out that stat being in init causes sometimes to menu fire before init. This must not happen, this breaks some contirb modules in extremely subtle ways. As we have some issues that are deemed critical but they are not -- I would like to point out that this one IS a release blocking issue: it is not acceptable for hooks to change behaviour just because you switch on statistics.

I will work on the issue later if needed.

I guess I'll do it.

Here is a patch. Explanation is in code comments and/or #16 here.

nice neil.

BOOTSTRAP_PATH is still needed.

statistics module is still a real drain and i wish drupal.org didn't run it. the DB melted down today and accesslog was implicated. turning this on is dumb considering all the optimizing i did in sess_write() to avoid a write for crawlers.

I would just move that code to _exit() for now, and rework _exit() in D6.

Committed John's patch.

Optimization is always good. What about we add a settings here (maybe 'log crawlers') which would trigger a check on cookie being set?

Between Moshe's comment I read in the drupal issues mail and my answer to it the issue was suddenly fixed which is actually good but Moshe's comment is a real gem, so I reopen this.