634276 - Socorro - rework the bugzilla cron for new bugzilla signature field

Status: VERIFIED FIXED

(Socorro :: General, task)

Description

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

A movement is afoot to add a signature field to Bugzilla.  This would replace the signatures within the title that is used now.

The Socorro Bugzilla cron will have to be reworked to access the new signature field.

Comment 1

[Laura Thomson :laura]

Let's get a rough patch worked up for when 577724 lands.  I put this under 1.7.8 to get it on the radar, but it may occur after that.

Comment 2

[Laura Thomson :laura]

Bug 577724 landed, and the old format sigs will be migrated next week: hence, we need this patch nowish, and will push it out in a 1.7.8.2 release ASAP.

Comment 3

[Lonnen :lonnen]

The current system allows for a set of signatures. How do we expect multiple signatures to be delimited with this field? Will each be enclosed in []?

Comment 4

[Robert Kaiser]

(In reply to comment #3)
> The current system allows for a set of signatures. How do we expect multiple
> signatures to be delimited with this field? Will each be enclosed in []?

The idea is to have the same [] enclosure, yes. And attachment 536751 [details] [diff] [review], the script that does the initial migration from summaries to this field seems to agree, if I read it correctly. :)

Comment 5

[David Lawrence [:dkl]]

(In reply to comment #4) 
> The idea is to have the same [] enclosure, yes. And attachment 536751 [details] [diff] [review]
> [details] [review], the script that does the initial migration from
> summaries to this field seems to agree, if I read it correctly. :)

Yes this is what the script does currently. It pulls out one or more signature looking strings from the summary of the bug, and puts one per line in the new Crash Signature textarea field. So if any automated scripts could do the same that would keep it consistent.

dkl

Comment 6

[Lonnen :lonnen]

Attachment: Bugzilla cron field update

May need to use `patch -p1` to apply this, as it was created with git diff.

This updates the Bugzilla cron job to populate signatures from the new signature field. This patch does not update the unit test for the bugzilla cron, but should be enough for reviewers to give feedback.

Comment 7

[Lonnen :lonnen]

Attachment: use the crash signature field

Parse the crash signature field instead of trying to find crashes in the description field. Includes updated tests for the new methods.

Comment 8

[Robert Helmer [:rhelmer]]

Comment on attachment 538211 [details] [diff] [review]
use the crash signature field

Review of attachment 538211 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine to me, just a few nit-picky suggestions. Feel free to ignore if you disagree :)

::: socorro/cron/bugzilla.py
@@ +24,5 @@
> +    while True:
> +      sigStart = signatureString.index("[@", sigEnd) + 2
> +      sigEnd = signatureString.index("]", sigEnd + 1)
> +      signatureSet.add(signatureString[sigStart:sigEnd].strip())
> +  except ValueError:

would be nice to comment here why this is passed

@@ +43,1 @@
>  

I know this is existing code, but I'd drop "Dict" from "bugReport".

Comment 9

[Lonnen :lonnen]

Attachment: use the crash signature field (webapp and backend)

Fixed a small sig parser bug, addressed your comments, added some more test cases, and changed the webapp report bug link to fill in the appropriate fields.

Comment 10

[Robert Helmer [:rhelmer]]

Comment on attachment 538393 [details] [diff] [review]
use the crash signature field (webapp and backend)

>@@ -240,20 +243,21 @@ class Report_Controller extends Controller {
...
>+             
>+            preg_match('/[A-Za-z0-9_:]+/' , $report->signature, $matches, PREG_OFFSET_CAPTURE);
>+            $report_bug_url .= 'short_desc=' . rawurlencode('crash ' . $matches[0][0]) . '&';

I think you need to allow . in that regex too, I just tested on my vagrant install with a signature of "xul.dll@0x1ea87c" and while the crash_signature field was filled out correctly, the summary was "crash xul" (we only see crash signatures of this form when we don't have matching symbols, which does happen occasionally in production for example https://crash-stats.mozilla.com/report/index/b6258653-af79-4763-899d-ba4ac2110609 ... on my vagrant instance I have no symbol stores available so it's more common :) ).

tangent - I notice that bugzilla doesn't attempt the "possible duplicates" search unless the summary field is focused, that's a little unfortunate but probably nothing we can do about it. Might be worth filing a bug against bugzilla.

Anyway, I think the changes are fine overall, r+ with that change.

Comment 11

[Robert Kaiser]

(In reply to comment #10)
> I think you need to allow . in that regex too

Probably even @ as in this case we probably want the full "xul.dll@0x1ea87c" to appear in the summary by default, I think. But now we might be going too far into details, let's get it out to users and we can tweak such details later as well. :)

Comment 12

[Lonnen :lonnen]

Updated the regex to accept both . and @ chars. It is still likely to give us some wonky default titles, but it is easy to patch later if it becomes problematic.

Committed as r3199. http://code.google.com/p/socorro/source/detail?r=3199
Fixed the unit test in r3200. http://code.google.com/p/socorro/source/detail?r=3200

We may need to run this manually once to fill in gaps since the changeover yesterday.

Comment 13

[Robert Kaiser]

(In reply to comment #12)
> Committed as r3199. http://code.google.com/p/socorro/source/detail?r=3199

What are the plans to deploy it?

> We may need to run this manually once to fill in gaps since the changeover
> yesterday.

Changeover? AFAIK, Bugzilla hasn't completely copied over signatures from summaries to the new field yet (yesterday's copy run missed a number of bugs, a re-run with a fix has been requested but from what I know not yet executed) and that run is also not removing things from summaries, so we shouldn't be losing anything, right?

Comment 14

[David Lawrence [:dkl]]

Sorry for the late reply on this. The updated script was ran late on Friday and the remaining signatures were migrated over. Yes, all summaries were left intact.

dkl

Comment 15

[Matt Brandt [:mbrandt]]

Verified as [qa-]

Comment 16

[Matt Brandt [:mbrandt]]

Just wanted to add a few notes:
Code reviewed with lonnen. The output from the cron looks good.