I notice you've dropped some indexes. Have you checked these aren't affecting other parts of Moodle using logs (non-stats) ? They must have been there for a reason.

But from a sysadmin point of view we had some (small!) sites which hogged cron overnight to process a single days stats. (I never really looked into what was going on and just disabled the processing im afraid).

I've just forced through reprocessing of one of the sites with the patch to process since last jan:

...completed 398 days of statistics.
Cron script completed correctly
Execution took 283.956878 seconds

I wish I could give you a time report like Dan did, but the cron did it on it's own.

We have 550 students with an average of 5 Moodle classes each, many with daily assignments or other activities.

Very good patch – and large!

Overall, I agree with the patch - it's excellent stuff. Parts of it are identical to mine but yours is further reaching. (Did you have a chance to review my take on the stats generation?) I do have some comments though.

• The main "activity" stats handling (around line 470 in your patch, "how many view actions in each course+role") SQL has 2 things I would improve
  -> You are using an inner join against RA which means that perfectly valid user activity will be ignored – for example, this completely murders stats against sitecourse. Use an outer join, and a coalesce to have a role fallback.
  -> Rather than using 2 subselects, it is much faster to use a single subslelect with a CASE block (supported on all the DBs we care about)

Have a look at the SQL in this patch - it shows the 2 ideas mentioned above http://git.catalyst.net.nz/gitweb?p=moodle-r2.git;a=commitdiff;h=dbbe5093d967cc15d644eff7e15f019bfe68a599

• In admin/cron you set a hard limit of 3 days for stats_cron_daily() – i don't think this will work. Some sites have a big backlog, and available grunt. With your patches, processing 3 days will be fast. This is a problem since the block is still controlled by $timetocheck, which will be once a day – perhaps you meant to remove the $timetocheck logic?

I did not look much at your code before, mostly because I had my patch sort of working before you submitted into the tracker. The SQL was the easier part and I hoped you will be able to merge my SQL with yours easily. The trouble for me was with the locking, timezones and tuning mysql indexes to make it faster.

It would be great if you could improve/polish the patch a bit more, post new patch here, then I could quickly verify it still works on mysql and compare the speed, then you could commit it into cvs. Is it ok this way? I would return back to forum stuff in the meantime

This commit touches 3 elements

• enrolments query

We need a separate query to deal fairly with the sitecourse.

• activity query -

This SQL query is a bit more expensive mainly because of how it reads
role assignments to grab the "primary role". The "primary role"
SQL could be improved further (in dmllib) to be faster and more correct.

• stats_get_action_names() replaces stats_get_action_sql_in()

which simplifies things. It's only called once, so there's no point in
holding a cache, and it doesn't make any assumptions about what the
caller will do with it, so callers can use it with more freedom.

that's my patch, which applies on top of yours. I suspect it is a bit slower on sites with tons of courses, but it is more correct as a result. The primary roles logic is quite patchy still, I think we can do something much much better with a tad of cunning and some SQL substring operations on the context.path to do a simplified "cascading" of the roles.

The SQL operations are DB specific, but I think the 4 DBs support doing what we want "inline" (that is, without stored procedures).

I do not understand why we should use the primary role concept, we are not using it anywhere else, right? The graph shows hits per role, the trouble is that one user may have several roles. I guessed the primary role tries to work around it so that each user has only one role, why? Or is there another reason? I proposed to count user's hits in each role he/she belongs to and construct a new query for course total, because the sum of hits from all roles would not make sense anymore. I believe the second approach is more correct and as a bonus is at least 100x faster on mysql (or maybe it is caused by CASE, not sure yet). I am afraid that if we try to look into parent context roles + primary roles it will be again 100x times slower which would not be acceptable IMHO.

"if (time() > $timetocheck) {" true only once a day? I would expect it to be true all the rest of the day after $timetocheck, though I did not test the code yet

the major changes are:

• added not-logged-in access to gues role on frontpage
• default frontpage role stats
• enrolments in parent course categories should work
• temporary course guest access for normal users
• all users total displayed on role graphs
• fixed graph colors and graphs use the same sorting of roles as table

speed is still <60s for moodle.org day

The patch25 is looking good, but role_assignment is getting inner-joined rather than having an outer-join with a COALESCE to the default site role. The issue with that is that activity gets seriously underreported - unless you move away from the "activity by role" model, but users find it really useful being able to see whether students are lukers/active and if teachers are lurkers or active in a particular course. It is important for users (as in "admin users").

