795460 - Don't try to generate annotated summaries for each failure when there are 10000 of them

Status: RESOLVED FIXED

(Tree Management Graveyard :: TBPL, defect)

Description

[Ed Morley [:emorley]]

eg:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&jobname=t-oth&onlyunstarred=1&rev=abf5f7d05488
https://tbpl.mozilla.org/php/getParsedLog.php?id=15639317&tree=Mozilla-Inbound

We should just cap the annotated summary box at say 20 failures.

Will save the bzapi calls as well as preventing the UI from hanging when trying to star the damn things.

Comment 1

[Ed Morley [:emorley]]

Swatinem, I don't quite understand why we're doing this:

  public function getExcerpt($asHTML = false) {
    $lines = $this->getLines();
    $filteredLines = $this->getFilteredLines();
    $excerptLines = array();
    foreach ($filteredLines as $i => $lineNumber) {
      $lineMatch = $this->lineFilter->matchLine($lines[$lineNumber]);
      if ($lineMatch === true)
        $lineMatch = $lines[$lineNumber];
      $excerptLines[] = $asHTML ? $this->linkLine('#error'.$i, $lineMatch) : $lineMatch;
    }
    return implode('', $excerptLines);
  }

--

$filteredLines is already a matched list of lines, so why do we matchLine again? It makes me think $filteredLines isn't what I believe it to be, which will alter how I implement the patch for this bug.

Comment 2

[Arpad Borsos [:Swatinem]]

Annotate shows that its mstanges code. As I read it, yep, the matching is redundant, not sure why. Must have missed it back when i reviewed it.

Comment 3

[Ed Morley [:emorley]]

Attachment: Part A - Improve variable naming

Comment 4

[Ed Morley [:emorley]]

Attachment: Part B - Cap annotated summary at 20 failures

* Displays only the first 20 failures in the annotated summary (happy to adjust number up or down), to stop the pathological cases from (a) hanging the browser, and (b) spamming BzAPI.

* Since part C will add a similar (but higher) cap to the excerpt used for the brief log & bug starring message, we have to check if we hit that cap too, in which case we give the vague "first 20 of 100+ failures" message. Didn't seem worth the hassle to extract the (Part C) excerpt truncation message failure count to set the proper failure count.

* I'm not overly happy with having to keep $maxParsedFailures in sync between this file and LogParser.php, but I think the best solution is to put them in Config.php, once we have an in-repo config.php with passwords separated out, that isn't a PITA to update on tbpl-dev & production. (I'll do this in another bug). Alternatively we could just miss out $totalParsedFailures altogether, and simply state "Only showing first N failures - View log".

Comment 5

[Ed Morley [:emorley]]

Attachment: Part B - Screenshot 1

Example of us hitting the $maxAnnotatedFailures limit (in this case of 5, just for testing).

Comment 6

[Ed Morley [:emorley]]

Attachment: Part B - Screenshot 2

Example of us hitting the $maxAnnotatedFailures limit, but in a case where the number of failures also exceeded $maxParsedFailures (of 10; lowered just to test, normally 100).

Comment 7

[Ed Morley [:emorley]]

Attachment: Part C - Don't show bug suggestions for a failure line if >50 of them

* Means we don't display bug suggestions for an individual failure line in the annotated summary, if there are more than 50 matching bugs (happy to adjust the number up or down).
* Added in a span, since often occurs midway through the failures list, so gets lost (unlike the Leak Analysis row etc, which is always at the end).
* Ideally we'd fall back to checking if <N of the bugs are open, and if so, displaying just the open bugs. However I think I'll leave this to a followup since we'll need to play around with the numbers a bit.

Comment 8

[Ed Morley [:emorley]]

Attachment: Part C - Screenshot

Example with the limit set to 5 for testing (actual patch uses a limit of 50).

Comment 9

[Ed Morley [:emorley]]

Attachment: Part D - Cap unannotated log excerpt at 100 failures

* Same as the patch for the annotated summary, except caps the number of failures shown for the log excerpt used for:
  - parsed log anchor links/highlighting
  - bug comments when starring
  - the creation of the annotated summary
* Shows at most 100 failures in the excerpt (again, happy to adjust up or down).
* One of the main benefits of this will be the reduced spam in bugs for the 'thousands of failures' cases.
* To view failures after the first 100, the full log view can be used (albeit the later failures won't be highlighted or linked to, since the same excerpt is used for that) or obviously one can look at the raw log.

Comment 10

[Ed Morley [:emorley]]

Attachment: Part D - Screenshot

Example brief log (with limit set to 10 for testing; normally 100).

Comment 11

[Ed Morley [:emorley]]

philor, please may I have your feedback on:
(a) The limits used in the attached patches - ie: 
    - Only display first 20 failures in the annotated summary.
    - Max of 50 bugs returned for a bzapi search (in which case we don't show any of them).
    - Only display first 100 failures for (unannotated) log excerpt (eg top of parsed log & in starred bug comments).
(b) The UX / layout in the screenshots (bear in mind screenshots use lower limits for testing, actual values are as above)

Comment 12

[Ed Morley [:emorley]]

Landed for now, as my TBPL patch queue is backing up - philor, if you have any thoughts on changing the thresholds in comment 11, let me know :-)

https://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/00b1dbb59ecc
https://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/fafa85b0d5fc
https://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/ad100e3b6ef0
https://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/9f9bb60203e5