Skip to content

Notification when a Thing is down monitering #65

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

Conversation

sailalithkanumuri8
Copy link

I have added a monitoring folder with the code to detect if a thing is down or if it is up and running. If a thing is down, it will send a notification to the email specified in the .env file in the thing-moniter folder. This may need more additional testing: I have tested if all things are up and whether an email is sent if a thing is down. To run my test, type "node test-moniter.js" in the terminal in the root directory.

@danielpeintner
Copy link
Member

Very quick high level comment. The macOS files like .DS_Store should not be part of the PR.
I think it makes sense to add them to .gitignore

Copy link
Member

@egekorkan egekorkan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am sorry but this PR is way too much and feels like AI-generated. You are adding way too many unnecessary things and also code that is not used (e.g. adding node-wot dependency but not using it). Please reconsider how you approach adding features

.env Outdated
Comment on lines 31 to 35
SMTP_HOST=sandbox.smtp.mailtrap.io
SMTP_PORT=2525
SMTP_USER=47aa65a98dd245
SMTP_PASS=b553f1f0235cde
NOTIFICATION_EMAIL=[email protected]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

such information should never be committed to git

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I didn't mean to commit this, I wanted to put it in the .gitignore but I forgot

@@ -256,6 +256,18 @@ services:
memory: 25M
networks:
- things_network
mqtt-broker:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure why an mqtt broker is added

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because I used the broker in the code to do the manual health check, I had to include it. I will remove it though

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is just duplicating the whole .env file . Why?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did that so that I can put it in the .gitignore file as it as the mailtrap credentials, sorry forgot to do that


private async checkMqttThing(thing: ThingConfig, currentStatus: ThingStatus): Promise<void> {
return new Promise((resolve, reject) => {
const client = mqtt.connect(`mqtt://${thing.host}:${thing.port}`, {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const client = mqtt.connect(`mqtt://${thing.host}:${thing.port}`, {
const client = mqtt.connect(`mqtt://${thing.host}:${thing.port}`, {

thing.host should not exist? I am not sure why you are doing this if you are including the mqtt binding of node-wot anyways

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I have removed the code for the mqtt health check and now I am fully relying on node-wot for this

// For now, we'll do a simple UDP socket connection to the port
const dgram = await import('dgram');
return new Promise((resolve, reject) => {
const client = dgram.createSocket('udp4');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please don't write such low level code. you have node wot coap binding anyways?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. I will fix this

// NOTE: The health check now verifies the Thing Description is available over HTTP.
// Ensure your test Things have a TD available at the specified host/port.
const testConfig: MonitorConfig = {
heartbeatInterval: 5000, // 5 seconds for testing
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be configurable

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pretty much all this kind of parameters

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I didn't do it because it is a test file but that works as well


// Start the WoT servient ONCE. This is a critical change.
try {
await this.servient.start();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are you using the servient at all?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, I just realized this


// Let it run for a while to test notifications
console.log('\nMonitoring for 20 seconds...');
await new Promise(resolve => setTimeout(resolve, 20000));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be configurable

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this added?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was part of the mqtt broker code I included before. Deleted it now

test-monitor.js Outdated
{
name: 'http-express-calculator-simple',
protocol: 'http',
host: 'localhost',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in the deployment, they are not localhost. This should be configurable

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok fixed this

@sailalithkanumuri8
Copy link
Author

Ok hopefully I have implemented all your feedback. I have looked throughly on all the changes. I have used servient and node-wot throughout the entire monitoring code structure and removed all the mqtt code. Please let me know if there are any other changes I should make

@danielpeintner
Copy link
Member

mosquitto/log/mosquitto.log is probably not needed. mosquitto/log/mosquitto.db I don't think either.
It is the about the data that gets generated, right?

@sailalithkanumuri8
Copy link
Author

Yeah I don't think that is needed anymore. I will delete it. To fix the ECA issue, I will make a new PR if that is fine with you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants