678205 - Signature links from new TCBS don't work (/report/list not aware of new beta versions)

Status: VERIFIED FIXED

(Socorro :: General, task, P1)

Description

[Robert Kaiser]

Clicking at any signature listed on https://crash-stats.allizom.org/topcrasher/byversion/Firefox/6.0b5/14 results in a report list that states "There were no reports in the time period specified." - probably because /report/list doesn't know what to do with a "Firefox:6.0b5" version.

Comment 1

[Robert Kaiser]

An example for this is https://crash-stats.allizom.org/report/list?range_value=14&range_unit=days&date=2011-08-10&signature=hang%20%7C%20mozilla%3A%3Aplugins%3A%3APPluginInstanceParent%3A%3ACallNPP_HandleEvent%28mozilla%3A%3Aplugins%3A%3ANPRemoteEvent%20const%26%2C%20short%2A%29&version=Firefox%3A6.0b5 which is the first link off the named report.

Comment 2

[Laura Thomson :laura]

Ned to investigate if this is a bug or a data problem.

Comment 3

[Robert Helmer [:rhelmer]]

(In reply to Robert Kaiser (:[e-mail]) from comment #0)
> Clicking at any signature listed on
> https://crash-stats.allizom.org/topcrasher/byversion/Firefox/6.0b5/14
> results in a report list that states "There were no reports in the time
> period specified." - probably because /report/list doesn't know what to do
> with a "Firefox:6.0b5" version.

Yes this looks like the problem. I've done a rough patch for this in my dev instance and it comes down to the following query:

http://code.google.com/p/socorro/source/browse/branches/releases/2.2/webapp-php/application/models/common.php#186

I think what we need to do here is check if the product/version is old or new tcbs (using the product_info table) and if it is new, then the query should use report.version = '6.0' (not '6.0b') with a clause something like:

AND build IN 
  (SELECT build_id::varchar FROM product_version_builds AS pvb
   JOIN product_versions AS pv
     ON (pvb.product_version_id = pv.product_version_id)
   WHERE product_name = 'Firefox'
   AND version_string = '6.0b')

peterbe and I analyzed and also benchmarked this query and it seems to run fast enough, especially on production we didn't really see any difference. Worth testing further though, especially with a larger dataset.

I'm not sure if it'd be more or less work to move this to a middleware service, if we had more time I definitely would but am tempted to patch it in place. 

Brandon do you have any thoughts on the above? Do you think it'd be easy to modify this PHP code to do a middleware call? If so I think we might as well just do that.

Comment 4

[Robert Kaiser]

(In reply to Robert Helmer [:rhelmer] from comment #3)
>    WHERE product_name = 'Firefox'
>    AND version_string = '6.0b')

You mean '6.0' here, right? ;-)

Also, I think we really should have this look for matching build ID and channel in the future, so that what's found with the link really matches the report the link comes from, but if that's too complicated right now, let's go with just version and leave the rest for a followup.

Comment 5

[Laura Thomson :laura]

Talked to Brandon: Probably should patch this in place, given the timing.

Rob, can you put up the patch for review?  (Probably also want to inc. Kairo's feedback)

Comment 6

[Robert Helmer [:rhelmer]]

(In reply to Robert Kaiser (:[e-mail]) from comment #4)
> (In reply to Robert Helmer [:rhelmer] from comment #3)
> >    WHERE product_name = 'Firefox'
> >    AND version_string = '6.0b')
> 
> You mean '6.0' here, right? ;-)

'6.0' in reports.version, '6.0b' in product_version_builds.version_string


> Also, I think we really should have this look for matching build ID and
> channel in the future, so that what's found with the link really matches the
> report the link comes from, but if that's too complicated right now, let's
> go with just version and leave the rest for a followup.

Might be able to add this now, I'll look at it.

Comment 7

[Robert Helmer [:rhelmer]]

(In reply to Robert Helmer [:rhelmer] from comment #6)
> (In reply to Robert Kaiser (:[e-mail]) from comment #4)
> > Also, I think we really should have this look for matching build ID and
> > channel in the future, so that what's found with the link really matches the
> > report the link comes from, but if that's too complicated right now, let's
> > go with just version and leave the rest for a followup.
> 
> Might be able to add this now, I'll look at it.

Yes I think in order to get the right entries from reports table to come up we'll need to:

1) look up version in product_info (or product_selector)
2) if it's new, pull apart the version string:
a) "b" = beta
b) " (beta)" = release
3) restrict query to both buildid and release channel

Otherwise the "beta" and "final beta" won't be differentiated correctly.

Comment 8

[Robert Helmer [:rhelmer]]

Attachment: make /report/list aware of new beta versions

I think it's more pragmatic to put this in the PHP for now. Brandon what do you think?

This does not address the release channel issue brought up by kairo, can do that in a followup if this approach looks ok.

Comment 9

[Robert Helmer [:rhelmer]]

(In reply to Robert Helmer [:rhelmer] from comment #7)
> 1) look up version in product_info (or product_selector)

done in the latest patch

> 2) if it's new, pull apart the version string:
> a) "b" = beta
> b) " (beta)" = release

Actually we don't need to do this, we can lookup major_version as part of #1 above.


> 3) restrict query to both buildid and release channel

done for buildid, looking into release channel now

> Otherwise the "beta" and "final beta" won't be differentiated correctly.

I meant to say "final beta" and "final" here.

Comment 10

[Robert Helmer [:rhelmer]]

Attachment: make /report/list aware of new beta versions

Same as last time, but additionally restrict by channel if:

1) "major_version" and "version_string" are the same (release channel)
2) "version_string" contains "b" (beta channel)

Both of these cases are only checked for if this product+version is marked as a newtcbs table.

Comment 11

[Brandon Savage [:brandon]]

Comment on attachment 552768 [details] [diff] [review]
make /report/list aware of new beta versions

This feels much like a hack, though I think anything besides moving this to the middleware is. Please make sure a bug is on file to relocate this logic or refine it somehow, but for now this will do.

r+

Comment 12

[Robert Helmer [:rhelmer]]

Trunk:
Committed revision 3410.
2.2 branch:
Committed revision 3411.

Comment 13

[Robert Helmer [:rhelmer]]

FWIW bug 678741 backs out the buildid restriction added to the query in this bug, see that one for more details.

Comment 14

[Matt Brandt [:mbrandt]]

QA verified.

https://crash-stats.allizom.org/topcrasher/byversion/Firefox/6.0b5/14

https://crash-stats.allizom.org/report/list?range_value=14&range_unit=days&date=2011-08-10&signature=hang%20|%20mozilla%3A%3Aplugins%3A%3APPluginInstanceParent%3A%3ACallNPP_HandleEvent%28mozilla%3A%3Aplugins%3A%3ANPRemoteEvent%20const%26%2C%20short*%29&version=Firefox%3A6.0b5

Comment 15

[Robert Helmer [:rhelmer]]

BTW filed bug 678642 to move this to middleware