TSPS-140 Send email notifications for succeeded and failed jobs #181

mmorgantaylor · 2024-12-11T20:36:20Z

Description

Add the ability for Teaspoons to send email notifications upon job completion, via PubSub messages sent to a topic that Thurloe (Terra service) subscribes to and uses SendGrid to send the email. Success for Teaspoons is posting the PubSub message.

If the PipelineRun succeeds, the final step in the flight is to send the "job succeeded" notification.
If the PipelineRun fails, the StairwaySendFailedJobNotificationHook is called that sends the "job failed" notification.

Email notifications from BEE testing:
succeeded:

failed:

Related PRs:

terra-helmfile - add sendgrid notification IDs for non-prod environments: https://github.com/broadinstitute/terra-helmfile/pull/5984
terra-helmfile - add Teaspoons environment variables related to pubsub/thurloe https://github.com/broadinstitute/terra-helmfile/pull/5991
workbench-libs - add new notification classes: TSPS-140 Add two notification classes for Teaspoons succeeded and failed jobs broadinstitute/workbench-libs#1714
thurloe - add new notification types: TSPS-140 Add Teaspoons notification types broadinstitute/thurloe#360
checking in the SendGrid templates to this repo: TSPS-372 add email notification template files #179

Jira Ticket

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

jsotobroad

havent looked through the tests yet. have a couple of questions/suggestions

jsotobroad · 2024-12-18T22:28:37Z

service/src/main/resources/db/changelog.xml

@@ -7,6 +7,7 @@
  <include file="changesets/20241106-add-indexes.yaml" relativeToChangelogFile="true"/>
  <include file="changesets/20241106-base-data-insertion.yaml" relativeToChangelogFile="true"/>
  <include file="changesets/20241125_drop_result_url_column.yaml" relativeToChangelogFile="true"/>
+  <include file="changesets/20241212_add_quota_consumed_update_pipeline_display_name.yaml" relativeToChangelogFile="true"/>


it shouldn't actually matter but to be safe this should probably go after the min_quota one in this file

jsotobroad · 2024-12-18T22:32:24Z

service/src/test/java/bio/terra/pipelines/controller/PipelineRunsApiControllerTest.java

  }

  /** helper method to create a PipelineRun object for a completed job. */
