Skip to content
Snippets Groups Projects

Return the last coverage in trace stream

What does this MR do?

This MR fixes the coverage issue raised at https://gitlab.com/gitlab-org/gitlab-ce/issues/31556. Currently, the first coverage appears in the trace will be considered as the coverage. Since general total scores will likely appear at the last part of logs, coverage extraction should be targeting the last matched part.

Are there points in the code the reviewer needs to double check?

Need to be checked by @godfat

Why was this MR needed?

Because #31556 (closed) labeled as regression

Screenshots (if relevant)

Does this MR meet the acceptance criteria?

What are the relevant issue numbers?

Closes #31556 (closed)

Edited by Shinya Maeda

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Kamil Trzciński assigned to @dosuken123

    assigned to @dosuken123

  • Kamil Trzciński changed milestone to %9.2

    changed milestone to %9.2

  • Shinya Maeda added 71 commits

    added 71 commits

    Compare with previous version

  • Shinya Maeda resolved all discussions

    resolved all discussions

  • Shinya Maeda marked the checklist item Changelog entry added, if necessary as completed

    marked the checklist item Changelog entry added, if necessary as completed

  • Shinya Maeda marked the checklist item Conform by the merge request performance guides as completed

    marked the checklist item Conform by the merge request performance guides as completed

  • Shinya Maeda marked the checklist item Conform by the style guides as completed

    marked the checklist item Conform by the style guides as completed

  • Shinya Maeda marked the checklist item Branch has no merge conflicts with master (if it does - rebase it please) as completed

    marked the checklist item Branch has no merge conflicts with master (if it does - rebase it please) as completed

  • Shinya Maeda added 1 commit

    added 1 commit

    Compare with previous version

  • Author Maintainer

    @ayufan Could you review again? Thank you!

  • @dosuken123 Sorry for the delay. I was off for last week. I think we could somehow extend from read_last_lines instead of doing this over again. Something like:

    def read_last_lines(last_lines)
      to_enum(:reverse_line, last_lines).reverse_each.to_a.join
    end
    
    def reverse_line(last_lines)
      # ... some algorithm from previous `read_last_lines`
    end
    Edited by Lin Jen-Shin
  • @dosuken123 I tried it here: !11381 (closed) This is probably still a bit complicated though. /cc @ayufan

  • Lin Jen-Shin mentioned in merge request !11381 (closed)

    mentioned in merge request !11381 (closed)

  • Author Maintainer

    @godfat

    I think we could somehow extend from read_last_lines instead of doing this over again

    Yes, I was thinking the same thing, however, I hesitated because extracting coverage doesn't need last_lines. I think eliminating unnecessary comparisons would make it faster. If we're implementing general/utility reverse_line method, I 100% endorse your MR.

    My MR was considered as "complicated", but In my honest opinion, I think it's ok that the low-level implementation becomes a little bit tricky appearance :thinking:

  • Lin Jen-Shin
  • @dosuken123 Yes I agree, I don't think your implementation is really complicated. Maybe it could be tweaked in some way, but the algorithm won't be too different.

    My only concern is that sharing the same algorithm would make it easier to maintain (and test), and that's all.

  • Author Maintainer

    @godfat Anyway, I found another bug.

    stream.read(BUFFER_SIZE - (pos - max))

    https://gitlab.com/gitlab-org/gitlab-ce/blob/master/lib/gitlab/ci/trace/stream.rb#L109

    This should be stream.read(BUFFER_SIZE + (pos - max)). Because (pos - max) is negative diff.

    P.S. I thought the line is a little bit complex so I re-organized in this MR.

  • Author Maintainer

    @godfat Sorry. The above comment is wrong. Never mind.

  • Shinya Maeda added 456 commits

    added 456 commits

    Compare with previous version

  • Shinya Maeda
  • Shinya Maeda added 85 commits

    added 85 commits

    Compare with previous version

  • Author Maintainer

    @godfat I think we've discussed enough the possibility of bugs. The last thing is the code's appearance.

    We have two options.

    1. Combine reverse_line and read_last_lines (Like your MR)
    2. Optimize reverse_line for extracting coverage (Using each chunk instead of each line, change method name)

    What's your vote?

  • Author Maintainer

    No.2 has a concern that we need to combine two chunks(in the case of the coverage string is split). The string size which is inspected by regex would be BUFFER_SIZE*2. If you want, I can give you the performance comparison.

  • Lin Jen-Shin
    • Resolved by Lin Jen-Shin

      @dosuken123 Thank you so much for working on this. I think I would personally lean more toward to have one single algorithm (i.e. (1)), but it could just be me, so I might not really give my vote to that yet.

      I don't really care too much about the performance here, because we're doing each_line before, which could be slow anyway. If we're not making it significant slower, it's probably fine. So just merge this as is without doing 1 or 2 is fine for me, too.

      I would be curious about the performance though, of course. More information is always better :P Just that it might not make an impact on production, yet.

      Good point about that the string could be as long as BUFFER_SIZE*2, considering that there's an extremely huge line. It's probably already the case though. Matching a huge string with regexp won't be an issue though, as we should match bottom up with \z. I am more worried about constructing a huge string in Ruby, which is memory consuming.

  • Shinya Maeda added 2 commits

    added 2 commits

    • b2e02419 - Use force_encoding(regex.encoding)
    • 21c1803f - Refer reverse_line from read_last_lines

    Compare with previous version

  • Lin Jen-Shin
  • Shinya Maeda added 1 commit

    added 1 commit

    Compare with previous version

    • Resolved by Lin Jen-Shin

      @dosuken123 Awesome. So I think the only thing left would be trying to merge the two loops into one, and I think that would be the ideal. If not, I am also fine with that.

  • Shinya Maeda added 1 commit

    added 1 commit

    Compare with previous version

  • Shinya Maeda added 1 commit

    added 1 commit

    Compare with previous version

  • Lin Jen-Shin resolved all discussions

    resolved all discussions

  • Lin Jen-Shin
  • Lin Jen-Shin
  • Lin Jen-Shin
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Please register or sign in to reply
    Loading