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

RESOLVED FIXED

Status

defect
--
major
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: emorley, Assigned: emorley)

Tracking

Dependency tree / graph

Details

(Whiteboard: [sheriff-want])

Attachments

(8 attachments)

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.
Summary: Don't try to generate annotated summaries for 10000 failures → Don't try to generate annotated summaries for each failure when there are 10000 of them
Blocks: 790889
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.
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.
Attachment #673837 - Flags: review?(arpad.borsos)
* 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".
Attachment #673838 - Flags: review?(arpad.borsos)
Example of us hitting the $maxAnnotatedFailures limit (in this case of 5, just for testing).
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).
* 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.
Attachment #673843 - Flags: review?(arpad.borsos)
Posted image Part C - Screenshot
Example with the limit set to 5 for testing (actual patch uses a limit of 50).
Attachment #673843 - Attachment description: Part C - Don't show bug suggestions if >50 of them → Part C - Don't show bug suggestions for a failure line if >50 of them
* 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.
Attachment #673845 - Flags: review?(arpad.borsos)
Posted image Part D - Screenshot
Example brief log (with limit set to 10 for testing; normally 100).
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)
Attachment #673837 - Flags: review?(arpad.borsos) → review+
Attachment #673838 - Flags: review?(arpad.borsos) → review+
Attachment #673843 - Flags: review?(arpad.borsos) → review+
Attachment #673845 - Flags: review?(arpad.borsos) → review+
Depends on: 805201
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Blocks: 650916
Duplicate of this bug: 767915
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.