CpsFlowExecution: parseScript(): log "Method Too Large" situations more readably #817
Conversation
|
Out of curiosity, what output does this give you and how is it helpful? |
plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java
Outdated
Show resolved
Hide resolved
plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java
Outdated
Show resolved
Hide resolved
…to avoid catching by full name Signed-off-by: Jim Klimov <[email protected]>
…argeExceptionRealistic() tests Signed-off-by: Jim Klimov <[email protected]>
…sException as a carrier of MethodTooLargeException; re-throw with just a compact message to appear in the build log Signed-off-by: Jim Klimov <[email protected]>
|
Tests helped me realize that the PR updated with handling of both eventualities, and if the only exception in view is the MTL - re-throw with a short message avoiding a wall of text in pipeline log. From self-test console (showing both server log Still got a bit of stack trace from generic handling, but it is reasonably short - the ultimate pipeline devs won't have to scroll up a couple of screenfuls just to learn that their script was too long or complex. Now that the message is visible to end users, I am open to suggestions how to make it more actionable, and/or if the e.g. to explain about script length or its coding complexity and what to do about it (nesting, amount of stages, separation of methods into JSL classes, etc.); maybe point to a knowledge-base URL for a better maintainable article on the subject (don't want to replace one wall of text in the logs with another, right?) |
plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java
Show resolved
Hide resolved
plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java
Outdated
Show resolved
Hide resolved
…unt to satisfy different CI platforms Signed-off-by: Jim Klimov <[email protected]>
…t importing the class Signed-off-by: Jim Klimov <[email protected]>
…uggestions for pipeline devs Signed-off-by: Jim Klimov <[email protected]>
… expected matching line Signed-off-by: Jim Klimov <[email protected]>
Until today, it was helpful to myself and a few colleagues who may/can/do follow traces of the Jenkins server logs when developing pipelines (testing in private Jenkins instances and those wrapped by IDEs notwithstanding). After the review comments some months ago (thanks, and sorry for not noticing) and a coding effort today, this should be better visible and more "actionable" to a more general population - now shown as a smaller wall of text right in the pipeline build logs: ...which is IMHO a bit more comprehensible than the original (especially the first time you see it): ...and requires less scrolling to get to the crux of the issue =D |
…ethods (class complexity) from maxTryCatch (nesting depth) Signed-off-by: Jim Klimov <[email protected]>
jglick
left a comment
jglick
left a comment
There was a problem hiding this comment.
The logic here is pretty convoluted and seems excessive when we do not really need to make things particularly pretty, we just need to offer some clue beyond what there is today. Would suffice to do something along the lines of
if (Functions.printThrowable(x).containsString("MethodTooLargeException")) {
throw new IllegalArgumentException("…your hint here…", x);
} else {
throw x;
}
plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java
Outdated
Show resolved
Hide resolved
plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java
Outdated
Show resolved
Hide resolved
…wExecution.java Do not log to server console log (not all server admins are those who run the projects with issues). Co-authored-by: Jesse Glick <[email protected]>
| if (ecCount > 1) { | ||
| // Not squashing with explicit MethodTooLargeException | ||
| // re-thrown below, in this codepath we have other errors. | ||
| throw new RuntimeException(msg, x); | ||
| } else { | ||
| // Do not confuse pipeline devs by a wall of text in the | ||
| // build console, but let the full context be found in | ||
| // server log with some dedication. | ||
| LOGGER.log(Level.FINE, mtlEx.getMessage()); | ||
| //throw new RuntimeException(msg, mtlEx); | ||
| throw new RuntimeException(msg); | ||
| } |
There was a problem hiding this comment.
Regarding the concern of "pretty convoluted logic" - this block is largely why I did it so: groovy can return a MultipleCompilationErrorsException with an attached collection of one or more errors (broadly speaking), so we need to count if we only had the MethodTooLargeException one way or another here (and then print the pretty log), or also something else and then better print everything for human devs to sift through it diligently.
plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java
Outdated
Show resolved
Hide resolved
|
Sorry it took a while to notice that there was finally a new review here - too many browser tabs open to keep track of such :D Thanks for doing it! |
|
Hm, got another use-case to handle: managed to get a step definition too large (aka "global variable", anyhow in JSL as So the pipeline chugged along until it called that step - and that's where the job crashed, with a different exception: Note there are actually two lines Gotta look into the structure of |
Notably, the latter is a Throwable but not an Exception so e.g. LinkageError might be not handled. Signed-off-by: Jim Klimov <[email protected]>
…excerpts - only add a newline before final wrapper if not present in wrapped text Signed-off-by: Jim Klimov <[email protected]>
…eraction" with ContinuationGroup.fixupStackTrace() Signed-off-by: Jim Klimov <[email protected]>
…x in the end, use a dedicated "RuntimeException rtex" object Signed-off-by: Jim Klimov <[email protected]>
…line Signed-off-by: Jim Klimov <[email protected]>
|
Updated screenshots, now with some detail learned about MTL issues happening in the JSL also:
Notably, code was made to trace the messages I've seen (and posted earlier above) with "bread-crumbs" to see the code path through the pipeline script and groovy (JSL) files and line numbers (for steps/classes with code too large), only to discover this is slapped on later via Probably some refactoring can be due to take advantage of that bit. At least, the groovy-cps code is now in the same plugin. |
…ding for logged message Signed-off-by: Jim Klimov <[email protected]>
| } | ||
| actionableMsg.append("; please refactor to simplify code structure"); | ||
| if (overflowedClassNameReport.contains("WorkflowScript") || CLASSNAME_SCRIPTNUM_PATTERN.matcher(overflowedClassName).find()) | ||
| actionableMsg.append(" and/or move logic to a Jenkins Shared Library"); |
There was a problem hiding this comment.
BTW the terminology in https://www.jenkins.io/doc/book/pipeline/shared-libraries/ is unfortunate. By definition a “library” is “shared”; why else would you create a library? The plugin itself uses the term Pipeline Groovy library.
There was a problem hiding this comment.
Updated to mention both terminologies, hopefully that makes it clearer to both populations of readers.
OTOH, I often see (and use) the acronym JSL though, but never saw a PGL...
There was a problem hiding this comment.
Although I would say that code-sharing is one aspect. Code offloading (into a separate compilable unit) is another :)
Some organizations might want the looks of a declarative pipeline for some job with their custom logic behind it, so the logic is in a library (trusted, un-sandboxed and all) while the pipeline seems to call a few steps with a clean interface. But it may be the only consumer (the not-quite-sharing aspect of this).
|
|
||
| static final Logger TIMING_LOGGER = Logger.getLogger(CpsFlowExecution.class.getName() + ".timing"); | ||
|
|
||
| static final Logger METHOD_TOO_LARGE_LOGGER = Logger.getLogger(CpsFlowExecution.class.getName() + ".MethodTooLargeLogging"); |
There was a problem hiding this comment.
Is there some reason not to simply use
?There was a problem hiding this comment.
Something about keeping the possibly large messages out of sight unless truly wanted in troubleshooting.
plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java
Show resolved
Hide resolved
plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java
Outdated
Show resolved
Hide resolved
| .append(METHOD_TOO_LARGE_LOGGER.getName()) | ||
| .append(" is required)"); | ||
|
|
||
| //return new RuntimeException(actionableMsg.toString(), mtlEx); |
There was a problem hiding this comment.
Do not leave commented-out code in a final PR (unless there is a real possibility it should be resurrected some day soonish and a preceding comment explains those circumstances).
| } else if (x instanceof CpsCompilationErrorsException) { | ||
| // Defined in this plugin, to clone a message and stack trace | ||
| // from a MultipleCompilationErrorsException and be serializable. | ||
| // Grep it as text for "MethodTooLargeException" and "1 error" |
There was a problem hiding this comment.
Would it not make more sense to just enrich CpsCompilationErrorsException with whatever information you need? AFAICT all you are looking for ultimately is the MethodTooLargeException.message. So, we could just keep that in a field?
There was a problem hiding this comment.
Interesting idea actually. At the time this was written, I did not realize such a possibility on one hand, and probably wanted the changes to not metastacize all over the codebase on another.
| @Test public void methodTooLargeExceptionFabricated() throws Exception { | ||
| // Fabricate a MethodTooLargeException which "normally" happens when evaluated |
There was a problem hiding this comment.
Is there a reason to keep this test case, given the “realistic” test below?
There was a problem hiding this comment.
I think yes, this fabricated test is about coded reaction to the particular exception, which may be or not be the one thrown in realistic test below (if CPS VM does at all break such way there and with that amount of threads).
| // * java.lang.StackOverflowError varies per JDK platform | ||
| // (CI on Linux was unhappy with 255, on Windows with 1023) |
There was a problem hiding this comment.
You can set a per-thread stack size, but unfortunately the unit of measurement is unspecified and presumably specific to JVM implementation details, so this is probably not that helpful.
There was a problem hiding this comment.
(Why would you would be getting a JVM-level SOE, anyway? This is all dealing with the CPS VM that does not use the native stack.)
There was a problem hiding this comment.
Saw some code in this plugin throwing it (for "excessively nested" cases at least), so thought it is worth keeping in mind.
jglick
left a comment
jglick
left a comment
There was a problem hiding this comment.
OK I think, though I can barely follow most of the code here. @dwnusbaum did you expect to review?
|
I have no plans to review this. My impression from a very brief look at the code was that it seemed quite complicated, but I am not blocking it. IMO creating a page like https://www.jenkins.io/doc/book/pipeline/cps-method-mismatches/ explaining the situation and recommendations in detail seems like it could help, and then perhaps this code could be simplified to only check the stack trace for the relevant class types and add a suppressed exception that links to that page, accepting potential false positives and not adding the additional context currently added by this PR (but maybe that context is critically useful, IDK). |
#817 (review) more or less. IIUC this PR offers the same actionable information as was already there, but formatted differently?
Actually a redirect URL like in jenkins-infra/jenkins.io#2290. |
|
I'm not sure how the simple redirect to a wiki page would be better? Do we not-print a wall of text and redirect to a wiki saying "sometimes the script is too big, guess which one of yours that is"? Or do we print the wall of text and redirect to a wiki that says same and "here's how to look for a needle in the haystack"? Currently the changeset pinpoints which pipeline, step or JSL class has upset the parser (finds those needles in the haystack for us), so I am not sure how the other options are better. Surely a Wiki link can be posted "in addition" to this, but "instead" - I am not so excited about that path (even not due to author bias - just due to loss of usability improvement we use for over a year). UPDATE: Looking back at the comment trail (it has been a while overall), maybe there's a way to simplify code to achieve a similar effect. Also unit tests would be nice, although it can take a while to conjure up some test pipeline/var/class files that do not contain an internal codebase we hit this with. Not sure when/if I'd get to this - got a big higher-priority backlog of work not yet completed at work and in other projects, and this code (and many similarly stalled PRs) is at least completed as far as practical effect is concerned... just 90% of work remains to fulfill the maintainability (or more esoteric ritual) requirements of the project du-jour... I guess for now I'll keep remerging master into the PR to use incremental builds, maybe will come back to this eventually (hopefully before retirement), anyone beating me to it and bringing this to completion is very welcome though :) |
…logging-exMTL Signed-off-by: Jim Klimov <[email protected]>
Signed-off-by: Jim Klimov <[email protected]>
… to mention Pipeline Groovy library/PGL (plugin term) @jglick> BTW the terminology in https://www.jenkins.io/doc/book/pipeline/shared-libraries/ is unfortunate. By definition a “library” is “shared”; why else would you create a library? The plugin itself uses the term Pipeline Groovy library. Signed-off-by: Jim Klimov <[email protected]>
Apply suggestion from @jglick Co-authored-by: Jesse Glick <[email protected]>
jimklimov
force-pushed
the
logging-exMTL
branch
from
3511375 to
ef62003
Compare
4 participants
A small almost-cosmetic change which makes life easier for my colleagues when they develop (and debug failures of) Jenkins scripted pipelines.
UPDATE: Grew to be not so small, so evicted into a method of its own to constrain changes to other parts of the codebase. Learned a few more tricks over time, as new MTL situations were found in the field.