-
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
Changes from all commits
3948ce8
b5753bb
8f5b078
6648776
8fe7eb3
dcbc4bf
434b42e
7f55bcb
c303dc8
340c471
7e9b32c
6f07999
2ab15a0
b756669
964e25d
32d6f7a
6de3480
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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.flightMapKeyIsTrue; | ||
|
||
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. | ||
* | ||
* <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 (flightMapKeyIsTrue( | ||
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; | ||
} | ||
} |
snf2ye marked this conversation as resolved.
Show resolved
Hide resolved
|
This file was deleted.
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.flightMapKeyIsTrue; | ||
|
||
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 (flightMapKeyIsTrue( | ||
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; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,28 +3,28 @@ | |
import java.util.Arrays; | ||
import java.util.List; | ||
|
||
public enum JobMapKeys { | ||
public class JobMapKeys { | ||
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. This is interesting - we follow the same enum/getKeyName() pattern in TDR. We don't ever reference the enum without the keyName. I like this new approach - it feels much less bulky. Is there something driving this change besides that it is cleaner? Has this been accepted as the new standard? I'll have to keep this in mind for incremental changes to TDR. 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. I don't think this is any new standard - when we copied things from WSM, they had the enum JobMapKeys and a class for custom keys. We realized that there wasn't really a reason to have both (and it was confusing to have both), and the class version is less bulky, like you said. so we're going for it! |
||
// parameters for all flight types | ||
DESCRIPTION("description"), | ||
USER_ID("user_id"), | ||
PIPELINE_NAME("pipeline_name"), | ||
STATUS_CODE("status_code"), | ||
RESPONSE("response"), // result or output of the job | ||
RESULT_PATH( | ||
"result_path"); // path to the result API endpoint for this job; only used for asynchronous | ||
// endpoints | ||
public static final String DESCRIPTION = "description"; | ||
public static final String USER_ID = "user_id"; | ||
public static final String PIPELINE_NAME = "pipeline_name"; | ||
public static final String PIPELINE_ID = "pipeline_id"; | ||
public static final String STATUS_CODE = "status_code"; | ||
public static final String RESPONSE = "response"; // result or output of the job | ||
public static final String RESULT_PATH = | ||
"result_path"; // path to the result API endpoint for this job; only used for asynchronous | ||
|
||
private final String keyName; | ||
// keys to determine which Stairway hooks to run | ||
public static final String DO_SET_PIPELINE_RUN_STATUS_FAILED_HOOK = | ||
"do_set_pipeline_run_status_failed_hook"; | ||
public static final String DO_INCREMENT_METRICS_FAILED_COUNTER_HOOK = | ||
"do_increment_metrics_failed_counter_hook"; | ||
|
||
JobMapKeys(String keyName) { | ||
this.keyName = keyName; | ||
} | ||
|
||
public String getKeyName() { | ||
return keyName; | ||
JobMapKeys() { | ||
throw new IllegalStateException("Attempted to instantiate utility class JobMapKeys"); | ||
} | ||
|
||
public static List<String> getRequiredKeys() { | ||
return Arrays.asList(JobMapKeys.USER_ID.getKeyName(), JobMapKeys.PIPELINE_NAME.getKeyName()); | ||
return Arrays.asList(USER_ID, PIPELINE_NAME, PIPELINE_ID); | ||
} | ||
} |
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.
can we add some more documentation around what key to use to enable this functionality on a flight