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

Changes from all commits
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
115 changes: 86 additions & 29 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,41 +1,62 @@
Behaviour.specify("span.pipeline-new-node", 'NewNodeConsoleNote', 0, function(e) {
if (e.processedNewNodeConsoleNote) {
return
}
e.processedNewNodeConsoleNote = true
var label = e.getAttribute('label')
/**
* Fix label attributes
*/
function fixLabels(pipelineElement) {
var label = pipelineElement.getAttribute('label')
if (label != null) {
var html = e.innerHTML
var html = pipelineElement.innerHTML
var suffix = ' (' + label.escapeHTML() + ')';
if (html.indexOf(suffix)<0) {
e.innerHTML = e.innerHTML.replace(/.+/, '$&' + suffix) // insert before EOL
}
}
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;
pipelineElement.innerHTML = pipelineElement.innerHTML.replace(/.+/, '$&' + suffix) // insert before EOL
}
}
var nodes = $$('.pipeline-new-node')
}
}

/**
* Generate hide/show links for all of the pipeline elements
*/
function addLinks (pipelineElement) {
var nodeId = pipelineElement.getAttribute('nodeId')
var startId = pipelineElement.getAttribute('startId')
if (startId == null || startId == nodeId) {
pipelineElement.innerHTML = pipelineElement.innerHTML.replace(/.+/, '$&<span class="pipeline-show-hide"> (<a href="#" onclick="showHidePipelineSection(this); return false">hide</a>)</span>')
}
}

/**
* Generate enclosings for a element
*/
function generateEnclosing (node) {
var enclosings = new Map()
var labels = new Map()
var oid = node.getAttribute('nodeId')

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

/**
* Generate enclosings and labels for all of the pipeline elements
*/
function generateEnclosings () {
var enclosings = new Map() // id → enclosingId
var labels = new Map() // id → label
Comment on lines 42 to 43
Copy link
Member

@dwnusbaum dwnusbaum Aug 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Did you mean to create a local version of enclosings and labels here, or were you wanting to reuse and modify the global variables and use them as a kind of cache or something?

The way things are now, it doesn't look like generateEnclosing (no s) actually does anything because the global variables aren't used.

var nodes = $$(".pipeline-new-node")
Copy link
Member

@halkeye halkeye Feb 3, 2022

Choose a reason for hiding this comment

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

in general, document.querySelectorAll will be much faster than $$ as one is a native browser API, and the other would be a prototypes scanner. its not a drop in replacement, but it doesn't seem like there's any prototype specific apis being used for this, so should work.


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'))
appendBranchNames(oid, enclosings, labels)
Copy link
Member

@dwnusbaum dwnusbaum Aug 10, 2020

Choose a reason for hiding this comment

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

It looks like this is the only place where appendBranchNames is called, which seems wrong, since this method is only called for the parallel step itself. What if we start executing the first branch in a parallel step, and then that branch stops and we start executing the other branch? The first branch might get labelled correctly, but I think that the second branch wouldn't be labeled (unless later on in the Pipeline there was another parallel step).

}
}

/**
* Inject the branch name in the pipeline label
*/
function appendBranchNames(nodeId, enclosings, labels) {
var id = nodeId
while (true) {
id = enclosings.get(id)
Expand All @@ -51,12 +72,25 @@ Behaviour.specify("span.pipeline-new-node", 'NewNodeConsoleNote', 0, function(e)
break
}
}
});
}

/**
* Determine if the pipeline contains parallel steps
*/
function isParallelBuild(node) {
return node.innerHTML.includes('parallel')
}

/**
* 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 +100,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 +119,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 +144,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 All @@ -127,3 +165,22 @@ function showHidePipelineSection(link) {
}
}
}

Behaviour.specify("span.pipeline-new-node", 'NewNodeConsoleNote', 0, function(e) {
// check if the current element has already been processed
if (e.processedNewNodeConsoleNote) {
return
}
e.processedNewNodeConsoleNote = true

// fix labels, generate hide/show links and generate enclosings for each element
fixLabels(e)
addLinks(e)
generateEnclosing(e);

// if a parallel build is underway, append branch names to the elements
// Note: we need to traverse all of the elements in parallel builds
if (isParallelBuild(e)) {
generateEnclosings()
}
});