TSPS-181 Add stairway hooks for failed flights - update metrics and status #141

mmorgantaylor · 2024-10-03T18:21:43Z

Description

We have two actions we want to take if a pipeline run flight fails:

Set the pipelineRun status in our db to FAILED
Increment the pipeline failed metric counter for that pipeline type

We had put these into the undo step of the first step of a flight, but this was hacky and wouldn't get executed in cases of dismal failures. The better thing to do is to create Stairway hooks that are called at the end of each flight.

Because these hooks rely on specific fields being present in the working map, they would not necessarily work with any arbitrary flight. Therefore, we add logic to these two hooks to check for the presence of an input parameter that specifies whether to execute the task in the hook.

This PR also includes a refactor of JobMapKeys from an enum to a class so it behaves more like the (newly named) ImputationJobMapKeys (we weren't gaining anything from its being an enum).

Jira Ticket

https://broadworkbench.atlassian.net/browse/TSPS-181

service/IMPLEMENTATION_NOTES.md

jsotobroad

some small nitpicks around the tests and some questions. Overall looks great and this is so much nicer!

jsotobroad · 2024-10-07T13:57:47Z

service/src/main/java/bio/terra/pipelines/common/utils/FlightUtils.java

@@ -158,4 +158,19 @@ public static boolean flightComplete(FlightState flightState) {
        || flightState.getFlightStatus() == FlightStatus.FATAL
        || flightState.getFlightStatus() == FlightStatus.SUCCESS);
  }
