🎉 add ability to send success notifications on webhook #5517

prasrvenkat · 2021-08-19T02:07:13Z

What

Ability to get notified on successful sync as well. This makes the success/failure notifications configurable with toggle on the webhook settings page. This is based on #2706 and will fix #4519

How

Adds two flags sendOnSuccess and sendOnFailure to indicate whether to send notification on success and/or failure.
Default value is true for sendOnFailure and false for sendOnSuccess to keep it same as current behavior.
Ability in front-end to toggle both on/off. I am not that familiar with frontend in general, but tried to get a working solution, which works with current state.
- The toggle is not auto-save, its tied to the current formik form so reflects only after save changes.
- Test will send different notifications for success and failure
The call site for successful notification is in JobSubmitter after finishing something that is a success. Let me know if this not the right place. Ideally, I thought notification will be downstream option after tracking but there was a notifier already available there so re-used it.
Migration test was failing so I had to add one, let me know if that is fine, especially the version

Pre-merge Checklist

Expand the relevant checklist and delete the others.

New Connector

Community member or Airbyter

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

Create a non-forked branch based on this PR and test the below items on it
Build is successful
Credentials added to Github CI. Instructions.
/test connector=connectors/<name> command is passing.
New Connector version released on Dockerhub by running the /publish command described here

Updating a connector

Community member or Airbyter

Grant edit access to maintainers (instructions)
Secrets in the connector's spec are annotated with airbyte_secret
Unit & integration tests added and passing. Community members, please provide proof of success locally e.g: screenshot or copy-paste unit, integration, and acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.
Code reviews completed
Documentation updated
- Connector's README.md
- Changelog updated in docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
PR name follows PR naming conventions
Connector version bumped like described here

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

Create a non-forked branch based on this PR and test the below items on it
Build is successful
Credentials added to Github CI. Instructions.
/test connector=connectors/<name> command is passing.
New Connector version released on Dockerhub by running the /publish command described here

Connector Generator

Issue acceptance criteria met
PR name follows PR naming conventions
If adding a new generator, add it to the list of scaffold modules being tested
The generator test modules (all connectors with -scaffold in their name) have been updated with the latest scaffold by running ./gradlew :airbyte-integrations:connector-templates:generator:testScaffoldTemplates then checking in your changes
Documentation which references the generator is updated as needed.

ChristopheDuong · 2021-08-20T11:42:48Z

airbyte-server/src/main/java/io/airbyte/server/handlers/WorkspacesHandler.java

