Enable email notifier to send emails for health check failures #244

prakhar10 · 2024-02-10T18:22:51Z

This change enables to send email notifications for healthcheck failures on backend trino clusters.

New fields added in the NotifierConfiguration.java class:
enabled - To enable or disable sending emails for healthcheck failures
gatewayInstance - To specify gateway instance name in case there are multiple instances. This will be displayed in the email subject so that one can know in which instance the cluster is unhealthy.
customContent - To provide custom email body in case the default content is not very helpful.

cla-bot · 2024-02-10T18:22:54Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

prakhar10 · 2024-02-10T19:04:47Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

I have already sent this.

ebyhr

Just skimmed.

gateway-ha/src/main/java/io/trino/gateway/ha/clustermonitor/HealthChecker.java

gateway-ha/src/main/java/io/trino/gateway/ha/notifier/EmailNotifier.java

gateway-ha/src/main/java/io/trino/gateway/ha/module/ClusterStateListenerModule.java

cla-bot · 2024-02-14T20:21:42Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

willmostly · 2024-02-16T18:37:24Z

@prakhar10 could you squash your commits?

I think this enhancement should include some tests, there are currently none for notifications.

gateway-ha/src/main/java/io/trino/gateway/ha/clustermonitor/HealthChecker.java

gateway-ha/src/main/java/io/trino/gateway/ha/notifier/EmailNotifier.java

gateway-ha/src/test/java/io/trino/gateway/ha/clustermonitor/TestHealthCheck.java

gateway-ha/src/test/java/io/trino/gateway/module/TestClusterStateListenerModule.java

willmostly

Good progress, some suggestions on the tests

willmostly · 2024-02-23T17:35:11Z

gateway-ha/src/main/java/io/trino/gateway/ha/module/ClusterStateListenerModule.java

