Issue #1733674 by markpavlitski, marcingy, and Jeremy: only collect statistics...

Issue #1733674 by markpavlitski, marcingy, and Jeremy: only collect statistics if enabled, disable by default.

dmemcache.inc

@@ -35,7 +35,9 @@ $_memcache_statistics = array();
 function dmemcache_set($key, $value, $exp = 0, $bin = 'cache', $mc = NULL) {
   global $_memcache_statistics;
   $full_key = dmemcache_key($key, $bin);
-  $_memcache_statistics[] = array('set', $bin, $full_key, '');
+  if (dmemcache_collect_stats()) {
+    $_memcache_statistics[] = array('set', $bin, $full_key, '');
+  }
   if ($mc || ($mc = dmemcache_object($bin))) {
     if ($mc instanceof Memcached) {
       return $mc->set($full_key, $value, $exp);
@@ -71,7 +73,9 @@ function dmemcache_set($key, $value, $exp = 0, $bin = 'cache', $mc = NULL) {
 function dmemcache_add($key, $value, $exp = 0, $bin = 'cache', $mc = NULL, $flag = FALSE) {
   global $_memcache_statistics;
   $full_key = dmemcache_key($key, $bin);
-  $_memcache_statistics[] = array('add', $bin, $full_key, '');
+  if (dmemcache_collect_stats()) {
+    $_memcache_statistics[] = array('add', $bin, $full_key, '');
+  }
   if ($mc || ($mc = dmemcache_object($bin))) {
     if ($mc instanceof Memcached) {
       return $mc->add($full_key, $value, $exp);
@@ -102,8 +106,10 @@ function dmemcache_get($key, $bin = 'cache', $mc = NULL) {
     $php_errormsg = '';
 
     $result = @$mc->get($full_key);
-    $statistics[] = (bool) $result;
-    $_memcache_statistics[] = $statistics;
+    if (dmemcache_collect_stats()) {
+      $statistics[] = (bool) $result;
+      $_memcache_statistics[] = $statistics;
+    }
 
     if (!empty($php_errormsg)) {
       register_shutdown_function('watchdog', 'memcache', 'Exception caught in dmemcache_get: !msg', array('!msg' => $php_errormsg), WATCHDOG_WARNING);
@@ -129,7 +135,9 @@ function dmemcache_get_multi($keys, $bin = 'cache', $mc = NULL) {
   $statistics = array();
   foreach ($keys as $key => $cid) {
     $full_key = dmemcache_key($cid, $bin);
-    $statistics[$full_key] = array('getMulti', $bin, $full_key);
+    if (dmemcache_collect_stats()) {
+      $statistics[$full_key] = array('getMulti', $bin, $full_key);
+    }
     $full_keys[$cid] = $full_key;
   }
   $results = array();
@@ -150,9 +158,11 @@ function dmemcache_get_multi($keys, $bin = 'cache', $mc = NULL) {
       ini_set('track_errors', $track_errors);
     }
   }
-  foreach ($statistics as $key => $values) {
-    $values[] = isset($results[$key]) ? '1': '0';
-    $_memcache_statistics[] = $values;
+  if (dmemcache_collect_stats()) {
+    foreach ($statistics as $key => $values) {
+      $values[] = isset($results[$key]) ? '1': '0';
+      $_memcache_statistics[] = $values;
+    }
   }
 
   // If $results is FALSE, convert it to an empty array.
@@ -180,7 +190,9 @@ function dmemcache_get_multi($keys, $bin = 'cache', $mc = NULL) {
 function dmemcache_delete($key, $bin = 'cache', $mc = NULL) {
   global $_memcache_statistics;
   $full_key = dmemcache_key($key, $bin);
-  $_memcache_statistics[] = array('delete', $bin, $full_key, '');
+  if (dmemcache_collect_stats()) {
+    $_memcache_statistics[] = array('delete', $bin, $full_key, '');
+  }
   if ($mc || ($mc = dmemcache_object($bin))) {
     return $mc->delete($full_key, 0);
   }
@@ -199,7 +211,9 @@ function dmemcache_delete($key, $bin = 'cache', $mc = NULL) {
  */
 function dmemcache_flush($bin = 'cache', $mc = NULL) {
   global $_memcache_statistics;
-  $_memcache_statistics[] = array('flush', $bin, '', '');
+  if (dmemcache_collect_stats()) {
+    $_memcache_statistics[] = array('flush', $bin, '', '');
+  }
   if ($mc || ($mc = dmemcache_object($bin))) {
     return memcache_flush($mc);
   }
@@ -461,3 +475,33 @@ function dmemcache_key($key, $bin = 'cache') {
 
   return $full_key;
 }
+
+
+/**
+ * Checks whether memcache stats need to be collected.
+ */
+function dmemcache_collect_stats() {
+  global $user;
+  static $drupal_static_fast;
+  if (!isset($drupal_static_fast)) {
+    $drupal_static_fast = &drupal_static(__FUNCTION__, array('variable_checked' => NULL, 'user_access_checked' => NULL));
+  }
+  $variable_checked = &$drupal_static_fast['variable_checked'];
+  $user_access_checked  = &$drupal_static_fast['user_access_checked'];
+
+  // Confirm DRUPAL_BOOTSTRAP_VARIABLES has been reached. We don't use
+  // drupal_get_bootstrap_phase() as it's buggy. We can use variable_get()
+  // here because _drupal_bootstrap_variables() includes module.inc
+  // immediately after it calls variable_initialize().
+  if (!isset($variable_checked) && function_exists('module_list')) {
+    $variable_checked = variable_get('show_memcache_statistics', FALSE);
+  }
+  // If statistics are enabled we need to check user access.
+  if (!empty($variable_checked) && !isset($user_access_checked) && !empty($user) && function_exists('user_access')) {
+    // Statistics are enabled and the $user object has been populated, so check
+    // that the user has access to view them.
+    $user_access_checked = user_access('access memcache statistics');
+  }
+  // Return whether or not statistics are enabled and the user can access them.
+  return (!isset($variable_checked) || $variable_checked) && (!isset($user_access_checked) || $user_access_checked);
+}

memcache_admin/memcache_admin.install

@@ -11,3 +11,10 @@ function memcache_admin_update_7001() {
   drupal_flush_all_caches();
   menu_rebuild();
 }
+
+/**
+ * Implements hook_uninstall().
+ */
+function memcache_admin_uninstall() {
+  variable_del('show_memcache_statistics');
+}

memcache_admin/memcache_admin.module

@@ -104,9 +104,10 @@ function memcache_admin_menu() {
  * Settings form.
  */
 function memcache_admin_admin_settings() {
-  $form['show_memcache_statistics'] = array('#type' => 'checkbox',
+  $form['show_memcache_statistics'] = array(
+    '#type' => 'checkbox',
     '#title' => t('Show memcache statistics at the bottom of each page'),
-    '#default_value' => variable_get('show_memcache_statistics', 1),
+    '#default_value' => variable_get('show_memcache_statistics', FALSE),
     '#description' => t("These statistics will be visible to users with the 'access memcache statistics' permission."),
   );
   return system_settings_form($form);
@@ -434,7 +435,7 @@ function memcache_admin_stats_types($bin) {
     return array();
   }
 }
- 
+
 function theme_memcache_admin_stats_raw_table($variables) {
   $cluster = $variables['cluster'];
   $server = $variables['server'];
@@ -549,7 +550,7 @@ function memcache_admin_shutdown() {
     }
   }
 
-  if (variable_get('show_memcache_statistics', TRUE) && function_exists('user_access') && user_access('access memcache statistics')) {
+  if (variable_get('show_memcache_statistics', FALSE) && function_exists('user_access') && user_access('access memcache statistics')) {
     if (!empty($_memcache_statistics)) {
       foreach ($_memcache_statistics as $row => $stats) {
         $_memcache_statistics[$row][1] = check_plain($stats[1]);

tests/memcache.test

@@ -584,3 +584,116 @@ class MemCacheRealWorldCase extends DrupalWebTestCase {
     return $item;
   }
 }
+
+/**
+ * Test statistics generation.
+ */
+class MemCacheStatisticsTestCase extends MemcacheTestCase {
+
+  public static function getInfo() {
+    return array(
+      'name' => 'Statistics tests',
+      'description' => 'Test that statistics are being recorded appropriately.',
+      'group' => 'Memcache'
+    );
+  }
+
+  function setUp() {
+    parent::setUp('memcache_admin');
+
+    $conf['cache_default_class'] = 'MemCacheDrupal';
+    $conf['cache_class_cache_form'] = 'DrupalDatabaseCache';
+  }
+
+  /**
+   * Checks for early bootstrap statistics.
+   *
+   * Tests that bootstrap cache commands are recorded when statistics are
+   * enabled and tests that statistics are not recorded when the user doesn't
+   * have access or displaying statistics is disabled.
+   */
+  function testBootstrapStatistics() {
+    global $_memcache_statistics;
+    // Expected statistics for cache_set() and cache_get().
+    $test_full_key = dmemcache_key($this->default_cid, $this->default_bin);
+    $expected_statistic_set[] = array('set', $this->default_bin, $test_full_key, '');
+    $expected_statistic_get[] = array('get', $this->default_bin, $test_full_key, 1);
+
+    // List of bootstrap cids to check for.
+    $cache_bootstrap_cids = array('variables', 'bootstrap_modules', 'lookup_cache', 'system_list', 'module_implements');
+
+    // Turn on memcache statistics.
+    variable_set('show_memcache_statistics', TRUE);
+    drupal_static_reset('dmemcache_collect_stats');
+
+    $this->drupalGet('<front>');
+    // Check that statistics are not displayed for anonymous users.
+    $this->assertNoRaw('<div id="memcache-devel">', 'Statistics not present.');
+
+    // Create and login a user without access to view statistics.
+    $account = $this->drupalCreateUser();
+    $this->drupalLogin($account);
+
+    // Check that statistics are not displayed for authenticated users without
+    // permission.
+    $this->assertNoRaw('<div id="memcache-devel">', 'Statistics not present.');
+
+    // Create and login a user with access to view statistics.
+    $account = $this->drupalCreateUser(array('access memcache statistics'));
+    $this->drupalLogin($account);
+
+    $this->drupalGet('<front>');
+    // Check that bootstrap statistics are visible.
+    foreach ($cache_bootstrap_cids as $stat) {
+      $key = $GLOBALS['drupal_test_info']['test_run_id'] . 'cache_bootstrap-' . $stat;
+      $this->assertRaw("<td>get</td><td>cache_bootstrap</td><td>{$key}</td>", t('Key @key found.', array('@key' => $key)));
+    }
+
+    // Clear boostrap cache items.
+    foreach ($cache_bootstrap_cids as $stat) {
+      _cache_get_object('cache_bootstrap')->clear($stat);
+    }
+
+    $this->drupalGet('<front>');
+    // Check that early bootstrap statistics are still visible and are being
+    // set too, after they were removed.
+    foreach ($cache_bootstrap_cids as $stat) {
+      $key = $GLOBALS['drupal_test_info']['test_run_id'] . 'cache_bootstrap-' . $stat;
+      $this->assertRaw("<td>get</td><td>cache_bootstrap</td><td>{$key}</td>", t('Key @key found (get).', array('@key' => $key)));
+      $this->assertRaw("<td>set</td><td>cache_bootstrap</td><td>{$key}</td>", t('Key @key found (set).', array('@key' => $key)));
+    }
+
+    // Clear the internal statistics store.
+    $_memcache_statistics = array();
+    // Check that cache_set() statistics are being recorded.
+    cache_set($this->default_cid, $this->default_value, $this->default_bin);
+    $this->assertEqual($_memcache_statistics, $expected_statistic_set);
+
+    // Clear the internal statistics store.
+    $_memcache_statistics = array();
+    // Check that cache_get() statistics are being recorded.
+    cache_get($this->default_cid, $this->default_bin);
+    $this->assertEqual($_memcache_statistics, $expected_statistic_get);
+
+    // Turn off memcache statistics.
+    variable_set('show_memcache_statistics', FALSE);
+    drupal_static_reset('dmemcache_collect_stats');
+
+    $this->drupalGet('<front>');
+    // Check that statistics are not when the user has access, but statistics
+    // are disabled.
+    $this->assertNoRaw('<div id="memcache-devel">', 'Statistics not present.');
+
+    // Clear the internal statistics store.
+    $_memcache_statistics = array();
+    // Confirm that statistics are not recorded for get()'s when disabled.
+    cache_set($this->default_cid, $this->default_value, $this->default_bin);
+    $this->assertEqual($_memcache_statistics, array());
+
+    // Clear the internal statistics store.
+    $_memcache_statistics = array();
+    // Confirm that statistics are not recorded for set()'s when disabled.
+    cache_get($this->default_cid, $this->default_bin);
+    $this->assertEqual($_memcache_statistics, array());
+  }
+}