Skip to content

Conversation

@dwnusbaum
Copy link
Member

See JENKINS-60862.

I am opening this PR as a draft to get feedback on the overall approach and to see if anyone has any ideas on a better way to fix the issue.

When viewing truncated logs, this PR directly injects <span> elements inside of a hidden <div> for all flow nodes containing the same data that would be added for the NewNodeConsoleNote for that node if it was on screen, so that NewNodeConsoleNote/script.js always has all of the data it needs to be able to display branch names.

Here are screenshots showing the effect of the change:

Before (no branch names because the branches started before the truncation point):
Details Screen Shot 2020-01-27 at 14 45 37
After (branch names shown):
Details Screen Shot 2020-01-27 at 14 44 47

This approach seems pretty awkward, and requires us to clone console.jelly from core. One improvement we could make while keeping the same general approach would be to modify FileLogStorage (and any other implementations of LogStorage) to directly inject the same content into the log stream inside of writeHtmlTo in the AnnotatedLargeText override in overallLog whenever the offset is non-zero, so that we don't need to clone console.jelly or modify WorkflowRun. I just kept things simple for now and went with the one-plugin fix to get feedback on the overall idea.

I'm not sure about adding an automated test for this. I will check if there is a way to see what CSS rules are enabled on a page using HtmlUnit in a way that supports CSS rules injected via JavaScript.

@kshultzCB
Copy link

Implementation aside, I like the idea of being able to see the branch names that way.

@dwnusbaum
Copy link
Member Author

@kshultzCB Just to clarify, branch names already show up like that, they just don't always show up correctly when you are viewing truncated logs instead of the full logs.

@kshultzCB
Copy link

@kshultzCB Just to clarify, branch names already show up like that, they just don't always show up correctly when you are viewing truncated logs instead of the full logs.

That's the thing I really like - that I don't have to load that entire mile-long full log to see the stage names. That would be a nice usability improvement.

Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

I do not like cloning console.jelly either but this seems like the most expedient fix for now.

@bitwiseman
Copy link
Contributor

This works fine, I'm happy with a single plugin change especially in the short to medium term.

How hard would it be to switch to the other approach later?

@dwnusbaum
Copy link
Member Author

How hard would it be to switch to the other approach later?

Taking this approach now shouldn't make it any harder to switch to any other approach later. Switching to injecting the spans into the log stream over in workflow-api would be relatively straightforward, we'd just move the logic over there, then remove it from this plugin and update the workflow-api dependency, so users wouldn't have to do anything special.

@dwnusbaum dwnusbaum marked this pull request as ready for review March 10, 2020 17:17
@dwnusbaum dwnusbaum merged commit 20da05f into jenkinsci:master Mar 10, 2020
@dwnusbaum dwnusbaum deleted the JENKINS-60862 branch March 10, 2020 18:13
@jessejoe
Copy link

jessejoe commented Feb 3, 2022

FYI this change (specifically 6e3de24) seems to have broken /console and /consoleFull for some users with large parallel pipeline logs (any version of this plugin with these changes is unusable for us). I've documented a lot of what I've found with a very simple repro case in https://issues.jenkins.io/browse/JENKINS-62241. We've also been discussing in the Jenkins Gitter channel.

@jglick
Copy link
Member

jglick commented Feb 3, 2022

#164?

@jessejoe
Copy link

jessejoe commented Feb 3, 2022

@jglick thanks, I wish I'd found that a few days ago!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants