-
Notifications
You must be signed in to change notification settings - Fork 226
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Discord webhook notification support #1612
Changes from 3 commits
28f5d35
172597e
d3810db
d83773a
84fe58b
92d9573
cc48f1d
8667f74
0bb7eae
d3fb41a
8f94062
1d9f94c
e44cbf3
aa977ce
964a923
e9bcf66
64c5aa9
01e73fe
d8379f7
f46b076
ccd6c58
ef41712
fa50706
67da574
023d943
012483d
8007daa
70ffe1b
12d9f15
07882e0
8409571
55c1a1c
ff2b3b4
6152679
e4e3f9c
f22b67a
d710ec0
c4168a5
afc9d46
03a938d
5be17af
4ef2b29
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
const SERVICE_NAME = "NotificationService"; | ||
import axios from 'axios'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No imports in the services please, it makes testing much more difficult 👍 There shouldn't be any imports anywhere except in the root server definition really. The exception being success/error messages There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
|
||
class NotificationService { | ||
static SERVICE_NAME = SERVICE_NAME; | ||
|
@@ -16,6 +17,77 @@ class NotificationService { | |
this.logger = logger; | ||
} | ||
|
||
async sendDiscordNotification(networkResponse, webhookUrl) { | ||
const { monitor, status } = networkResponse; | ||
const message = { | ||
content: `Monitor ${monitor.name} is ${status ? "up" : "down"}. URL: ${monitor.url}` | ||
}; | ||
|
||
try { | ||
await axios.post(webhookUrl, message); | ||
return true; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Network operations should be carried out from the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Complete. |
||
} catch (error) { | ||
this.logger.error({ | ||
message: error.message, | ||
service: this.SERVICE_NAME, | ||
method: "sendDiscordNotification", | ||
stack: error.stack, | ||
}); | ||
return false; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this error meant to be swallowed and operations meant to continue in event of error? If not it should have method specifics added to it and rethrown to be caught by middleware There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes! |
||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this if/else ladder meant to fall through at the end? Should we continue execution if the platform is not one of slack, discord, or telegram? Or are we meant to return early? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, sorry, you are right. It should not continue execution. I wrote an else to return early. |
||
} | ||
|
||
async sendSlackNotification(networkResponse, webhookUrl) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
We can simplify and just simply have
All webhooks are going to be pretty much the same There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
const { monitor, status } = networkResponse; | ||
const message = { | ||
text: `Monitor ${monitor.name} is ${status ? "up" : "down"}. URL: ${monitor.url}` | ||
}; | ||
|
||
try { | ||
await axios.post(webhookUrl, message); | ||
return true; | ||
} catch (error) { | ||
this.logger.error({ | ||
message: error.message, | ||
service: this.SERVICE_NAME, | ||
method: "sendSlackNotification", | ||
stack: error.stack, | ||
}); | ||
return false; | ||
} | ||
} | ||
|
||
async sendTelegramNotification(networkResponse, address) { | ||
const { monitor, status } = networkResponse; | ||
|
||
const [botToken, chatId] = address.split('|').map(part => part?.trim()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is odd, we are using the address to store a token and and ID? Creative solution, but I think it would be better to properly store these in their own fields rather than hack something together to make use of the address field There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
|
||
if (!botToken || !chatId) { | ||
return false; | ||
} | ||
|
||
const message = { | ||
chat_id: chatId, | ||
text: `Monitor ${monitor.name} is ${status ? "up" : "down"}. URL: ${monitor.url}`, | ||
}; | ||
|
||
const url = `https://api.telegram.org/bot${botToken}/sendMessage`; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Base URLs should be refcatored to constant vars in one place for easy maintenance There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
|
||
try { | ||
await axios.post(url, message, { | ||
headers: { | ||
'Content-Type': 'application/json', | ||
}, | ||
}); | ||
return true; | ||
} catch (error) { | ||
return false; | ||
ajhollid marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
} | ||
|
||
|
||
|
||
|
||
/** | ||
* Sends an email notification for hardware infrastructure alerts | ||
* | ||
|
@@ -57,19 +129,22 @@ class NotificationService { | |
|
||
async handleStatusNotifications(networkResponse) { | ||
try { | ||
//If status hasn't changed, we're done | ||
if (networkResponse.statusChanged === false) return false; | ||
|
||
// if prevStatus is undefined, monitor is resuming, we're done | ||
ajhollid marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if (networkResponse.prevStatus === undefined) return false; | ||
const notifications = await this.db.getNotificationsByMonitorId( | ||
networkResponse.monitorId | ||
); | ||
|
||
|
||
const notifications = await this.db.getNotificationsByMonitorId(networkResponse.monitorId); | ||
|
||
for (const notification of notifications) { | ||
if (notification.type === "email") { | ||
this.sendEmail(networkResponse, notification.address); | ||
} else if (notification.type === "discord") { | ||
this.sendDiscordNotification(networkResponse, notification.address); | ||
} else if (notification.type === "slack") { | ||
this.sendSlackNotification(networkResponse, notification.address); | ||
} else if (notification.type === "telegram") { | ||
this.sendTelegramNotification(networkResponse, notification.address); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. these should all be updated to use the webhook function? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
} | ||
|
||
// Handle other types of notifications here | ||
} | ||
return true; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need all these types anymore do we? They are all of type "webhook" now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup! You are right. All done.