+
+  public static boolean isContextInvalid(FlightContext context) {
+    if (context == null || context.getWorkingMap() == null) {


what was the scenario this method was meant to catch? Is this something we see in practice? I feel like this should at least be an error in the logs if not throw a full on exception.

i just copied this from WSM's hook implementation. i assumed it helped in testing scenarios when you don't want to bother with this, but i didn't test without it - just now did, and everything runs fine, so we can take it out for now.

jsotobroad · 2024-10-07T13:59:41Z

service/src/main/java/bio/terra/pipelines/common/utils/FlightUtils.java

+  /** Check whether the inputParameters contain a particular key and whether its value is true */
+  public static boolean inputParametersContainTrue(FlightMap inputParameters, String key) {
+    return inputParameters.containsKey(key)
+        && Boolean.TRUE.equals(inputParameters.get(key, boolean.class));


should this be Boolean.class or can you compare to just && inputParameters.get(key, boolean.class)

yeah, the way Stairway pulls things out, you can't get just a boolean. I did it this way to try to ward you off from complaining about Boolean.TRUE.equals, but you're right it should be Boolean.class.

jsotobroad · 2024-10-07T14:00:47Z

service/src/main/java/bio/terra/pipelines/common/utils/StairwayFailedMetricsCounterHook.java

+import org.springframework.stereotype.Component;
+
+/**
+ * A {@link StairwayHook} that logs a pipeline failure to Prometheus metrics upon flight failure.


can we add some more documentation around what key to use to enable this functionality on a flight

jsotobroad · 2024-10-07T14:01:56Z

service/src/main/java/bio/terra/pipelines/common/utils/StairwayLoggingHook.java

@@ -1,70 +0,0 @@
-package bio.terra.pipelines.common.utils;


awesome, cleaning up stuff!!

jsotobroad · 2024-10-07T14:02:26Z

service/src/main/java/bio/terra/pipelines/common/utils/StairwaySetPipelineRunStatusHook.java

+import org.slf4j.LoggerFactory;
+import org.springframework.stereotype.Component;
+
+/** A {@link StairwayHook} that updates the PipelineRun status to FAILED on flight failure. */


same here about adding some notes about what key(s) the hook expects

jsotobroad · 2024-10-07T14:05:16Z

...ice/src/main/java/bio/terra/pipelines/stairway/imputation/RunImputationJobFlightMapKeys.java

@@ -14,6 +14,12 @@ public abstract class RunImputationJobFlightMapKeys {
  public static final String WDL_METHOD_VERSION = "wdl_method_version";
  public static final String PIPELINE_RUN_OUTPUTS = "pipeline_run_outputs";

+  // keys to determine which Stairway hooks to run
+  public static final String DO_SET_PIPELINE_RUN_STATUS_FAILED_HOOK =


i would consider moving these flight wide keys to their own class. Also do you know why these keys are final Strings and the other key class is an enum?

yeah we can move to the generic JobMapKeys. JobMapKeys is something Stairway expects, but dealing with the enum ends up being uglier than this. we copied this pattern from other services that use Stairway.

yea i dont really have a preference as to which id just rather be consistent if we can. I just noticed that we have two ways of doing it and didnt know if there was a good reason for it

my understanding is the class with public static final String key definitions (i.e. RunImputationJobFlightMapKeys) is nicer to work with, but Stairway expects JobMapKeys to be an enum, so that's why we have both. if you feel strongly enough we can convert RunImputationJobFlightMapKeys to enum to match JobMapKeys and add .getKeyName() every time we want to access a key name

I was wrong about Stairway expecting JobMapKeys to be an enum - converted to a class and it works fine.

jsotobroad · 2024-10-07T14:05:36Z

...src/main/java/bio/terra/pipelines/stairway/imputation/steps/PrepareImputationInputsStep.java

@@ -133,25 +127,6 @@ public StepResult doStep(FlightContext flightContext) {

  @Override
  public StepResult undoStep(FlightContext flightContext) {
-    // this is the first step in RunImputationGcpJobFlight and RunImputationAzureJobFlight.


yay clean steps!

jsotobroad · 2024-10-07T14:08:38Z

service/src/test/java/bio/terra/pipelines/common/utils/FlightUtilsTest.java

+  }
+
+  @Test
+  void inputParametersContainTrue_true() {


i am not loving the mix of camel and snake case. I think you its just as understandable as just inputParametersContainsTrue and the one after this as inputParametersContainsFalse and inputParametersContainsNoKey or just probably better yet just one function that tests all three scenarios since they're not very complicated with a comment or two

jsotobroad · 2024-10-07T14:12:07Z

...ice/src/test/java/bio/terra/pipelines/common/utils/StairwayFailedMetricsCounterHookTest.java

+  }
+
+  @Test
+  void endFlight_notPipelineRunTypeFlight_success() throws InterruptedException {


same comment about the camel casing and snake casing and smooshing these tests down into a smaller number. These are a little more complicated but with some refactoring it can probably be made more manageable. @ParameterizedTest might be useful here?

jsotobroad · 2024-10-07T14:14:26Z

...ice/src/test/java/bio/terra/pipelines/common/utils/StairwaySetPipelineRunStatusHookTest.java

+  }
+
+  @Test
+  void endFlight_notPipelineRunTypeFlight_success() throws InterruptedException {


similar nitpick 🕊️

jsotobroad

awesome thanks for the updates!

snf2ye · 2024-10-10T15:33:48Z

service/src/main/java/bio/terra/pipelines/dependencies/stairway/JobMapKeys.java

-public enum JobMapKeys {
+public class JobMapKeys {


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.

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!

snf2ye · 2024-10-10T15:56:56Z

service/src/main/java/bio/terra/pipelines/common/utils/FlightUtils.java

+    return inputParameters.containsKey(key)
+        && Boolean.TRUE.equals(inputParameters.get(key, Boolean.class));


Suggested change

return inputParameters.containsKey(key)

&& Boolean.TRUE.equals(inputParameters.get(key, Boolean.class));

return inputParameters.containsKey(key) && inputParameters.get(key, Boolean.class);

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.

Although, sometime stairway gets picky about actually defining variables. Something like this might work instead:

Suggested change

return inputParameters.containsKey(key)

&& Boolean.TRUE.equals(inputParameters.get(key, Boolean.class));

Boolean booleanKey = inputParameters.get(key, Boolean.class);

return inputParameters.containsKey(key) && booleanKey;

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.

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 a boolean 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 😭

service/src/main/java/bio/terra/pipelines/common/utils/StairwayLoggingHook.java

snf2ye

A few more questions, but LGTM

sonarcloud · 2024-10-11T15:55:58Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
96.5% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

mmorgantaylor added 8 commits October 2, 2024 15:27

wip

3948ce8

wip2

b5753bb

refactor, still not working

8f5b078

mostly working for update status

6648776

working with filter by flight type

8fe7eb3

refactor using inputParams to specify hooks

dcbc4bf

some sonar fixes

434b42e

refactor isContextInvalid to FlightUtils

7f55bcb

mmorgantaylor commented Oct 4, 2024

View reviewed changes

service/IMPLEMENTATION_NOTES.md Outdated Show resolved Hide resolved

mmorgantaylor added 2 commits October 4, 2024 15:28

add assertions

c303dc8

update documentation/comments

340c471

mmorgantaylor marked this pull request as ready for review October 4, 2024 19:33

mmorgantaylor requested a review from jsotobroad October 4, 2024 19:33

add tests for doHook false

7e9b32c

mmorgantaylor requested a review from okotsopoulos October 4, 2024 19:46

jsotobroad reviewed Oct 7, 2024

View reviewed changes

PR comments

6f07999

mmorgantaylor requested a review from jsotobroad October 7, 2024 15:51

mmorgantaylor added 2 commits October 8, 2024 13:42

JobMapKeys refactor

2ab15a0

try to make sonar happy?

b756669

jsotobroad approved these changes Oct 8, 2024

View reviewed changes

ugh sonar are you happy now

964e25d

okotsopoulos removed their request for review October 10, 2024 13:32

snf2ye reviewed Oct 10, 2024

View reviewed changes

snf2ye approved these changes Oct 10, 2024

View reviewed changes

mmorgantaylor added 2 commits October 11, 2024 10:16

use boolean, prove it works

32d6f7a

cleanup

6de3480

mmorgantaylor merged commit 69441a7 into main Oct 11, 2024
12 checks passed

mmorgantaylor deleted the TSPS-181_mma_stairway_hooks branch October 11, 2024 16:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TSPS-181 Add stairway hooks for failed flights - update metrics and status #141

TSPS-181 Add stairway hooks for failed flights - update metrics and status #141

mmorgantaylor commented Oct 3, 2024 •

edited

Loading

jsotobroad left a comment

jsotobroad Oct 7, 2024

mmorgantaylor Oct 7, 2024

jsotobroad Oct 7, 2024

mmorgantaylor Oct 7, 2024

jsotobroad Oct 7, 2024

jsotobroad Oct 7, 2024

jsotobroad Oct 7, 2024

jsotobroad Oct 7, 2024

mmorgantaylor Oct 7, 2024

jsotobroad Oct 7, 2024

mmorgantaylor Oct 7, 2024

mmorgantaylor Oct 8, 2024

jsotobroad Oct 7, 2024

jsotobroad Oct 7, 2024

jsotobroad Oct 7, 2024

jsotobroad Oct 7, 2024

jsotobroad left a comment

snf2ye Oct 10, 2024

mmorgantaylor Oct 10, 2024

snf2ye Oct 10, 2024

snf2ye Oct 10, 2024

mmorgantaylor Oct 10, 2024

snf2ye left a comment

sonarcloud bot commented Oct 11, 2024

		return inputParameters.containsKey(key)
		&& Boolean.TRUE.equals(inputParameters.get(key, Boolean.class));

	return inputParameters.containsKey(key)
	&& Boolean.TRUE.equals(inputParameters.get(key, Boolean.class));
	return inputParameters.containsKey(key) && inputParameters.get(key, Boolean.class);

TSPS-181 Add stairway hooks for failed flights - update metrics and status #141

TSPS-181 Add stairway hooks for failed flights - update metrics and status #141

Conversation

mmorgantaylor commented Oct 3, 2024 • edited Loading

Description

Jira Ticket

jsotobroad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsotobroad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snf2ye left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Oct 11, 2024

Quality Gate passed

mmorgantaylor commented Oct 3, 2024 •

edited

Loading