Empty data for latest week

Nice detective work, aclight!

The usage pages are cached independently of the page cache (so that we cache them for auth users, too). It's {cache_project_usage} and I had just cleared that manually. Indeed, now things are fine.

Indeed, next step is to consider aclight's proposed solution or look into other ways to make sure that we've got the data when we think we do. I've gotta run right now, but I'll be back later today to work on this and other things...

Thanks, folks.

aclight and I just had a very fruitful conversation in IRC about this. Here's what we came up with:

A) The root of this bug is that the project_usage code can't make up its mind about if it cares about now() or the last time a "usage week" started.

B) At first, I wanted to fix (A) by not storing the timestamp when we last processed the weekly data, but rather the timestamp of the beginning of the last usage week that we already processed. Then, the code in cron() would be:

  $current_week = project_usage_weekly_timestamp($now, 0);
  $last_week_processed = variable_get('project_usage_last_week_processed', 0);
  if ($current_week > $last_week_processed) {
    project_usage_process_weekly($current_week);
    variable_set('project_usage_last_week_processed', $current_week);
  }

However, aclight pointed out that we'd then loose the data about when the last time we actually processed the data was, which is sometimes useful for debugging, especially on d.o where cron often dies prematurely. So, something like aclight's pseudo patch above is probably what we'll want in the end.

C) The processing code is totally whack, and regularly eats data. hunmonk and I are debugging more closely. Meanwhile, I had to restore the weekly data from before I deployed #165380: Make usage statistics visible on d.o. I also temporarily disabled weekly processing until we can get to the bottom of it.

D) Even if the weekly processing was right, there's still a race condition between when the usage pages start thinking a new period has started that it should display, and when we actually process that weekly data and have it in the DB. The fact that the window of this race was huge is why we saw the bug so obviously, but even if we fix everything else, it'd still be there. Attached patch fixes the display pages to query the DB for the timestamps we actually have, instead of doing all the date math again to figure out the timestamps we think we should have.

This patch is only the start to fix this, but it's going to temporarily stop the bleeding while we figure out everything else...

mysql> DESCRIBE SELECT DISTINCT(timestamp) FROM project_usage_week_release order by timestamp DESC LIMIT 6;
+----+-------------+----------------------------+-------+---------------+---------+---------+------+--------+----------------------------------------------+
| id | select_type | table                      | type  | possible_keys | key     | key_len | ref  | rows   | Extra                                        |
+----+-------------+----------------------------+-------+---------------+---------+---------+------+--------+----------------------------------------------+
|  1 | SIMPLE      | project_usage_week_release | index | NULL          | PRIMARY | 8       | NULL | 116270 | Using index; Using temporary; Using filesort | 
+----+-------------+----------------------------+-------+---------------+---------+---------+------+--------+----------------------------------------------+
1 row in set (0.01 sec)

Eeek, "Using filesort"? That's evil. And, it's happening on every page load, since we end up calling that to help us even figure out if we have the resulting page cached. :(

Also, the right values to use only change once per week when we process another week's data, so it seems stupid to re-do this expensive query on every page load.

Instead, why don't we just generate the array of currently active timestamps (which should be the same in both {project_usage_week_project} and {project_usage_week_release}) when we do the weekly processing, and just stash that array into a setting? Then, each page load just grabs something out of the setting cache. That means we'll have to retrieve and unserialize this array of 6 integers on every page load on d.o, but that seems better than such an evil query on every usage statistics page view. Alternatively, we could just add a new table, {project_usage_active_weeks} or something, that just contains a row for each timestamp we think is currently active. That'd be an additional (incredibly cheap) query on each usage stat page view, but it'd save the unserialize() on every d.o page load... Certainly just using variable_(set|get) would be easier in the code than a whole new table.

I put this patch on d.o earlier tonight, while the DB was melting down. Things seem to be better now, so I presume this is an improvement. ;)

Right, I wasn't calling project_usage_get_active_weeks(TRUE) since on d.o we've basically disabled the usage cron stuff for now. It was indeed the plan to add that back in once we resolve everything else in here, but it's fine to include it in the patch now.

db_query_range() -- cool, yeah, just an oversight.

I agree we should only invalidate {cache_project_usage} if anything has changed. I don't see any reason to clear it every day if the data only changes every week. Seems like we could pretty safely just TRUNCATE {cache_project_usage} at the end of the weekly processing, and otherwise, set those items to never expire, no? ;)

That said, I don't want to turn on this cron code again so long as the DELETE queries are in there. I'm sorry I failed to put up a fight about this when I first committed the code -- I don't understand why those are necessary, and they seem like an awfully easy way to destroy a ton of useful data on your site.

That's not the problem. There should never be existing data in these tables, except when cron dies half-way through. It should be an error if we're processing weekly data and are trying to update rows that already exist. However, cron dies pretty regularly on d.o, so we need to be smarter about this.

Reviewed closely, tested on p.d.o, backed up {project_usage_*} on d.o, deployed, and ran. All is good. Now I just have to get the OSUOSL to setup a cron job to invoke the new script regularly. Committed to HEAD and DRUPAL-5.

@hanoii: Because of #344344-1: Weekly usage stats not processed if we have a full week of data but we processed less than 1 week ago.