(In that sense, I know the "primary role" thing wasn't 200% accurate, but it was close enough to give users very valuable stats 99% of the time. And some aspects of the primary role subselect can be improved significantly.)

hmmm – perhaps I'm not understanding the patch now that it's gotten so large. All the joins I see are inner joins against role... ah, I tracked it down, you are talkign about stats_get_enrolled_sql(). Hmmm. If you do the join like that (instead of in a subselect that gets collapsed with a distinct or a group by) won't it join into multiple rows (for users with multiple RAs), thus resulting in over-counting? I definitely think it will.

It is getting quite hard to see this huge patch clearly in all its parts and with so many changes – would work much better as a patch series. Can I talk to you about a version control system I've developed in my spare time?

ah - that's perfect then. Makes sense.

------------------------
in case of any problems you can force stats recalculation by:

<?php
require ('config.php');
require_once($CFG->dirroot.'/lib/statslib.php');
require_once($CFG->dirroot.'/lib/adminlib.php');
require_once($CFG->dirroot.'/course/lib.php');

set_cron_lock('statsrunning', null); // force unlocking
$time = time() - 60*60*24*20; // 20 days back
set_config('statslastdaily', $time);
set_config('statslastweekly', $time);
set_config('statslastmonthly', $time);

and then run cron

please test and report any problems here

thanks!

Dan

please let me know which db you are using and what/how query failed

thanks,

Dan

Here is a field report from our medium-size installation. I just tried the latest CVS update with stats for our past 20 days. It took 81 minutes to run, or about 4 minutes per day. Once the initial stats are collected, that seems to be a reasonable amount.

We have a 570 student secondary school with most students having 3 to 5 classes on Moodle. Many classes have daily assignments or other activities (hence probably more activities than college courses). We average about 10,000 entries in mdl_log per day.

This was run on a 2.8 GHZ Linux development box with plenty of memory, duel processors and no other users (it is a backup of the production machine).

Stats never worked for us before work was started on this improvement.

--Gary

One small buglet - if the course has a forced $course->lang then it gets used for displaying the stats for that course (eg whole stats page for Norwegian course display in Norwegian. Without looking I'm guessing it's $COURSE being set incorrectly ($COURSE should be $SITE).

Also, are you using the role name aliases? (when a course overrides the name of a role for local use)?

Doesn't COUNT() return 0 if there are no matching rows?

anyway it is great that somebody is reading the code :-D

Trivial yes. Just wondered if this is because of something that I don't understand. Probably no harm done with the COALESCE there, just makes the query a bit more unreadable, I'd guess performance hit is negligible.

we have set in admin > Server > settings
Maximum runtime: Until Complete
Run at 1 : 5

in Admin> Location > Location Settings Timezones we have set:
Default Timezone: Pacific/Auckland
Users can choose their own timezone
Default Country "New Zealand"

the query that I notice is running at 8:30am looks a bit like this:
update mdl_stats_daily set stat2=(SELECT COUNT(DISTINCT ra.userid) FROM mdl_role_assignments ra, mdl_context ctx, mdl_course c, mdl_rol_capabilities .......

Dan

Dan

so cron_lock for statsrunning is set to 8:50am....

then $nextmidnight rolls around, but cron_lock is still set, untill 24 hours later when timeout value occurs...

$timeout = empty($CFG->statsmaxruntime) ? 60*60*24 : $CFG->statsmaxruntime;

if (!set_cron_lock('statsrunning', $now + $timeout)) {

so. - timeout occurs, 24hours after last cron_lock, (happens to be 8:50am) then checks $nextmidnight and finds that it's after that time, so it runs again at 8:50am

Dan

example:

mssql_n internal transformation:
INSERT INTO mdl_stats_weekly (stattype, timeend, courseid, roleid, stat1, stat2) SELECT stattype, (timeend - 946080000) AS ntimeend, courseid, 0, SUM(stat1), SUM(stat2) FROM mdl_stats_daily WHERE (stattype = 'enrolments' OR stattype = 'activity') AND roleid <> 0 GROUP BY stattype, ntimeend, courseid
to
INSERT INTO mdl_stats_weekly (stattype, timeend, courseid, roleid, stat1, stat2) SELECT stattype, (timeend - 946080000) AS ntimeend, courseid, 0, SUM(stat1), SUM(stat2) FROM mdl_stats_daily WHERE (stattype = N'enrolments' OR stattype = N'activity') AND roleid <> 0 GROUP BY stattype, ntimeend, courseid
207: Invalid column name 'ntimeend'.

ADOConnection._Execute(INSERT INTO mdl_stats_weekly (stattype, timeend, courseid, roleid, stat1, stat2)

SELECT stattype, (timeend - ..., false) % line 891, file: adodb.inc.php
ADOConnection.Execute(INSERT INTO mdl_stats_weekly (stattype, timeend, courseid, roleid, stat1, stat2)

SELECT stattype, (timeend - ...) % line 89, file: dmllib.php
execute_sql(INSERT INTO mdl_stats_weekly (stattype, timeend, courseid, roleid, stat1, stat2)

SELECT stattype, (timeend - ...) % line 1456, file: statslib.php
stats_upgrade_totals() % line 2761, file: upgrade.php
xmldb_main_upgrade(2007101508) % line 358, file: index.php

running this directly on the db:
SELECT stattype, (timeend - 20) AS ntimeend, courseid, 0, SUM(stat1), SUM(stat2)
FROM mdl_stats_daily
GROUP BY stattype, ntimeend, courseid

gives this:
Invalid column name 'ntimeend'.

running this:
SELECT stattype, (timeend - 20) AS ntimeend, courseid, 0, SUM(stat1), SUM(stat2)
FROM mdl_stats_daily
GROUP BY stattype, (timeend - 20), courseid

query executes successfullly.... (doesn't return any results though, but that's probably right!)

please reopen in case of major problems or file new reports,
thanks for testing!