+        ImmutableList.Builder<TrinoClusterStatsObserver> observerBuilder = ImmutableList.builder();
+        observerBuilder.add(new HealthCheckObserver(mgr));
+        observerBuilder.add(new ClusterStatsObserver(backendStateManager));
+        if (notifierConfiguration.isEnabled()) {


Remove

NotifierConfiguration notifierConfiguration = getConfiguration().getNotifier();

above and use

if (monitorConfig.getNotifier().isEnabled()) {

I think we cannot get monitorConfig.getNotifier() because monitorConfig is an object of MonitorConfiguration and not NotifierConfiguration

willmostly · 2024-02-23T17:46:33Z

gateway-ha/src/test/java/io/trino/gateway/ha/notifier/TestEmailNotifier.java

+    @Test
+    public void testSendNotificationCustomContent()
+    {
+        NotifierConfiguration notifierConfiguration = mock(NotifierConfiguration.class);


A mock isn't needed here - just initialize directly and use the setters. This applies to the other tests using NotifierConfiguration as well

willmostly · 2024-02-23T17:49:00Z

gateway-ha/src/test/java/io/trino/gateway/ha/notifier/TestEmailNotifier.java

+
+        EmailNotifier emailNotifier = new EmailNotifier(notifierConfiguration);
+
+        emailNotifier.sendNotification("Test Subject", "Test Content");


Its not clear to me what this is testing. Can you assert a success condition, or is the test just that this runs without throwing an error?

yeah it was just basically to see if there are no errors produced since the method returns void

willmostly · 2024-02-23T18:03:19Z

gateway-ha/src/test/java/io/trino/gateway/ha/clustermonitor/TestHealthChecker.java

+    @Test
+    public void testNotifyUnhealthyCluster()
+    {
+        Notifier notifier = Mockito.mock(Notifier.class);


This test suite may be better with a TestingNotifier implementation of Notifier instead of of mocks - wdyt @prakhar10 and @ebyhr? Sth like

public class TestingNotifier implements Notifier { List<Notification> notifications = new ArrayList<>(); public void sendNotification(String from, List<String> recipients, String subject, String content) { recipients.forEach(recipient -> notifications.add(new Notification(from, recipient, subject, content)) } public List<Notification> getNotifications() { return notifications; } class Notification { //should have fields for rom, recipient, subject, content } }

Then tests can observe the notifications produced rather than testing for internal details like method invocations.

willmostly · 2024-02-23T18:45:04Z

gateway-ha/src/test/java/io/trino/gateway/module/TestClusterStateListenerModule.java

+public class TestClusterStateListenerModule
+{
+    @Test
+    public void testGetClusterStatsObservers()


Rename to testClusterStateListenerModule

mosabua · 2024-03-20T16:09:07Z

@prakhar10 can you address to the feedback comments

- Includes new stack with Semi UI, Vite, React, TypeScript, and E-charts framework as major components - Removes Dropwizard UI usage, further enabling removal of Dropwizard altogether

* Clean brew install in quick start doc * Use gateway version 7 * Use PostgreSQL docker container

- Including recommendation for dev and test usage only

- change langing page to /dashboard - Add hyperlink to Trino Gateway in UI

The restUserInfo method in the LoginResource class and the findQueryHistory method in the GatewayWapResource were using string manipulation of the getMemberOf method from the LbPrincipal to do role authorization. This change fixes the role authorization to be consistent throughout the app by utilizing the isUserInRole method that keeps the context of the LbAuthorization when checking user access to the ADMIN, USER, and API roles.

…#318)

- with process forwarding config only for now

Ensure objects are serializable without AUTO_DETECT_GETTERS. Convert request objects into record to reduce boilerplate.

prakhar10 · 2024-04-15T04:26:50Z

Branch was messed up, created a new PR - #319

ebyhr changed the title ~~fix: Enable email notifier to send emails for health check failures~~ Enable email notifier to send emails for health check failures Feb 14, 2024

ebyhr reviewed Feb 14, 2024

View reviewed changes

prakhar10 force-pushed the email-notifier branch from 63c4aff to e21ac36 Compare February 16, 2024 17:40

cla-bot bot added the cla-signed label Feb 16, 2024

prakhar10 force-pushed the email-notifier branch from f3d067d to 73e9a67 Compare February 16, 2024 22:04

ebyhr reviewed Feb 17, 2024

View reviewed changes

prakhar10 force-pushed the email-notifier branch from 7ef8848 to 8d57974 Compare February 21, 2024 04:30

Enable email notifier to send emails for healthcheck failures

6db0428

prakhar10 force-pushed the email-notifier branch from 8d57974 to 6db0428 Compare February 22, 2024 13:51

willmostly requested changes Feb 23, 2024

View reviewed changes

oneonestar mentioned this pull request Apr 4, 2024

Simplify module class inheritance #300

Merged

prakhar10 and others added 14 commits April 9, 2024 13:52

fix test classes

482100f

remove unused imports

100d9f7

Allow customization of test containers image name

b74ffbb

Disallow JUnit assertions

746f2da

Enforce errorprone UnnecessaryParentheses (trinodb#274)

dd3aefe

Update testcontainers to 1.19.6

9b3b104

Fix typo in issue link at release note (trinodb#275)

63d72be

Replace user interface with a new modern UI

59ad6f2

- Includes new stack with Semi UI, Vite, React, TypeScript, and E-charts framework as major components - Removes Dropwizard UI usage, further enabling removal of Dropwizard altogether

Rename SQL files to clarify the target database

b27ae2c

Merge yml files into docker-compose.yml

266b8f9

Fix query id matching in request querystring

f5e4c65

Get cluster health from v1/info

c6b8026

Improve and doc release process

2a7d13e

Use Airlift logger

1e4fa4f

oneonestar and others added 28 commits April 14, 2024 23:11

Replace WireMock with okhttp3:mockwebserver

5a42549

Update OkHttp to 4.12.0

a883454

Fix the error caused by the missing 'audiences' configuration

7437aa0

Add configurable routers and query count based router

a6f91a4

Improve quickstart doc

e2d19ad

* Clean brew install in quick start doc * Use gateway version 7 * Use PostgreSQL docker container

Addhealth check conf in docker compose

5e7de11

- Including recommendation for dev and test usage only

Use JDBI for selectors

b235217

Use JDBI for ResourceGroups

bd74d5f

Remove activejdbc

dc82f80

Improve synchronization in QueryCountBasedRouter

682c323

Update Jdbi to 3.45.1

7241368

Allow page permissions in UI

97ac16e

- change langing page to /dashboard - Add hyperlink to Trino Gateway in UI

Allow pnpmRegistryURL to be configurable

6d66a15

Refactor JDBI related classes

ca3671a

Add /static folder to gitignore

26f7810

Remove reflections

fa7f5cf

Remove hardcoded registry URLs in pnpm-lock file

de68b38

Remove dropwizard configuration and jackson

b5f4db6

fix typo in method names

3d6e7fc

Add airlift stats

fad2cca

Simplify module class inheritance

9d32652

Remove AppConfiguration

5e294f4

Update JWKS RSA to 0.22.1

b2c4b00

Remove console logging in web UI

bc1a29a

Standardize the path for the gateway jar on the docker image (trinodb…

8bd54d0

…#318)

Add Trino configuration docs

d8adbbe

- with process forwarding config only for now

Annotate JsonProperty on getter

950276a

Ensure objects are serializable without AUTO_DETECT_GETTERS. Convert request objects into record to reduce boilerplate.

prakhar10 closed this Apr 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable email notifier to send emails for health check failures #244

Enable email notifier to send emails for health check failures #244

prakhar10 commented Feb 10, 2024

cla-bot bot commented Feb 10, 2024

prakhar10 commented Feb 10, 2024

ebyhr left a comment

cla-bot bot commented Feb 14, 2024

willmostly commented Feb 16, 2024

willmostly left a comment

willmostly Feb 23, 2024

prakhar10 Mar 13, 2024

willmostly Feb 23, 2024

willmostly Feb 23, 2024

prakhar10 Mar 13, 2024

willmostly Feb 23, 2024

willmostly Feb 23, 2024

mosabua commented Mar 20, 2024

prakhar10 commented Apr 15, 2024


		EmailNotifier emailNotifier = new EmailNotifier(notifierConfiguration);

		emailNotifier.sendNotification("Test Subject", "Test Content");

Enable email notifier to send emails for health check failures #244

Enable email notifier to send emails for health check failures #244

Conversation

prakhar10 commented Feb 10, 2024

cla-bot bot commented Feb 10, 2024

prakhar10 commented Feb 10, 2024

ebyhr left a comment

Choose a reason for hiding this comment

cla-bot bot commented Feb 14, 2024

willmostly commented Feb 16, 2024

willmostly left a comment

Choose a reason for hiding this comment

willmostly Feb 23, 2024

Choose a reason for hiding this comment

prakhar10 Mar 13, 2024

Choose a reason for hiding this comment

willmostly Feb 23, 2024

Choose a reason for hiding this comment

willmostly Feb 23, 2024

Choose a reason for hiding this comment

prakhar10 Mar 13, 2024

Choose a reason for hiding this comment

willmostly Feb 23, 2024

Choose a reason for hiding this comment

willmostly Feb 23, 2024

Choose a reason for hiding this comment

mosabua commented Mar 20, 2024

prakhar10 commented Apr 15, 2024