API - Dealing with statistics data collection on cached page requests

After some trials, I tend to think that this case requires dedicated focus while developing the integration plugin.

Out of experience with two modules I developed some integration for, 'Browscap' and 'IP Geolocation Views and Maps':

• drupal_get_path() won't work (see #1839090: Call to undefined function drupal_get_path())
• also, module_load_include() won't work
• so, to include module files you will need to use require_once dirname(__FILE__) . '/path/to/yourmodule.module';, and ensure that any chained inclusion follow the same logic (i.e. you cannot have a module_load_include() in the file you include through require_once...)
• file inclusion may not be enough - a hook_init implementation that would normally 'do something' and be triggered at module load, would need to be explicitly called by the plugin in case of cached page (this is the case for ip_geoloc)
• any dependency should be addressed manually (i.e. if a module's function depends on a function in another module, the latter would have to be manually loaded as well)

With this said, I believe options 2 and 3 above would fall short anyway (#2 because if developer is not checking all the possible implications, dependencies, etc, the entire call would fail, #3 because you would miss the function calls of hook_init). It is really a matter of developing the statistics plugin callback in such a manner that it can cope with cached page requests.

My ideal scenario is as follows (I do not know if this is all possible, but let's dream :)):

1. Make clear to developers they have to deal specifically with this case, if they want to (per #1 above).
2. In the API, add a parameter in hook_statistics_api() to declare if the module is capable or not of collecting data for cached page requests. By default, NOT. Better Statistics should use this to decide whether or not to call the callbacks of the module at runtime.
3. In the admin form, add a 'cached request' fieldset where an additional flag for each of the modules that declared themselves 'cache-ready' could be set/unset by an administrator. For performance reasons, you may want to disable data collection in this case even if the module is capable of. (example - geolocation adds a heavy overhead that overcomes the benefit of caching the page)
4. In the admin form, again, and again for perfomance opportunities, have a general switch to switch on/off completely accesslog statistics for any request served from cache.

Cheers

I just realised an alternative could be to call drupal_bootstrap(DRUPAL_BOOTSTRAP_FULL); if we are serving a cached page. But that would slash any benefit of caching I believe.

EDIT:

However, calling that bootstrap phase during hook_exit can lead to nasty issues like

Warning: Cannot modify header information - headers already sent by (output started at /.../includes/bootstrap.inc:1364) in drupal_send_headers() (line 1216 of /.../includes/bootstrap.inc).

if the called module's hook is trying to write to the returned page (I guess that's because in a caching concern the request has been executed and returned already)...

I'd leave that to the plugin developer, in order not to load Better Statistics with unnecessary overload.

For instance, I ended up including that for the IP Geolocation plugin (it is working now), as one dependency requires also drupal_http_request()

    // If caching, the module will not be loaded. In such case,
    // ip_geoloc_get_visitor_location() will be undefined.
    if (function_exists('ip_geoloc_get_visitor_location')) {
      $geoloc = ip_geoloc_get_visitor_location(); // uses internal cache
    } 
    else {
      require_once DRUPAL_ROOT . '/includes/common.inc';
      $list = system_list('module_enabled');
      require_once DRUPAL_ROOT . '/' . $list['ip_geoloc']->filename;
      require_once DRUPAL_ROOT . '/' . $list['smart_ip']->filename;
      $geoloc = ip_geoloc_get_location_by_ip(ip_address(), FALSE, TRUE, FALSE, TRUE);
      if (!empty($geoloc) and (!isset($geoloc['provider']) or empty($geoloc['provider']))) {
        $geoloc['provider'] = 'ip_geoloc';
      }
    }

I tend to agree :), I am also proposing to change the issue title here.

Just a few free thoughts for your consideration, iamEAP:

1. I still believe that a general switch 'get statistics for requests served from cache yes/no' would make sense to admins, for general performance opportunity. Also it looks like in Drupal 8 cache/non-cache statistics collection may take fully separate paths (if hook_exit() is eventually dropped), so this 'main switch' could support activation (or not) of the cached functionality.
2. Is there a way that the information whether a page is served or not by cache is 'gathered' by Better Statistics and passed on to the implementing module? Just to avoid that each module get that info its own way (e.g. checking function_exists, headers, drupal_bootstrap), not to mention that bootstrap phase may be raised by a module in the process of data collection.
3. Last, currently in better_statistics_exit() the very first thing the routine is doing is raising bootstrap phase to DRUPAL_BOOTSTRAP_SESSION. I believe this is so to match the core function. But - is it really necessary for cached pages?

Maybe I am paranoid.... but theoretically better_statistics_served_from_cache() could be invoked before headers are fully sent, so freezing the value of the static variable in the function. I changed that to include a switch to force scanning the headers, and included a call at the beginning of better_statistics_exit() - so to make sure cache status returned is the one relevant at that point.

Maybe with that the following code could become conditional on $cache_status === TRUE??

    drupal_bootstrap(DRUPAL_BOOTSTRAP_SESSION);
    include_once DRUPAL_ROOT . '/includes/unicode.inc';

I prefer to have static as module plugins will likely call the function in the callbacks to determine how to return the field value.

So either patch in #9 or in #10 are ok with me. The #10 will just make sure that even if better_statistics_served_from_cache() is called before the headers are built, the status of headers at hook_exit is checked.