Fixes #27574: Post-hooks for campaigns should be executed even even if pre-hooks are in failure #6611
Conversation
823b2ff to
9806fb6
Compare
|
PR updated with a new commit |
| object HookReturnCode { | ||
|
|
||
| def isError(code: Int): Boolean = code < 0 || code > 0 && code < 32 | ||
| def isWarning(code: Int): Boolean = code >= 32 && code < 64 |
| PureHooksLogger.For(logIdentifier).trace(s" -> stderr : ${result.stderr}") | ||
| } | ||
| _ <- ZIO.when(result.code >= 32 && result.code <= 64) { // warning | ||
| _ <- ZIO.when(HookReturnCode.isWarning(result.code)) { // warning |
There was a problem hiding this comment.
This is a change that will certainly lead to no problem at all, it's really not the kind of change that lead to error, at all.
Please double check definition of isWarning.
| } else if (HookReturnCode.isError(result.code)) { // error | ||
| ScriptError(path, result.code, result.stdout, result.stderr, msg) | ||
| } else if (result.code >= 32 && result.code <= 64) { // warning | ||
| } else if (HookReturnCode.isWarning(result.code)) { // warning |
There was a problem hiding this comment.
This is a change that will certainly lead to no problem at all, it's really not the kind of change that lead to error, at all.
Please double check definition of isError and isWarning.
| case HookExecutionHistory.DoNotKeep => (c, Nil) | ||
| // Noop are not real hook execution, just filter them out from the result | ||
| case HookExecutionHistory.Keep => (c, (x :: historyList).filterNot(_ == Noop)) | ||
| val runAllSeq: IOResult[(HookReturnCode, List[(String, HookReturnCode)])] = { |
There was a problem hiding this comment.
add the String which is the command name in the history of returned code
| history match { | ||
| case HookExecutionHistory.DoNotKeep => (c, Nil) | ||
| // Noop are not real hook execution, just filter them out from the result | ||
| case HookExecutionHistory.Keep => (c, ((nextHookName, c) :: historyList).filterNot(_._2 == Noop)) |
There was a problem hiding this comment.
we now keep the last code also as head of the history, because we need the name.
So we could just have no first returned value and use a non empty list with some default value and check the head, but I think we will want to have different global value and last one at some point - perhaps in case of warning for example.
| Scheduled, | ||
| PreHooks(HookResults(Nil)), | ||
| PreHooks(HookResults(HookResult(e.id.value, 1, "pre-hooks", "", "") :: Nil)), | ||
| PostHooks(FailureType, HookResults(Nil)), |
There was a problem hiding this comment.
look, directly from pre-hooks to post-hooks.
| case (None, FailureType) => Failure("pre-hooks were in error and post-hooks completed successfully", "") | ||
| case (None, s) => getDefault(s) | ||
| case (Some(f1), FailureType) => Failure("pre-hooks and post-hooks were in error. Last error:", f1.cause) | ||
| case (Some(f1), _) => f1 |
There was a problem hiding this comment.
this logic is just here to put something relevant in the failure message. We don't keep the whole error, so we don't know what hook errored at that point.
In the futur, we will just have the whole history in the campaign event UI and the user will be able to see what broke.
| timeHooks1 <- currentTimeMillis | ||
| _ <- | ||
| PureHooksLogger.For(loggerName).trace(s"Campaign pre-hooks ran in ${timeHooks1 - timeHooks0} ms") | ||
| PureHooksLogger.For(loggerName).trace(s"Campaign ${hookType.entryName} ran in ${timeHooks1 - timeHooks0} ms") |
There was a problem hiding this comment.
all that foreach just for better logs :)
| ("CAMPAIGN_EVENT_ID", e.id.value), | ||
| ("CAMPAIGN_EVENT_NAME", e.name) | ||
| ) | ||
| .optAdd("CAMPAIGN_NEXT_STATE", nextState.map(_.entryName)), |
There was a problem hiding this comment.
the new parameter, only defined in post-hooks
| } | ||
|
|
||
| case PostHooksType => | ||
| val state = event.state.asInstanceOf[PostHooks] |
There was a problem hiding this comment.
could this not be avoided, by matching on the event.state: CampaignEventState instead ?
There was a problem hiding this comment.
Perhaps, but most 8f the time it's far too powerful for the workflow engine, those state's coverture becomes very hard to check.
But I agree that having that is a smell in the encoding that exposes a mismatch. I chose ro keep it because it's very obviously Not Good and claims it needs to be refactored and I didn't want to massively extends the risks with a big refactoring so late.
| val nextState = { | ||
| val optError = res.results.collectFirst { case r if isError(r) => Failure("post-hooks were in error", r.stderr) } | ||
| (optError, state.nextState) match { | ||
| case (None, FailureType) => Failure("pre-hooks were in error and post-hooks completed successfully", "") |
There was a problem hiding this comment.
does the FailureType not mean that the post-hook completed with a "failure", instead of "successfully" ?
There was a problem hiding this comment.
no optError is None then it mean that post-hooks ran successfully, but we have nextState in Failure type, meaning that there was an error in pre-hooks
There was a problem hiding this comment.
The case you talk about is treated two lines below
|
This PR is not mergeable to upper versions. |
|
OK, squash merging this PR |
…f pre-hooks are in failure
23235cd to
4b4dc4f
Compare
https://issues.rudder.io/issues/27574
This commit handle one annoyance and a semantic change for campaign hooks:
post-hooks, even ifpre-hooksfailed, so that we let user a chance to correct state changed inpre-hooksClearer logs
Now, when a campaign hook fails or warn, it has a corresponding line in webapp logs:
This needed some adaptation:
RunHooks.asyncRunHistoryWorkflow change: always go to
post-hookThe idea here is that is we do something in
pre-hooks, we may want to undo it inpost-hooks.For that, we must always go to
post-hooks- but we need to more info:And actually, it's the same bit of information: we just need to set a
next-statevalue in post-hook before we go to them:pre-hook, thenext-stateisfailure,next-stateisfinished.And we let the user know in the hook with a new environnment variable:
CAMPAIGN_NEXT_STATE.From a code point of view, I had to:
CampaignEventState.PostHooksto storenextState: CampaignEventStateTypepost-hooksin place offailureafter pre hooks, setingnextStatenextStatein base forpost-hooksAnd add a unit test to demonstrate the behavior