Send Customer.io notifications and warnings for connections being auto-disabled #11670

terencecho · 2022-04-01T23:46:55Z

What

A feature flag exists where connections that have been constantly failing will be auto-disabled to save on resources. This PR utilizes infrastructure from a previous PR to send notifications of when connections have been auto-disabled, or warnings that they are about to be auto-disabled.

These notifications require that a Customer.io api key is set as an env variable and that the AUTO_DISABLE_FAILING_CONNECTIONS flag is turned on. It also makes the assumption that an email template exists in your Customer.io account.

This PR address issue #10929.

This followup issue is to allow for users to specify which email they want the notifications to go to.

How

A ticket exists to allow users to set up a customer.io notification configuration, similar to how it works with slack notifications. Because this behavior does not exist yet, a separate method has been created to make sure that these notifications are sent even without a configuration being set. These notifications are sent to the email associated with the workspace of the connection being disabled.

To do

send warning when there has been only failing connections for the past maxDaysOfOnlyFailedJobs / 2 days

Recommended reading order

airbyte-scheduler/persistence/src/main/java/io/airbyte/scheduler/persistence/JobNotifier.java
airbyte-workers/src/main/java/io/airbyte/workers/temporal/scheduling/activities/AutoDisableConnectionActivityImpl.java

terencecho · 2022-04-08T16:50:43Z

I wanted to point out for the reviewers that the warning notifications are meant to be sent out when they have reached 50% of the failure limit, so 50 out of 100 consecutive failure OR 7 days of straight failure, halfway to the 14 days where the auto disable kicks in.

For the consecutive failures, we can just warn once when the number of failures is exactly at 50, but for the days of straight failures, the current implementation does keep a state of if a warning has been sent. This means currently, we send a warning after each failure after 7 days of straight failures, which is not idea.

pmossman

Took a read through this, I think the logic for the most part makes sense, though it is pretty convoluted and a bit hard to follow. I wonder if readability would be helped if the large autoDisableFailingConnection() method were broken into discrete private method calls?

I'm also curious what happens to the temporal workflow when a connection is auto-disabled. I see that the autoDisableFailingConnection activity updates the database status of the connection to INACTIVE, but what happens to the temporal connection manager workflow?

terencecho · 2022-04-08T17:11:14Z

Thanks for the comments Parker. Totally agree with the code being a bit hard to follow after I added the 7 day warning. I'll work on splitting it up and making it easier to understand. I wanted to get feedback on if the logic made sense and if I should be handling the logic in a different way.

When the connection is set to Inactive, the temporal connection manager workflow is still running, it's just that the next job to be scheduled is set to like 100s of years in the future (I forget the exact value), so basically stuck in a state of running, but doesn't do anything unless it receives another signal to do something.

terencecho · 2022-04-08T17:13:45Z

Going to move this PR back to a draft while I refactor for readability.

benmoriceau · 2022-04-08T17:45:36Z

airbyte-scheduler/persistence/src/main/java/io/airbyte/scheduler/persistence/JobNotifier.java

