611018 - Topcrashers calls failing when missing trending data

Status: VERIFIED FIXED

(Socorro :: General, task)

Description

[Ryan Snyder [:ryansnyder] [:rsnyder] [:rysny]]

I've noticed that top crasher api calls tend to fail when they are newish versions.  When top crasher api calls take place, 2 queries are called - the first grabs the top crasher data for the current time period (today through 14 days ago) and the second call grabs the top crasher data for the previous time period for trending data ( today minus 15 days ago through 28 days ago).  It seems that if the second call does not have any data, the api call fails to return any data and throws a 500 error.

Comment 1

[Ryan Snyder [:ryansnyder] [:rsnyder] [:rysny]]

To be more specific...

On newish versions 3.6.12 and 4.0b7, the api call returns 500 errors for 7 day and 14 day durations, while other, older versions return data successfully.  If you make a call with a 3 day duration, top crasher calls for versions 3.6.12 and 4.0b7 return data successfully.

Comment 2

[Robert Helmer [:rhelmer]]

Attachment: traceback from middleware server

I can reproduce this on prod and staging. Going to dig into this a bit more in dev.

Comment 3

[Robert Helmer [:rhelmer]]

The problem seems to be that totalNumberOfCrashesForPeriod() returns None rather than 0 if there are no crashes for the specified period:

http://code.google.com/p/socorro/source/browse/tags/releases/1.7.5_r2680_20101103/socorro/services/topCrashBySignatureTrends.py#34

This is called from getListOfTopCrashesBySignature() to calculate the percentage of crashes displayed versus the total for the period:

http://code.google.com/p/socorro/source/browse/tags/releases/1.7.5_r2680_20101103/socorro/services/topCrashBySignatureTrends.py#62

I will attach a patch for this momentarily.

Comment 4

[Robert Helmer [:rhelmer]]

Attachment: return early if no total crashes in period

Also added asserts to make sure the types are what we expect, would have made this easier to diagnose from traceback.

Also pushed to github, if you'd like to review there:
https://github.com/rhelmer/socorro/commit/de024e87f84b512d08ca654e38c80982c40ae87f

Comment 5

[Robert Helmer [:rhelmer]]

Sending        socorro/services/topCrashBySignatureTrends.py
Transmitting file data .
Committed revision 2695.

Comment 6

[Stephen Donner [:stephend] Not actively reading bugmail]

Laura mentioned in today's call that the fix might not yet be on staging; indeed: 

13:52:17.717: Network: GET http://crash-stats.stage.mozilla.com/topcrasher/byversion/Fennec/2.0a1 [HTTP/1.1 500 Internal Server Error 147ms]

Is that a separate bug, or part of this?  I can reproduce on http://crash-stats.stage.mozilla.com/topcrasher/byversion/Firefox/3.6.12 and http://crash-stats.stage.mozilla.com/topcrasher/byversion/Firefox/4.07b, as mentioned in comment 1.  Thanks!

Comment 7

[Robert Helmer [:rhelmer]]

Attachment: assert proper type for databaseParameters["numberOfCrashes"]

Caught this on staging; I should have tested that this worked in the failure case *and* the normal case ;) Sorry for the noise.

Comment 8

[K Lars Lohn [:lars] [:klohn]]

Comment on attachment 489667 [details] [diff] [review]
assert proper type for databaseParameters["numberOfCrashes"]

the long vs int thing surprises me.

Actually a message in the asserts would probably be helpful.  On that one, if the assert fails with None, the error will just say "AssertionError, None" which isn't very helpful.  Granted you'll get the line number and once you look, you'll understand what's happening, but a more useful error message would be reassuring.

do what you think is best.

Comment 9

[Robert Helmer [:rhelmer]]

