The --regenerate option took a long time for my db. I rewrote it to use a bit more memory and take much less time. It takes under 10 minutes now, whereas it seemed like it was going to take days (I never let it run that long). In the process of rewriting the regenerate code, I also cleaned up some minor bugs and inconsistencies (I probably introduced some in the process too). I have not tested the duplicate code even though I made some edit there. So that code is suspect. I tested the counting code fairly carefully. (I should have made a test suite in the process but I want to move onto other things. Anyway, I thought I would submit my patch here in case you find it useful.

Comment on attachment 122077 [details] [diff] [review] collectstats.pl rewrite gerv does graphing stuff

Comment on attachment 122077 [details] [diff] [review] collectstats.pl rewrite Hold off on reviewing this for the moment. I think there is an off-by-one error hiding in there.

I found the problem with the previous patch. I had written a function to convert MySQL days to YYYYMMDD and it did not take daylight savings into account. I changed it to use MySQL FROM_DAYS() instead.

Thing is... a load of this is about to be obsoleted by bug 16009 :-( Gerv

Ugh, that is exciting and depressing at the same time. Maybe you can commit my changes before you move on with yours?

Can you tell me exactly what this patch does? If it's just speed improvements, I don't care too much, and the scope of the changes means it's a fair bit of work to integrate. If there's more, that might be different. Gerv

Plusses 1. Speed - Plus, the % counter reflects how much time is left (currently % does not fairly estimate time left) 2. Correctness. - Takes into account bugs that change products - Checks for invalid products, statuses, and resolutions - Dies if file format changes (in which case you need to --regenerate) - Does not hardcode statuses and resolutions (reads from file or db) + The new charting patch hardcodes the arrays again in checksetup.pl :-( 3. Robustness - Catches up if data has not been recorded for a few days - Avoids multiple updates in the same day. Minuses 1. Uses more memory - But, it should be within reason for any installation. In otherwords, if you have a big installation, you should already have good hardware. 2. The algorithm is more complicated - Tried to break in down into digestible chunks 3. Touched almost every line

> + The new charting patch hardcodes the arrays again in checksetup.pl :-( That's by design; because the arrays have always been fixed, it makes migration to the new world a whole lot easier. Much as I appreciate your hard work, I am going to check in my changes from bug 16009, and then come along later and see whether any of this is still relevant. I can't let 16009 get held up again - it's been long enough as it is. Gerv

> > + The new charting patch hardcodes the arrays again in checksetup.pl :-( > > That's by design; because the arrays have always been fixed, it makes > migration to the new world a whole lot easier. The arrays have always been fixed in CVS, but anyone who has edited their status will have to edit that place too. Why not read the list out of the file? I guess I am arguing over something that does not matter very much. > > Much as I appreciate your hard work, I am going to check in my changes > from bug 16009, and then come along later and see whether any of > this is still relevant. > I can't let 16009 get held up again - it's been long enough as it is. > > Gerv That's fine. At least you know what's in there. I probably would have asked you about the file before I started had I known my changes would be so extensive. I got sucked in! I look forward to trying out your new charting system.

FWIW, I just tried out this patch on Bugscape. We discovered the cron job hasn't been running since February.... to make a long story short, the original version in CVS took 3 hours to get to 43% of -All- (hadn't done any of the other 60 products yet) before I gave up on it and came hunting down this patch. :) With this patch in, it took 23 minutes to run, and the charts look great :)

Glad to hear it!

-> patch author

Comment on attachment 122207 [details] [diff] [review] collectstats.pl rewrite It works. Even if it's only a temporary thing until the reporting changes go in, it's worth having for the short term.

oops, forgot to cc Gerv when I took him off the owner. As I was saying, I think this is worth checking in, even if it's only temporary until the new reporting lands.

Dave: the new reporting system in bug 16009 is totally ready to land, having been written for the past four months, and waiting on its latest review (with you as one of the requestees) for almost a month - but if this patch is checked in, it will get set back _again_, because I'd have to reintegrate. I'd much rather check that in first, and then see if any of this stuff is still relevant (which it might not be.) Gerv

Passing ownership back to Gerv. He can decide on this patch after he lands his new "General Summary Charts", bug 16009. When I looked at the proposed patch in bug 16009, it looked like this patch would still apply since it only adds two new subroutines to the collectstats.pl script.

was gonna set the dependency but I see it's already here. :)

ok, what's the status of this, now that bug 16009 has landed?

Now that 16009 has landed, old charts are going to go away, thereby obsoleting this (reportedly excellent :-) code. I'm hoping to get rid of old charts by 2.18, but that relies on me making the new system solid enough. Gerv

Gerv, going by comment 20, is this supposed to get WONTFIXed?

I guess so... Gerv