+      final String logUrl = connectionPageUrl + connectionId;
+      final String jobDescription = getJobDescription(job, "");
+
+      if (CONNECTION_DISABLED_NOTIFICATION.equals(action)) {


That's sad we don't use an enum here :(.

agreed. I'll make a note to change these to enums once notification configurations/tables have been finalized as a part of #10999

benmoriceau · 2022-04-08T17:47:29Z

airbyte-scheduler/persistence/src/main/java/io/airbyte/scheduler/persistence/JobNotifier.java

+      final String logUrl = connectionPageUrl + connectionId;
+      final String jobDescription = getJobDescription(job, "");
+
+      if (CONNECTION_DISABLED_NOTIFICATION.equals(action)) {


Nit: We can use a switch here, like:

switch (action) { case CONNECTION... break; case CONNECTION_DIS.... break; default }

benmoriceau · 2022-04-08T17:57:28Z

airbyte-scheduler/persistence/src/main/java/io/airbyte/scheduler/persistence/JobNotifier.java

+    final String durationString = formatDurationPart(duration.toDaysPart(), "day")
+        + formatDurationPart(duration.toHoursPart(), "hour")
+        + formatDurationPart(duration.toMinutesPart(), "minute")
+        + formatDurationPart(duration.toSecondsPart(), "second");


Why don't we use https://commons.apache.org/proper/commons-lang/apidocs/org/apache/commons/lang3/time/DurationFormatUtils.html#formatDuration-long-java.lang.String- it look like a more standard way to format duration.

benmoriceau · 2022-04-08T18:00:09Z

...ava/io/airbyte/workers/temporal/scheduling/activities/AutoDisableConnectionActivityImpl.java

+
+        final Job lastJob;
+        final Optional<Job> optionalJob = jobPersistence.getLastReplicationJob(input.getConnectionId());
+        if (optionalJob.isPresent()) {


it can be refactor in:

final lastJob = optionalJob.orElseThrow(() -> new Exeception())

terencecho · 2022-04-19T15:15:36Z

@pmossman PTAL

terencecho · 2022-04-19T21:02:41Z

For extra context, I refactored the database query to also retrieve timestamps when getting status. With those timestamps, we calculate if the previous failure should have triggered a warning, if i should have, then we skip sending another warning to avoid spam.

pmossman

@terencecho this makes sense to me! I made a recommendation to help with readability of the conditional logic in a couple places. I think once that's addressed, this should be good to go

pmossman · 2022-04-19T23:08:09Z

...ava/io/airbyte/workers/temporal/scheduling/activities/AutoDisableConnectionActivityImpl.java

+
+        // sending warning for disable connection if only failed jobs in the past
+        // maxDaysOfOnlyFailedJobsBeforeWarning days
+        if (numDaysSinceFirstReplicationJob >= maxDaysOfOnlyFailedJobsBeforeWarning &&


This is a bit hard to follow because of the nesting, can you pull some of the inner conditional logic out into named variables to help with readability?

Something like:

final boolean firstReplicationIsOldEnough = numDaysSinceFirstReplicationJob >= maxDaysOfOnlyFailedJobsBeforeWarning; final boolean enoughDaysPassedSinceLastSuccess = successTimestamp.isEmpty() || getDaysSinceTimestamp(currTimestampInSeconds, successTimestamp.get()) >= maxDaysOfOnlyFailedJobsBeforeWarning); if (firstReplicationIsOldEnough && enoughDaysPassedSinceLastSuccess) { ... }

You should pick better variable names if you can think of them, especially if mine don't actually match the logic 😅

Thanks for the suggestions. I edited the above to help with readability.

I went with uglier, longer names like firstReplicationOlderThanMaxDisableWarningDays and successOlderThanPrevFailureByMaxWarningDays to help make things as clear as possible, but do let me know if they actually make things less clear, happy to go with your suggestions if you think so.

pmossman · 2022-04-19T23:09:34Z

...ava/io/airbyte/workers/temporal/scheduling/activities/AutoDisableConnectionActivityImpl.java

+      prevFailedJob = jobs.get(i);
+    }
+
+    return (successTimestamp.isEmpty()


Similar recommendation here for using local boolean variables to aid comprehension of the nested conditional

same as above

pmossman

Nice! Definitely much easier to follow the logic now. LGTM!

pmossman · 2022-04-20T00:28:39Z

...ava/io/airbyte/workers/temporal/scheduling/activities/AutoDisableConnectionActivityImpl.java

+            noPreviousSuccess || getDaysSinceTimestamp(currTimestampInSeconds, successTimestamp.get()) >= maxDaysOfOnlyFailedJobsBeforeWarning;
+
+        // send warning if there are only failed jobs in the past maxDaysOfOnlyFailedJobsBeforeWarning days
+        // _except_ if a warning should have already been sent in the previous failure


super nit: instead of _except_ if a warning..., go with _unless_ a warning...

terencecho · 2022-04-20T22:02:09Z

@benmoriceau i'm going to merge this, but feel free to comment with any suggestions and I can follow up on another diff.

In case you want a tldr: didn't make any changes on the temporal side, instead decided to read timestamp info from database to determine if a warning should have already been sent to avoid spam instead of saving state of last warning sent somewhere.

…o-disabled (#11670) * init commit to send notifications regarding connection auto-disabling * remove debugging prints * Add logic for warning about only failures for days * cleanup comments * remove deplicate code * refactor activity to be more readable * small cleanup * Refactor JobNotifier * fix formatting * Save state of last warning * Move last warning state to WorkflowInternalState * Reduce spam by getting timestamp info instead of saving state * edit tests * Fix formatting * cleanup checkIfWarningPreviouslySent * fix formatting * refactor for readability * Add extra comments * edit comments

init commit to send notifications regarding connection auto-disabling

41c15b8

github-actions bot added area/platform issues related to the platform area/scheduler area/worker Related to worker labels Apr 1, 2022

remove debugging prints

45997ed

terencecho temporarily deployed to more-secrets April 2, 2022 00:21 Inactive

terencecho requested review from pmossman and removed request for pmossman April 4, 2022 17:46

Add logic for warning about only failures for days

2c251a5

terencecho temporarily deployed to more-secrets April 6, 2022 22:23 Inactive

terencecho marked this pull request as ready for review April 6, 2022 22:55

cleanup comments

fb5f5ec

terencecho temporarily deployed to more-secrets April 6, 2022 23:45 Inactive

remove deplicate code

96a132c

terencecho requested a review from pmossman April 7, 2022 00:00

terencecho temporarily deployed to more-secrets April 7, 2022 00:01 Inactive

terencecho requested a review from benmoriceau April 7, 2022 00:01

pmossman reviewed Apr 8, 2022

View reviewed changes

terencecho marked this pull request as draft April 8, 2022 17:13

benmoriceau reviewed Apr 8, 2022

View reviewed changes

refactor activity to be more readable

c9cf687

terencecho temporarily deployed to more-secrets April 8, 2022 18:53 Inactive

terencecho temporarily deployed to more-secrets April 19, 2022 05:20 Inactive

terencecho temporarily deployed to more-secrets April 19, 2022 05:43 Inactive

cleanup checkIfWarningPreviouslySent

fb61cbb

terencecho temporarily deployed to more-secrets April 19, 2022 06:30 Inactive

fix formatting

9c31a31

terencecho temporarily deployed to more-secrets April 19, 2022 06:38 Inactive

terencecho requested a review from benmoriceau April 19, 2022 15:15

terencecho mentioned this pull request Apr 19, 2022

Clean up auto-disabled connections notification messaging and allow for slack notifications #12154

Merged

pmossman requested changes Apr 19, 2022

View reviewed changes

refactor for readability

d0665b1

terencecho temporarily deployed to more-secrets April 20, 2022 00:18 Inactive

Add extra comments

c23dfb0

terencecho temporarily deployed to more-secrets April 20, 2022 00:23 Inactive

terencecho temporarily deployed to more-secrets April 20, 2022 00:24 Inactive

pmossman approved these changes Apr 20, 2022

View reviewed changes

edit comments

fa9075a

terencecho temporarily deployed to more-secrets April 20, 2022 00:33 Inactive

terencecho merged commit 9619f28 into master Apr 20, 2022

terencecho deleted the tcho/10929-auto-disable-email-alert branch April 20, 2022 22:02

octavia-squidington-iii mentioned this pull request Apr 21, 2022

Bump Airbyte version from 0.36.1-alpha to 0.36.2-alpha #12258

Merged

terencecho linked an issue Apr 21, 2022 that may be closed by this pull request

Notify users of auto-disabled connections #10929

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Send Customer.io notifications and warnings for connections being auto-disabled #11670

Send Customer.io notifications and warnings for connections being auto-disabled #11670

terencecho commented Apr 1, 2022 •

edited

Loading

terencecho commented Apr 8, 2022

pmossman left a comment

terencecho commented Apr 8, 2022 •

edited

Loading

terencecho commented Apr 8, 2022

benmoriceau Apr 8, 2022

terencecho Apr 8, 2022

benmoriceau Apr 8, 2022

terencecho Apr 8, 2022

benmoriceau Apr 8, 2022

terencecho Apr 8, 2022

benmoriceau Apr 8, 2022

terencecho Apr 8, 2022

terencecho commented Apr 19, 2022

terencecho commented Apr 19, 2022

pmossman left a comment

pmossman Apr 19, 2022

terencecho Apr 20, 2022

pmossman Apr 19, 2022

terencecho Apr 20, 2022

pmossman left a comment

pmossman Apr 20, 2022

terencecho Apr 20, 2022

terencecho commented Apr 20, 2022

Send Customer.io notifications and warnings for connections being auto-disabled #11670

Send Customer.io notifications and warnings for connections being auto-disabled #11670

Conversation

terencecho commented Apr 1, 2022 • edited Loading

What

How

To do

Recommended reading order

terencecho commented Apr 8, 2022

pmossman left a comment

Choose a reason for hiding this comment

terencecho commented Apr 8, 2022 • edited Loading

terencecho commented Apr 8, 2022

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

terencecho commented Apr 19, 2022

terencecho commented Apr 19, 2022

pmossman 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

pmossman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terencecho commented Apr 20, 2022

terencecho commented Apr 1, 2022 •

edited

Loading

terencecho commented Apr 8, 2022 •

edited

Loading