203879 - collectstats.pl rewrite

Status: RESOLVED WONTFIX

(Bugzilla :: Reporting/Charting, enhancement)

Description

[Steve Lamm]

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 1

[Steve Lamm]

Attachment: collectstats.pl rewrite

Comment 2

[Bradley Baetz (:bbaetz)]

Comment on attachment 122077 [details] [diff] [review]
collectstats.pl rewrite

gerv does graphing stuff

Comment 3

[Steve Lamm]

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.

Comment 4

[Steve Lamm]

Attachment: collectstats.pl rewrite

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.

Comment 5

[Gervase Markham [:gerv]]

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

Gerv

Comment 6

[Steve Lamm]

Ugh, that is exciting and depressing at the same time.

Maybe you can commit my changes before you move on with yours?

Comment 7

[Gervase Markham [:gerv]]

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

Comment 8

[Steve Lamm]

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
   

Comment 9

[Gervase Markham [: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.

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

Comment 10

[Steve Lamm]

> >   + 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.                          

Comment 11

[Dave Miller [:justdave]]

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 :)

Comment 12

[Steve Lamm]

Glad to hear it! 

Comment 13

[Dave Miller [:justdave]]

-> patch author

Comment 14

[Dave Miller [:justdave]]

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.

Comment 15

[Dave Miller [:justdave]]

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.

Comment 16

[Gervase Markham [:gerv]]

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

Comment 17

[Steve Lamm]

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.

Comment 18

[Dave Miller [:justdave]]

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

Comment 19

[Dave Miller [:justdave]]

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

Comment 20

[Gervase Markham [:gerv]]

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

Comment 21

[Dave Miller [:justdave]]

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

Comment 22

[Gervase Markham [:gerv]]

I guess so...

Gerv