Report errors from unstable jobs#333
Conversation
If a job is unstable there might be a "warning" message associated. Read it and report it in the error field.
albertvaka
added
the
changelog/Added
andrea-mosk
left a comment
andrea-mosk
left a comment
There was a problem hiding this comment.
Looks good to me. Could we add a test for this?
| } | ||
|
|
||
| public boolean isUnstable() { | ||
| return "unstable".equalsIgnoreCase(this.result); |
There was a problem hiding this comment.
could we have this precalculated? string equals has more overhead than having a boolean field
There was a problem hiding this comment.
If we precalculate it there's more room for error (eg: we have to remember to update isError and isUnstable in setResult) and taking into account it's only called 2 times per span, I don't think it's worth the optimization.
I know performance overhead has been sometimes an issue for some customers, but I don't think the impact of this is noticeable (compared to, for example, the call to hostname we make once for every span).
| if(node.isError() && !parent.isError()) { | ||
| propagateResultToAllParents(node, "error"); | ||
| } else if(node.isUnstable() && !parent.isUnstable()) { | ||
| propagateResultToAllParents(node, "unstable"); |
There was a problem hiding this comment.
Should we make this a constant, perhaps? There're 12 places where this literal is used
4 participants
What does this PR do?
If a job is unstable there might be a "warning" message associated. Read it and report it in the error field.
Review checklist (to be filled by reviewers)
changelog/label attached. If applicable it should have thebackward-incompatiblelabel attached.do-not-merge/label attached.kind/andseverity/labels attached at least.