(In reply to comment #8)
> Comment on attachment 489667 [details] [diff] [review]
> assert proper type for databaseParameters["numberOfCrashes"]
> 
> the long vs int thing surprises me.
> 
> Actually a message in the asserts would probably be helpful.  On that one, if
> the assert fails with None, the error will just say "AssertionError, None"
> which isn't very helpful.  Granted you'll get the line number and once you
> look, you'll understand what's happening, but a more useful error message would
> be reassuring.
> 
> do what you think is best.

OK I agree, I will attach a patch as-landed.

Comment 10

[Robert Helmer [:rhelmer]]

Attachment: fix as landed

Sending        socorro/services/topCrashBySignatureTrends.py
Transmitting file data .    
Committed revision 2700.

Comment 11

[Robert Helmer [:rhelmer]]

(In reply to comment #6)
> Laura mentioned in today's call that the fix might not yet be on staging;
> indeed: 
> 
> 13:52:17.717: Network: GET
> http://crash-stats.stage.mozilla.com/topcrasher/byversion/Fennec/2.0a1
> [HTTP/1.1 500 Internal Server Error 147ms]
> 
> Is that a separate bug, or part of this?  I can reproduce on
> http://crash-stats.stage.mozilla.com/topcrasher/byversion/Firefox/3.6.12 and

The two above look like they work on stage now.

> http://crash-stats.stage.mozilla.com/topcrasher/byversion/Firefox/4.07b, as
> mentioned in comment 1.  Thanks!

This one does not work, because it is missing from the branch data screen. We should fix this too (really just returning a more informative error message), but this should be a different bug I think.

Stephen would you mind filing one, if you agree? Try adding that branch to test.

Comment 12

[Stephen Donner [:stephend] Not actively reading bugmail]

Sorry, Rob; that rogue URL was just a straight-up typo, and should be http://crash-stats.stage.mozilla.com/topcrasher/byversion/Firefox/4.0b7, rather than http://crash-stats.stage.mozilla.com/topcrasher/byversion/Firefox/4.07b, so no new bug needed.

Verified FIXED; thanks!

Comment 13

[Robert Helmer [:rhelmer]]

(In reply to comment #8)
> Comment on attachment 489667 [details] [diff] [review]
> assert proper type for databaseParameters["numberOfCrashes"]
> 
> the long vs int thing surprises me.

Me too, I looked into it a bit. http://www.postgresql.org/docs/8.2/static/functions-aggregate.html says sum() may return:

"bigint for smallint or int arguments, numeric for bigint arguments, double precision for floating-point arguments, otherwise the same as the argument data type "

In this case the arguments are int, so I think psycopg2 is mapping bigint -> long (this is harder for me to find a definitive link for, but it's the behavior I've observed).

Generally I don't really like doing asserts for checking types (why aren't we just using a statically typed language then? :)), but since this is going into a formatted string where the types are going to be checked I think it makes sense; might be nice to be able to pass e.g. a dict of expected types to socorro.database.database if we end up doing this elsewhere.

Comment 14

[Robert Helmer [:rhelmer]]

Attachment: fix hardcoded long

Typo in the error message, r2701.

Comment 16

[Carsten Book [:Tomcat]]

see bug 611263, seems not fixed right now

Comment 17

[Stephen Donner [:stephend] Not actively reading bugmail]

(In reply to comment #16)
> see bug 611263, seems not fixed right now

Right, it's not fixed on prod -- we still have to push this out.

Comment 18

[Laura Thomson :laura]

rhelmer, r2700 + r2702 are the sum of the changes for this bug?

Comment 19

[Robert Helmer [:rhelmer]]

(In reply to comment #18)
> rhelmer, r2700 + r2702 are the sum of the changes for this bug?

r2695 is the basic fix, r2697-r2701 is getting the asserts right, removing redundancy in the checks, and adding more informative error messages.

I'll attach a unified patch.

Comment 20

[Robert Helmer [:rhelmer]]

Attachment: return early if no total crashes in period, add asserts

Comment 21

[Stephen Donner [:stephend] Not actively reading bugmail]

What's the milestone for this?  1.7.5.2?

Comment 22

[Laura Thomson :laura]

Yeah.  I put it into 1.7.5.