-  private PipelineRun getPipelineRunWithStatus(CommonPipelineRunStatusEnum status) {
+  private PipelineRun getPipelineRunWithStatus(


i would either rename this function to getPipelineRunWithStatusAndQuotaConsumed or probably more cleanly select an appropriate value for quotaConsumed in this function depending on the status passed in.

jsotobroad · 2024-12-18T22:34:36Z

...ce/src/main/java/bio/terra/pipelines/common/utils/StairwaySendFailedJobNotificationHook.java

+    if (flightMapKeyIsTrue(inputParameters, JobMapKeys.DO_SEND_JOB_FAILURE_NOTIFICATION_HOOK)
+        && context.getFlightStatus() != FlightStatus.SUCCESS) {
+      logger.info(
+          "Flight has status {}, sending failed job notification email", context.getFlightStatus());


can we log the flight id as well here

chatted with faces, this already has the flight id associated with the log itself

jsotobroad · 2024-12-18T22:37:02Z

service/src/main/java/bio/terra/pipelines/notifications/BaseTeaspoonsJobNotification.java

+import lombok.Getter;
+
+@Getter
+public class BaseTeaspoonsJobNotification {


a light description would be nice here since its the base class

jsotobroad · 2024-12-18T22:41:29Z

service/src/main/java/bio/terra/pipelines/notifications/NotificationService.java

+   * @param jobId the job id
+   * @param userId the user id
+   */
+  public void configureAndSendPipelineRunSucceededNotification(UUID jobId, String userId) {


it looks liek this function and the failure equivalent are really the ones that should be called outside of this class, can we maek the rest of the functions private?

jsotobroad · 2024-12-18T23:02:51Z

service/src/main/java/bio/terra/pipelines/notifications/TeaspoonsJobSucceededNotification.java

+  public final String notificationType;
+  public final String quotaConsumedByJob;


these two fields seem to be in both subclasses of the base class. Any reason to not put them in the base class?

jsotobroad · 2024-12-18T23:04:24Z

service/src/main/java/bio/terra/pipelines/notifications/TeaspoonsJobSucceededNotification.java

+  public final String quotaConsumedByJob;
+
+  public TeaspoonsJobSucceededNotification(
+      BaseTeaspoonsJobNotification baseTeaspoonsJobNotification, String quotaConsumedByJob) {


this is a bit of an awkward pattern for a sub class to have its parent class as constructor argument. prob worth talkign about to see if there is a way to not do this

jsotobroad · 2024-12-18T23:05:54Z

service/src/main/java/bio/terra/pipelines/stairway/SendJobSucceededNotificationStep.java

+import org.slf4j.LoggerFactory;
+
+/**
+ * Step to send an email notification that a job has succeeded. This step cannot fail (we catch all


can we add the expectations from the working map and stuff like we do for other steps (i know its nothing but easier just to read it in the description)

jsotobroad · 2024-12-18T23:06:03Z

service/src/main/java/bio/terra/pipelines/stairway/SendJobSucceededNotificationStep.java

+    } catch (Exception e) {
+      logger.error("Failed to send email notification", e);
+    }
+    return StepResult.getStepResultSuccess();


always success!!

jsotobroad · 2024-12-18T23:07:06Z

service/src/main/resources/application.yml

@@ -34,6 +34,9 @@ env:
    imputation:
      # the default currently points to the GCP dev workspace teaspoons-imputation-dev/teaspoons_imputation_dev_storage_workspace_20240726
      storageWorkspaceStorageUrl: ${IMPUTATION_STORAGE_WORKSPACE_STORAGE_URL:gs://fc-secure-10efd4d7-392a-4e9e-89ea-d6629fbb06cc}
+    notifications:
+      projectId: ${NOTIFICATION_PROJECT_ID:broad-dsde-dev}


nice you already have the helmfile pr created, woot!

jsotobroad

woohoo! just had a question about the mocking in some of the tests. looks great though

jsotobroad · 2024-12-19T21:22:40Z

service/src/test/java/bio/terra/pipelines/notifications/NotificationServiceTest.java

+    int expectedQuotaRemaining = updatedUserQuota.getQuota() - updatedUserQuota.getQuotaConsumed();
+
+    // success is a void method
+    doNothing().when(pubsubService).publishMessage(any(), any(), any());


can this be made any more specific than any()s. same for the other mocking

yes, good call - are you ok leaving the any()s when it's mocking throwing an exception?

yup totally fine!

jsotobroad · 2024-12-19T21:23:32Z

service/src/test/java/bio/terra/pipelines/notifications/PubsubServiceTest.java

+  @Test
+  void initPublisher() throws IOException {
+    TopicName topicName = TopicName.of("projectId", "topicId");
+    try (MockedStatic<Publisher> mockedStaticPublisher = mockStatic(Publisher.class)) {


staticccccccc!!

sonarqubecloud · 2024-12-20T17:01:45Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud

jsotobroad reviewed Dec 18, 2024

View reviewed changes

mmorgantaylor added 22 commits December 19, 2024 13:14

wip

8974dda

resolve merge conflicts

d35f5fe

resolve merge conflicts

8ee74b7

add failed notification hook

a72e8e1

add log message for topic notification

6a8751c

add catch for error in PubsubService

11ad24c

add another log

b40ce69

don't try to create topic

104e73d

add back @PostConstruct

8c708bc

remove createTopic PostConstruct

3a24d4b

shut down pubsub publisher after sending message

ae3ea1b

add log for projectId and topicId

c6b9cba

use the correct env var 🤦

8a08658

update gha versions, some misc cleanup

5ffb6e4

things working, some tests

3b6f617

resolve conflicts

392cb7b

resolve conflicts

f60ac84

resolve conflicts

ed60cb6

dumb test don't even try

c7fb87b

update db changelog, try creating publisher once

eff5b8a

refactor notification classes, other pr comments

e10a1f3

add step expectations of input params and working map

7d35e9b

mmorgantaylor force-pushed the TSPS-140_send_email_notifications branch from cabd972 to 7d35e9b Compare December 19, 2024 18:33

mmorgantaylor added 4 commits December 19, 2024 14:06

add test for initPublisher

140e677

add test, try to get logging working properly with ApiFutureCallback

61f3525

try another logging thing

34815b6

suppress unboxing warning

8bb9531

mmorgantaylor marked this pull request as ready for review December 19, 2024 19:44

mmorgantaylor requested a review from jsotobroad December 19, 2024 19:44

wait up to 30 seconds to retrieve pubsub message publishing result

7fd29a3

jsotobroad approved these changes Dec 19, 2024

View reviewed changes

improve tests by removing some any() inputs to mocks

6e92a56

mmorgantaylor merged commit aa748b5 into main Jan 3, 2025
15 checks passed

mmorgantaylor deleted the TSPS-140_send_email_notifications branch January 3, 2025 14:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TSPS-140 Send email notifications for succeeded and failed jobs #181

TSPS-140 Send email notifications for succeeded and failed jobs #181

mmorgantaylor commented Dec 11, 2024 •

edited

Loading

jsotobroad left a comment

jsotobroad Dec 18, 2024 •

edited

Loading

jsotobroad Dec 18, 2024

jsotobroad Dec 18, 2024

jsotobroad Dec 19, 2024

jsotobroad Dec 18, 2024

jsotobroad Dec 18, 2024

jsotobroad Dec 18, 2024

jsotobroad Dec 18, 2024

jsotobroad Dec 18, 2024

jsotobroad Dec 18, 2024

jsotobroad Dec 18, 2024

jsotobroad left a comment

jsotobroad Dec 19, 2024

mmorgantaylor Dec 20, 2024

jsotobroad Jan 2, 2025

jsotobroad Dec 19, 2024

sonarqubecloud bot commented Dec 20, 2024

		public final String notificationType;
		public final String quotaConsumedByJob;

TSPS-140 Send email notifications for succeeded and failed jobs #181

TSPS-140 Send email notifications for succeeded and failed jobs #181

Conversation

mmorgantaylor commented Dec 11, 2024 • edited Loading

Description

Jira Ticket

jsotobroad left a comment

Choose a reason for hiding this comment

jsotobroad Dec 18, 2024 • edited Loading

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

sonarqubecloud bot commented Dec 20, 2024

Quality Gate passed

mmorgantaylor commented Dec 11, 2024 •

edited

Loading

jsotobroad Dec 18, 2024 •

edited

Loading