-
Notifications
You must be signed in to change notification settings - Fork 0
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.
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
TSPS-181 Add stairway hooks for failed flights - update metrics and status #141
Merged
Merged
Changes from 14 commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
3948ce8
wip
mmorgantaylor b5753bb
wip2
mmorgantaylor 8f5b078
refactor, still not working
mmorgantaylor 6648776
mostly working for update status
mmorgantaylor 8fe7eb3
working with filter by flight type
mmorgantaylor dcbc4bf
refactor using inputParams to specify hooks
mmorgantaylor 434b42e
some sonar fixes
mmorgantaylor 7f55bcb
refactor isContextInvalid to FlightUtils
mmorgantaylor c303dc8
add assertions
mmorgantaylor 340c471
update documentation/comments
mmorgantaylor 7e9b32c
add tests for doHook false
mmorgantaylor 6f07999
PR comments
mmorgantaylor 2ab15a0
JobMapKeys refactor
mmorgantaylor b756669
try to make sonar happy?
mmorgantaylor 964e25d
ugh sonar are you happy now
mmorgantaylor 32d6f7a
use boolean, prove it works
mmorgantaylor 6de3480
cleanup
mmorgantaylor File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
45 changes: 45 additions & 0 deletions
45
service/src/main/java/bio/terra/pipelines/common/utils/StairwayFailedMetricsCounterHook.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,45 @@ | ||
package bio.terra.pipelines.common.utils; | ||
|
||
import static bio.terra.pipelines.common.utils.FlightUtils.inputParametersContainTrue; | ||
|
||
import bio.terra.pipelines.app.common.MetricsUtils; | ||
import bio.terra.pipelines.dependencies.stairway.JobMapKeys; | ||
import bio.terra.stairway.FlightContext; | ||
import bio.terra.stairway.FlightStatus; | ||
import bio.terra.stairway.HookAction; | ||
import bio.terra.stairway.StairwayHook; | ||
import org.slf4j.Logger; | ||
import org.slf4j.LoggerFactory; | ||
import org.springframework.stereotype.Component; | ||
|
||
/** | ||
* A {@link StairwayHook} that logs a pipeline failure to Prometheus metrics upon flight failure. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we add some more documentation around what key to use to enable this functionality on a flight |
||
* | ||
* <p>This hook action will only run if the flight's input parameters contain the JobMapKeys key for | ||
* DO_INCREMENT_METRICS_FAILED_COUNTER_HOOK and the flight's status is not SUCCESS. | ||
* | ||
* <p>The JobMapKeys key for PIPELINE_NAME is required to increment the failed flight. | ||
*/ | ||
@Component | ||
public class StairwayFailedMetricsCounterHook implements StairwayHook { | ||
private static final Logger logger = | ||
LoggerFactory.getLogger(StairwayFailedMetricsCounterHook.class); | ||
|
||
@Override | ||
public HookAction endFlight(FlightContext context) { | ||
|
||
if (inputParametersContainTrue( | ||
context.getInputParameters(), JobMapKeys.DO_INCREMENT_METRICS_FAILED_COUNTER_HOOK) | ||
&& context.getFlightStatus() != FlightStatus.SUCCESS) { | ||
logger.info( | ||
"Flight has status {}, incrementing failed flight counter", context.getFlightStatus()); | ||
|
||
// increment failed runs counter metric | ||
PipelinesEnum pipelinesEnum = | ||
PipelinesEnum.valueOf( | ||
context.getInputParameters().get(JobMapKeys.PIPELINE_NAME, String.class)); | ||
MetricsUtils.incrementPipelineRunFailed(pipelinesEnum); | ||
} | ||
return HookAction.CONTINUE; | ||
} | ||
} |
70 changes: 0 additions & 70 deletions
70
service/src/main/java/bio/terra/pipelines/common/utils/StairwayLoggingHook.java
snf2ye marked this conversation as resolved.
Show resolved
Hide resolved
|
This file was deleted.
Oops, something went wrong.
51 changes: 51 additions & 0 deletions
51
service/src/main/java/bio/terra/pipelines/common/utils/StairwaySetPipelineRunStatusHook.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,51 @@ | ||
package bio.terra.pipelines.common.utils; | ||
|
||
import static bio.terra.pipelines.common.utils.FlightUtils.inputParametersContainTrue; | ||
|
||
import bio.terra.pipelines.dependencies.stairway.JobMapKeys; | ||
import bio.terra.pipelines.service.PipelineRunsService; | ||
import bio.terra.stairway.FlightContext; | ||
import bio.terra.stairway.FlightStatus; | ||
import bio.terra.stairway.HookAction; | ||
import bio.terra.stairway.StairwayHook; | ||
import java.util.UUID; | ||
import org.slf4j.Logger; | ||
import org.slf4j.LoggerFactory; | ||
import org.springframework.stereotype.Component; | ||
|
||
/** | ||
* A {@link StairwayHook} that updates the PipelineRun status to FAILED on flight failure. | ||
* | ||
* <p>This the endFlight hook action will only run if the flight's input parameters contain the | ||
* JobMapKeys key for DO_SET_PIPELINE_RUN_STATUS_FAILED_HOOK and the flight's status is not SUCCESS. | ||
* | ||
* <p>The JobMapKeys key for USER_ID is required to set the PipelineRun status to FAILED. | ||
*/ | ||
@Component | ||
public class StairwaySetPipelineRunStatusHook implements StairwayHook { | ||
private static final Logger logger = | ||
LoggerFactory.getLogger(StairwaySetPipelineRunStatusHook.class); | ||
private final PipelineRunsService pipelineRunsService; | ||
|
||
public StairwaySetPipelineRunStatusHook(PipelineRunsService pipelineRunsService) { | ||
this.pipelineRunsService = pipelineRunsService; | ||
} | ||
|
||
@Override | ||
public HookAction endFlight(FlightContext context) { | ||
|
||
if (inputParametersContainTrue( | ||
context.getInputParameters(), JobMapKeys.DO_SET_PIPELINE_RUN_STATUS_FAILED_HOOK) | ||
&& context.getFlightStatus() != FlightStatus.SUCCESS) { | ||
logger.info( | ||
"Flight has status {}, setting PipelineRun status to FAILED", context.getFlightStatus()); | ||
|
||
// set PipelineRun status to FAILED | ||
pipelineRunsService.markPipelineRunFailed( | ||
UUID.fromString(context.getFlightId()), | ||
context.getInputParameters().get(JobMapKeys.USER_ID, String.class)); | ||
} | ||
|
||
return HookAction.CONTINUE; | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm surprised you need the
Boolean.TRUE.equals
bit - can you directly use the boolean value from the get call?We have a few examples across TDR - here is one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although, sometime stairway gets picky about actually defining variables. Something like this might work instead:
You wouldn't think that pre-defining it in a variable would help, but I've come across a few cases where it is needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ugh yeah this is super annoying, I don't know if TDR uses SonarQube, but it complains about this approach:
because a
Boolean
can be null. I couldn't figure out how to extract aboolean
from the inputParameters.I think that we could get around this by saying if it's null, assume that's false... I'll do some poking... we hate the Boolean.TRUE.equals() thing too 😭