+      final String messageFormat = "Hello World! This is a test from Airbyte to try %s notification settings for sync %s";
+      final boolean failureNotified = notificationClient.notifyFailure(String.format(messageFormat, notification.getNotificationType(), "failures"));
+      final boolean successNotified = notificationClient.notifySuccess(String.format(messageFormat, notification.getNotificationType(), "successes"));
+      if (failureNotified || successNotified) {


This endpoint is called for testing when editing the workspace settings (not a real sync notification)

Do we really need to send both success and failure (two notifications) as tests to try if the webhook works?

Only one "test" notification should be enough since they are using the same webhook URL and only differs in the messaging?

I do not have strong preference. Only reasoning I had was the flags control success and/or failure notifications so it seemed right to send both if both are enabled and send one or other if only one of them (or none if none of them) is enabled.

If you feel strongly, I can drop one. Let me know.

airbyte-webapp/src/locales/en.json

cgardens

@prasrvenkat thank you for this PR! We really appreciate you digging into so many pieces of our system to put this together. I'm definitely happy to work with you to get this merged!

Here's the plan.

I left a few comments throughout the PR. LMK if anything doesn't make sense. I think there is only one significant change that I am suggesting around the data model.
I tried to address some of the questions you left in the PR body below. Again, lmk if anything doesn't make sense!
After we iterate one more time, I'll loop in our FE guy to help out with the refreshing issue you mentioned.

Ability in front-end to toggle both on/off. I am not that familiar with frontend in general, but tried to get a working solution.
The toggle is not auto-save, its tied to the current formik form so reflects only after save changes.

@jamakase can you take a look at the FE change in this PR and leave any feedback please?

The call site for successful notification is in JobSubmitter after finishing something that is a success. Let me know if this not the right place. Ideally, I thought notification will be downstream option after tracking but there was a notifier already available there so re-used it.

I agree with your assessment that this is not the ideal place. We want to change it so that the notifier happens from within a Temporal Activity. That being said, I think what you have done for now is a good compromise and we can move the call site in the future. So let's stick with what you have!

Migration test was failing so I had to add one, let me know if that is fine, especially the version

That looks right to me! I will re-review after you address some of the data model comments.

Thanks again!

airbyte-api/src/main/openapi/config.yaml

airbyte-migration/src/main/java/io/airbyte/migrate/migrations/MigrationV0_30_0.java

airbyte-config/models/src/main/resources/types/SlackNotificationConfiguration.yaml

airbyte-migration/src/test/java/io/airbyte/migrate/migrations/MigrateV0_30_0Test.java

prasrvenkat · 2021-08-24T17:04:59Z

@prasrvenkat thank you for this PR! We really appreciate you digging into so many pieces of our system to put this together. I'm definitely happy to work with you to get this merged!

Here's the plan.

I left a few comments throughout the PR. LMK if anything doesn't make sense. I think there is only one significant change that I am suggesting around the data model.

I tried to address some of the questions you left in the PR body below. Again, lmk if anything doesn't make sense!

After we iterate one more time, I'll loop in our FE guy to help out with the refreshing issue you mentioned.

Ability in front-end to toggle both on/off. I am not that familiar with frontend in general, but tried to get a working solution.
The toggle is not auto-save, its tied to the current formik form so reflects only after save changes.

@jamakase can you take a look at the FE change in this PR and leave any feedback please?

The call site for successful notification is in JobSubmitter after finishing something that is a success. Let me know if this not the right place. Ideally, I thought notification will be downstream option after tracking but there was a notifier already available there so re-used it.

I agree with your assessment that this is not the ideal place. We want to change it so that the notifier happens from within a Temporal Activity. That being said, I think what you have done for now is a good compromise and we can move the call site in the future. So let's stick with what you have!

Migration test was failing so I had to add one, let me know if that is fine, especially the version

That looks right to me! I will re-review after you address some of the data model comments.

Thanks again!

Thanks for detailed review, appreciate it. I reworked based on your comment. Let me know what you think @cgardens.

airbyte-webapp/src/pages/SettingsPage/pages/NotificationPage/components/WebHookForm.tsx

airbyte-webapp/src/pages/SettingsPage/pages/NotificationPage/NotificationPage.tsx

airbyte-webapp/src/components/hooks/services/useWorkspace.tsx

airbyte-webapp/src/pages/SettingsPage/pages/NotificationPage/NotificationPage.tsx

cgardens

@prasrvenkat thank you! the changes you made on the backend look good to me.

Once you are done iterating on @jamakase 's feedback on the frontend part, I will help you merge this.

jamakase · 2021-08-24T23:07:41Z

My overall comment is next: is it actually a good idea to extend notifications interface for every new state we want to add? Is it possible that in the future, we may want to add more types of events on which we notify like onStart or update with some onProgress? Isn't it better to have generic method that will handle them as boolean notify(Job job, EventType type)

prasrvenkat · 2021-08-25T01:10:59Z

My overall comment is next: is it actually a good idea to extend notifications interface for every new state we want to add? Is it possible that in the future, we may want to add more types of events on which we notify like onStart or update with some onProgress? Isn't it better to have generic method that will handle them as boolean notify(Job job, EventType type)

If you are talking about toggles then we need individual states if we want to allow that level of control for all such states in notifications.
If you are talking about backend interface/methods, I was just trying to build on top of what is already there. Happy to rework to make it generic if you want. Let me know. cc @cgardens (But that refactoring might make more sense once we actually have such well-defined EventTypes defined which afaik is not there yet?)

airbyte-webapp/src/pages/SettingsPage/pages/NotificationPage/components/WebHookForm.tsx

marcosmarxm · 2021-09-06T18:24:50Z

@prasrvenkat could you resolve the conflicts? After that we can start the process of merging this! thanks a lot

Co-authored-by: Christophe Duong <christophe.duong@gmail.com>

Co-authored-by: Charles <giardina.charles@gmail.com>

prasrvenkat · 2021-09-07T02:07:15Z

@prasrvenkat could you resolve the conflicts? After that we can start the process of merging this! thanks a lot

@marcosmarxm Done, let me know if anything else needs to be fixed. Thanks

cgardens

@prasrvenkat Awesome! This looks like it is ready to go. I am running the acceptance tests on this branch now. Once they are done I will merge.

Thanks for your patience. I was on vacation last week which is why review was postponed.

prasrvenkat · 2021-09-07T17:52:26Z

@prasrvenkat Awesome! This looks like it is ready to go. I am running the acceptance tests on this branch now. Once they are done I will merge.

Thanks for your patience. I was on vacation last week which is why review was postponed.

Sounds good, thanks!

michel-tricot · 2021-09-07T18:58:45Z

Thank you @prasrvenkat. This is a great new feature!

jrhizor · 2021-09-07T19:03:25Z

🎉

tuliren · 2021-09-10T13:47:44Z

This is a file based migration, which will mess up our database migration. It needs to be replaced with a Flyway migration.

sheshan-doye-konvergeai · 2022-02-08T12:31:56Z

Is there a way I can update the webhook config programatically ( without going to setting and updating manually) ?

prasrvenkat · 2022-02-08T13:54:35Z

Is there a way I can update the webhook config programatically ( without going to setting and updating manually) ?

It is part of the workspace so you can check that endpoint in APIs. I don't think a specific endpoint exists just for updating notification afaik.

sheshan-doye-konvergeai · 2022-02-09T07:14:22Z

the web hook doesn't work if endpoint is localhost or 0.0.0.0. it works if i set up my service with tools like ngrok.
but if airbyte is locally deployed it should also detect local endpoints. is it possible ?

github-actions bot added area/api Related to the api area/platform issues related to the platform area/documentation Improvements or additions to documentation area/frontend labels Aug 19, 2021

jrhizor requested review from ChristopheDuong, davinchia and cgardens and removed request for davinchia August 19, 2021 21:00

ChristopheDuong reviewed Aug 20, 2021

View reviewed changes

airbyte-webapp/src/locales/en.json Outdated Show resolved Hide resolved

marcosmarxm added the community_pr label Aug 20, 2021

cgardens reviewed Aug 24, 2021

View reviewed changes

cgardens assigned ChristopheDuong and cgardens Aug 24, 2021

prasrvenkat force-pushed the feat/add_sync_success_notification branch from db9bc2c to 2daf991 Compare August 24, 2021 17:03

prasrvenkat requested review from ChristopheDuong and cgardens August 24, 2021 18:54

prasrvenkat force-pushed the feat/add_sync_success_notification branch from 2daf991 to f23a84f Compare August 24, 2021 21:20

jamakase reviewed Aug 24, 2021

View reviewed changes

cgardens approved these changes Aug 24, 2021

View reviewed changes

prasrvenkat requested a review from jamakase August 25, 2021 01:03

jamakase approved these changes Aug 25, 2021

View reviewed changes

airbyte-webapp/src/pages/SettingsPage/pages/NotificationPage/components/WebHookForm.tsx Outdated Show resolved Hide resolved

airbyte-webapp/src/pages/SettingsPage/pages/NotificationPage/components/WebHookForm.tsx Outdated Show resolved Hide resolved

prasrvenkat requested review from jamakase and cgardens August 25, 2021 16:59

prasrvenkat and others added 2 commits September 6, 2021 18:52

add ability to send success notifications on webhook

8167f36

Update airbyte-webapp/src/locales/en.json

ee1051b

Co-authored-by: Christophe Duong <christophe.duong@gmail.com>

prasrvenkat and others added 5 commits September 6, 2021 18:52

Apply suggestions from code review

b37bf5a

Co-authored-by: Charles <giardina.charles@gmail.com>

address review comments - move flags to notification level

dbd460d

review comments - frontend

0b4e7ee

more review comments - frontend

df1729b

rebase fixes

0e1fcf1

prasrvenkat force-pushed the feat/add_sync_success_notification branch from d5a655f to 0e1fcf1 Compare September 7, 2021 02:06

cgardens approved these changes Sep 7, 2021

View reviewed changes

cgardens changed the title ~~add ability to send success notifications on webhook~~ 🎉 add ability to send success notifications on webhook Sep 7, 2021

cgardens merged commit 1ff177d into airbytehq:master Sep 7, 2021

jrhizor mentioned this pull request Sep 8, 2021

Bump Airbyte version from 0.29.15-alpha to 0.29.16-alpha #5925

Merged

tuliren mentioned this pull request Sep 10, 2021

Remove v0.30.0 file-based migration #5984

Merged

prasrvenkat deleted the feat/add_sync_success_notification branch September 21, 2021 22:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🎉 add ability to send success notifications on webhook #5517

🎉 add ability to send success notifications on webhook #5517

prasrvenkat commented Aug 19, 2021 •

edited

Loading

ChristopheDuong Aug 20, 2021 •

edited

Loading

prasrvenkat Aug 20, 2021

cgardens left a comment

prasrvenkat commented Aug 24, 2021

cgardens left a comment

jamakase commented Aug 24, 2021

prasrvenkat commented Aug 25, 2021 •

edited

Loading

marcosmarxm commented Sep 6, 2021

prasrvenkat commented Sep 7, 2021

cgardens left a comment

prasrvenkat commented Sep 7, 2021

michel-tricot commented Sep 7, 2021

jrhizor commented Sep 7, 2021

tuliren commented Sep 10, 2021

sheshan-doye-konvergeai commented Feb 8, 2022

prasrvenkat commented Feb 8, 2022

sheshan-doye-konvergeai commented Feb 9, 2022

🎉 add ability to send success notifications on webhook #5517

🎉 add ability to send success notifications on webhook #5517

Conversation

prasrvenkat commented Aug 19, 2021 • edited Loading

What

How

Recommended reading order

Pre-merge Checklist

Community member or Airbyter

Airbyter

Community member or Airbyter

Airbyter

ChristopheDuong Aug 20, 2021 • edited Loading

Choose a reason for hiding this comment

prasrvenkat Aug 20, 2021

Choose a reason for hiding this comment

cgardens left a comment

Choose a reason for hiding this comment

prasrvenkat commented Aug 24, 2021

cgardens left a comment

Choose a reason for hiding this comment

jamakase commented Aug 24, 2021

prasrvenkat commented Aug 25, 2021 • edited Loading

marcosmarxm commented Sep 6, 2021

prasrvenkat commented Sep 7, 2021

cgardens left a comment

Choose a reason for hiding this comment

prasrvenkat commented Sep 7, 2021

michel-tricot commented Sep 7, 2021

jrhizor commented Sep 7, 2021

tuliren commented Sep 10, 2021

sheshan-doye-konvergeai commented Feb 8, 2022

prasrvenkat commented Feb 8, 2022

sheshan-doye-konvergeai commented Feb 9, 2022

prasrvenkat commented Aug 19, 2021 •

edited

Loading

ChristopheDuong Aug 20, 2021 •

edited

Loading

prasrvenkat commented Aug 25, 2021 •

edited

Loading