Closed Bug 882670 Opened 7 years ago Closed 7 years ago

AnnotatedSummaryGenerator should processLine() harder, better, faster, stronger

Categories

(Tree Management Graveyard :: TBPL, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: emorley, Assigned: emorley)

References

(Blocks 1 open bug)

Details

Attachments

(7 files)

AnnotatedSummaryGenerator::processLine() currently doesn't handle a number of edge cases very well. In addition it could do with a fair amount of cleanup.

Rough overview of current algorithm:
* Check for the pipe symbol delimited format ("foo | bar | baz")
  ** If not delimited, fall back to searching for the whole line.
  ** If it is delimited, check if the test/step name is 'leakcheck'
     ** If so, and the format is valid, search for the objects listed.
     ** If so, but the format is not valid [1], then fall back to searching for the entire line.
     ** If not 'leakcheck', search for the test/step name (minus path).
* If no bugs were returned by the search, then check for a crash top frame, and if found, search again using that string.

Note when we call out to do the search, we give up if the search string is less than 5 characters long or if it's on a blacklist of terms known to not produce useful search results.

This causes problems, since for the cases that match the blacklist (eg step = 'Shutdown' or 'automation.py') we don't then fall back to searching for the entire line (which should be specific enough that we don't need to blacklist). This means we never suggest failures such as bug 630780, bug 820756, bug 848924, bug 667435, bug 797242 etc.

Proposed algorithm:
* Check for the pipe symbol delimited format ("foo | bar | baz")
  ** If it is delimited, check if a _valid_ leakcheck format is present.
     ** If so, search for the objects listed.
     ** If not, search for the test/step name (minus path).
* If no search term has been set so far, or if the chosen search term matches the blacklist or is less than 5 characters long - then set the search term to be the whole failure line.
* Run the search
* If no bugs were returned by the search, then check for a crash top frame, and if found, search again using that string.


[1] eg: http://hg.mozilla.org/mozilla-central/file/b197bed90a98/build/automationutils.py#l262
Swap the if and the else statements to make the diff clearer in part 6.
Attachment #763239 - Flags: review?(ryanvm)
leakcheck failures can either be of the form:
"TEST-UNEXPECTED-FAIL | leakcheck |.* leaked \d+ bytes (Object-1, Object-2, Object-3, ...)"
...or if an error occurred during processing:
"TEST-UNEXPECTED-FAIL | leakcheck |.* negative leaks caught!"

We currently special-case the latter - falling back to searching for the full
line (searching for 'leakcheck' would return too many results). However, with
part 6 we'll automatically fall back to searching for the whole line more
generically, so this special-casing is no longer required.

With the special-casing removed, we need to add 'leakcheck' to the blacklist
to prevent false positives and ensure the generic fallback in part 6 kicks in.
In addition, the 'automationutils.processLeakLog()' entry can be removed since
bug 850681 (where it was renamed to leakcheck) has landed on all active trees.
Attachment #763240 - Flags: review?(ryanvm)
If the search term is longer than 200 characters we truncate it, since bug
summaries can only hold 255 characters - and we need to leave room for the
"Intermittent ..." and alternative test names etc. This truncation can leave
whitespace at the end of the search term, so we should trim() afterwards.
Attachment #763241 - Flags: review?(ryanvm)
Part 6 needs to perform the search-term sanity checking independently of
getBugsForFailure(), so let's break it out into a separate function.
Attachment #763242 - Flags: review?(ryanvm)
Currently we only fall back to searching for the full failure line when unable
to find a search term (ie: when the line isn't in the pipe delimited format).
This patch makes us sanity check the search term earlier, so we can fall back
to full line searching for the "too short" and "matches blacklist" cases too.
Attachment #763243 - Flags: review?(ryanvm)
Avoids repetition of the search term, cleans up output indendation and state
number of bug results returned from the cache/bzapi.
Attachment #763244 - Flags: review?(ryanvm)
Example new log output:

{
Prefetching: Windows XP 32-bit try debug test mochitest-browser-chrome on 45ec4e12e4fa
php /var/www/tbpl/dataimport/../php/getLogExcerpt.php id=24105119&type=annotated
Starting general_error log generation
Starting raw log generation
Generating raw log: 5046ms
Generating general_error log: 9285ms
Starting annotatedsummary log generation
* Search term: "browser_capabilities.js"
  Found in cache
  Bugs returned: 1
* Search term: "browser_net_post-data.js"
  Found in cache
  Bugs returned: 0
* Search term: "browser_net_post-data.js"
  Found in cache
  Bugs returned: 0
* Search term: "browser_net_post-data.js"
  Found in cache
  Bugs returned: 0
Generating annotatedsummary log: 4ms
Completed after 9414ms
}
Let me know if you'd like to talk through any of the patches here :-0
:-) even
Attachment #763238 - Flags: review?(ryanvm) → review+
Attachment #763239 - Flags: review?(ryanvm) → review+
Attachment #763240 - Flags: review?(ryanvm) → review+
Attachment #763241 - Flags: review?(ryanvm) → review+
Attachment #763242 - Flags: review?(ryanvm) → review+
Attachment #763243 - Flags: review?(ryanvm) → review+
Comment on attachment 763244 [details] [diff] [review]
Part 7: Logging tweaks

Nice work!

Thanks for the small chunks and good explanations along the way. Was easy to follow what you were doing :)
Attachment #763244 - Flags: review?(ryanvm) → review+
(I'll leave this to simmer on tbpl-dev for a bit to be sure all is working as expected)
Blocks: 888932
Depends on: 891621
In production :-)
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Depends on: 900886
Product: Webtools → Tree Management
Product: Tree Management → Tree Management Graveyard
You need to log in before you can comment on or make changes to this bug.