Skip to content

Fix issue with big logs, causing the browser to freeze (JENKINS-56377) #164

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
zhekoff316 wants to merge 8 commits into from
Closed

Fix issue with big logs, causing the browser to freeze (JENKINS-56377) #164

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f84c16b
Fix issue with big logs, causing the browser to freeze (JENKINS-56377)
May 26, 2020
f785961
Add link generation back to front-end, move setTimeout() function, re…
May 27, 2020
10a58b7
Remove TODO comment
May 27, 2020
26cc512
Re-factor JS, fix issues
Jun 10, 2020
b0c2eb2
Re-add Behaviour.js, re-factor JS
Jun 18, 2020
d6f12dd
Merge branch 'jenkinsci:master' into master
zhekoff316 Nov 5, 2021
1cd870c
Remove global variables
Nov 9, 2021
43c58ea
Merge branch 'master' into master
zhekoff316 Nov 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/main/java/org/jenkinsci/plugins/workflow/job/console/NewNodeConsoleNote.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,8 @@ private NewNodeConsoleNote(FlowNode node) {
@Override
public ConsoleAnnotator<?> annotate(WorkflowRun context, MarkupText text, int charPos) {
StringBuilder startTag = startTagFor(context, id, start, enclosing);
text.addMarkup(0, text.length(), startTag.toString(), "</span>");
text.addMarkup(0, text.length(), startTag.toString(),
"<span class=\"pipeline-show-hide\"> (<a href=\"#\" onclick=\"showHidePipelineSection(this); return false\">hide</a>)</span></span>");
return null;
}

Expand Down
125 changes: 73 additions & 52 deletions src/main/resources/org/jenkinsci/plugins/workflow/job/console/NewNodeConsoleNote/script.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,62 +1,79 @@
Behaviour.specify("span.pipeline-new-node", 'NewNodeConsoleNote', 0, function(e) {
if (e.processedNewNodeConsoleNote) {
return
}
e.processedNewNodeConsoleNote = true
var label = e.getAttribute('label')
if (label != null) {
var html = e.innerHTML
var suffix = ' (' + label.escapeHTML() + ')';
if (html.indexOf(suffix)<0) {
e.innerHTML = e.innerHTML.replace(/.+/, '$&' + suffix) // insert before EOL
/**
* The setTimeout() method calls a function or evaluates an expression after a specified number of milliseconds.
* It is used to prevent a browser freeze that might occur if a very big log is being evaluated.
*/
setTimeout(() => {
/**
* Specifies something to do when an element matching a CSS selector is encountered.
*
* @param {String} selector a CSS selector triggering your behavior
* @param {String} id combined with selector, uniquely identifies this behavior; prevents duplicate registrations
* @param {Number} priority relative position of this behavior in case multiple apply to a given element; lower numbers applied first (sorted by id then selector in case of tie); choose 0 if you do not care
* @param {Function} behavior callback function taking one parameter, a (DOM) {@link Element}, and returning void
*/
Behaviour.specify("span.pipeline-new-node", 'NewNodeConsoleNote', 0, function (e) {
var nodeId = e.getAttribute('nodeId')
var oid = e.getAttribute('nodeId')
var nodes = $$('.pipeline-new-node')
var enclosings = new Map() // id → enclosingId
var labels = new Map() // id → label

if (e.processedNewNodeConsoleNote) {
return
}
}
var nodeId = e.getAttribute('nodeId')
var startId = e.getAttribute('startId')
if (startId == null || startId == nodeId) {
e.innerHTML = e.innerHTML.replace(/.+/, '$&<span class="pipeline-show-hide"> (<a href="#" onclick="showHidePipelineSection(this); return false">hide</a>)</span>')
// TODO automatically hide second and subsequent branches: namely, in case a node has the same parent as an earlier one
Copy link
Member

@jglick jglick May 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is already filed in JIRA somewhere.

Copy link
Author

@zhekoff316 zhekoff316 May 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed this part, because it would probably make more sense to generate this inside the back-end.

EDIT: upon further inspection, I will leave it as it was before due to its performance impact being way smaller than I thought (almost not noticeable).

Copy link
Member

@jglick jglick May 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should have clarified that my review comment was in reference to the removal of the one-line code comment, which is properly an RFE that should be tracked in Jira, not to behavioral changes in your patch.

}
// The CSS rule for branch names only needs to be added once per node, so we
// check in case we are viewing the truncated log and have already processed
// a duplicate synthetic span element for this node.
var maybeDupeNodes = $$('[nodeid=\"'+nodeId+'\"].pipeline-new-node');
for (var i = 0; i < maybeDupeNodes.length; i++) {
var node = maybeDupeNodes[i];
if (node !== e && node.processedNewNodeConsoleNote) {
return;
e.processedNewNodeConsoleNote = true

var label = e.getAttribute('label')
if (label != null) {
var html = e.innerHTML
var suffix = ' (' + label.escapeHTML() + ')';
if (html.indexOf(suffix) < 0) {
e.innerHTML = e.innerHTML.replace(/.+/, '$&' + suffix) // insert before EOL
}
}
}
var nodes = $$('.pipeline-new-node')
var enclosings = new Map() // id → enclosingId
var labels = new Map() // id → label
for (var i = 0; i < nodes.length; i++) {
var node = nodes[i]
var oid = node.getAttribute('nodeId')
enclosings.set(oid, node.getAttribute('enclosingId'))
labels.set(oid, node.getAttribute('label'))
}
Copy link
Member

@dwnusbaum dwnusbaum May 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing this loop causes branch names to no longer be shown when log output changes from one branch to another in many cases.

The problem is that in the while loop right after this, we need to walk through all of this node's parents to find what parallel branch it is a part of. Because the loop is gone, now we only walk through the immediate parents of each node, so if there is any nesting inside of the parallel branch, for example because of a block-scope step, we never find the enclosing parallel branch, and so the while loop exits before finding a label.

The old code is not optimal because we create a fresh enclosings and labels map for every node even though most of the content will be the same. I would try to come up with a good way to store those as page-level state so that you can keep the approach you have now of only updating enclosings and labels once per node, and then split the while loop below out somehow and only run it after the enclosings and labels have data for every node (not sure of a good way to do that).

Here is a Pipeline you can run to compare the behavior with and without this PR to see what I mean:

stage('Outer') {
    parallel(branchOne: {
        stage('Stage One') {
            node() {
                sh 'echo "from one"'
                sleep(time: 1)
                echo 'from one again'
            }
        }
    }, branchTwo: {
        stage('Stage Two') {
            node() {
                sh 'echo "from two"'
                sleep(time: 1)
                echo 'from two again'
            }
        }
    })
}

Copy link
Author

@zhekoff316 zhekoff316 Jun 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. I will try to find a solution to this problem.

var id = nodeId
while (true) {
id = enclosings.get(id)
if (id == null) {
break

// The CSS rule for branch names only needs to be added once per node, so we
Copy link
Member

@jglick jglick May 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW https://github.com/jenkinsci/workflow-job-plugin/pull/164/files?diff=unified&w=1 improves reviewability due to indentation changes.

// check in case we are viewing the truncated log and have already processed
// a duplicate synthetic span element for this node.
var maybeDupeNodes = $$('[nodeid=\"' + nodeId + '\"].pipeline-new-node');
for (var i = 0; i < maybeDupeNodes.length; i++) {
var node = maybeDupeNodes[i];
if (node !== e && node.processedNewNodeConsoleNote) {
return;
}
}
var label = labels.get(id)
if (label != null && label.indexOf('Branch: ') == 0) {
var branch = label.substring(8)
var ss = document.styleSheets[0]
// TODO https://stackoverflow.com/a/18990994/12916 does not scale well to add width: 25em; text-align: right
ss.insertRule('.pipeline-node-' + nodeId + '::before {content: "[' + branch.escapeHTML() + '] "; color: #9A9999; position: absolute; transform: translateX(-100%)}', ss.cssRules == null ? 0 : ss.cssRules.length)
break

enclosings.set(oid, e.getAttribute('enclosingId'))
labels.set(oid, e.getAttribute('label'))

var id = nodeId
while (true) {
id = enclosings.get(id)
if (id == null) {
break
}
var label = labels.get(id)
if (label != null && label.indexOf('Branch: ') == 0) {
var branch = label.substring(8)
var ss = document.styleSheets[0]
// TODO https://stackoverflow.com/a/18990994/12916 does not scale well to add width: 25em; text-align: right
ss.insertRule('.pipeline-node-' + nodeId + '::before {content: "[' + branch.escapeHTML() + '] "; color: #9A9999; position: absolute; transform: translateX(-100%)}', ss.cssRules == null ? 0 : ss.cssRules.length)
break
}
}
}
});
});
}, 2000)

/**
* Functionality to show/hide a pipeline step (label)
*
* @param link - hyperlink element
**/
function showHidePipelineSection(link) {
var span = link.parentNode.parentNode
var id = span.getAttribute('nodeId')
var display

if (link.textContent === 'hide') {
display = 'none'
link.textContent = 'show'
Expand All @@ -66,7 +83,8 @@ function showHidePipelineSection(link) {
link.textContent = 'hide'
link.parentNode.className = 'pipeline-show-hide'
}
var showHide = function(id, display) {

var showHide = function (id, display) {
var sect = '.pipeline-node-' + id
var ss = document.styleSheets[0]
for (var i = 0; i < ss.cssRules.length; i++) {
Expand All @@ -84,14 +102,16 @@ function showHidePipelineSection(link) {
var ids = []
var starts = new Map()
var enclosings = new Map() // id → enclosingId

for (var i = 0; i < nodes.length; i++) {
var node = nodes[i]
var oid = node.getAttribute('nodeId')
ids.push(oid)
starts.set(oid, node.getAttribute('startId'))
enclosings.set(oid, node.getAttribute('enclosingId'))
}
var encloses = function(enclosing, enclosed, starts, enclosings) {

var encloses = function (enclosing, enclosed, starts, enclosings) {
var id = enclosed
var start = starts.get(id)
if (start != null && start != id) {
Expand All @@ -107,6 +127,7 @@ function showHidePipelineSection(link) {
}
}
}

for (var i = 0; i < ids.length; i++) {
var oid = ids[i]
if (oid != id && encloses(id, oid, starts, enclosings)) {
Expand Down
3 changes: 2 additions & 1 deletion src/main/resources/org/jenkinsci/plugins/workflow/job/console/NewNodeConsoleNote/style.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
span.pipeline-new-node {
color: #9A9999
color: #9A9999;
display: flex !important;
}
span.pipeline-show-hide {
visibility: hidden
Expand Down
2 changes: 1 addition & 1 deletion src/test/java/org/jenkinsci/plugins/workflow/job/console/DefaultLogStorageTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ public class DefaultLogStorageTest {
assertLogContains(page, hudson.model.Messages.Cause_UserIdCause_ShortDescription(alice.getDisplayName()), alice.getUrl());
assertLogContains(page, "Running inside " + b.getDisplayName(), b.getUrl());
assertThat(page.getWebResponse().getContentAsString().replace("\r\n", "\n"),
containsString("<span class=\"pipeline-new-node\" nodeId=\"3\" enclosingId=\"2\">[Pipeline] hyperlink\n</span><span class=\"pipeline-node-3\">Running inside <a href="));
containsString("<span class=\"pipeline-new-node\" nodeId=\"3\" enclosingId=\"2\">[Pipeline] hyperlink\n<span class=\"pipeline-show-hide\"> (<a href=\"#\" onclick=\"showHidePipelineSection(this); return false\">hide</a>)</span></span><span class=\"pipeline-node-3\">Running inside <a href="));
DepthFirstScanner scanner = new DepthFirstScanner();
scanner.setup(b.getExecution().getCurrentHeads());
List<FlowNode> nodes = Lists.newArrayList(scanner.filter(FlowScanningUtils.hasActionPredicate(LogAction.class)));
Expand Down