Skip to content
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.

Sign up for GitHub

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

[groups] Put child's name in the notification, also added count of additional children #4371

Closed
wants to merge 6 commits into from
Closed

[groups] Put child's name in the notification, also added count of additional children #4371

Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
78ef79f
Resolves #3278 - put child name in notification, also added count of …
homeautomaton Jan 15, 2024
4e9afcf
eslint-recommended fixes
homeautomaton Jan 15, 2024
59d5148
Update server/model/monitor.js
homeautomaton Jan 15, 2024
a1fbc6f
Update server/model/monitor.js
homeautomaton Jan 15, 2024
63a41cf
Update server/model/monitor.js
homeautomaton Jan 15, 2024
d379157
variable name changes
homeautomaton Jan 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 38 additions & 4 deletions server/model/monitor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ class Monitor extends BeanModel {
id: this.id,
name: this.name,
sendUrl: this.sendUrl,
type: this.type,
type: this.type
};

if (this.sendUrl) {
Expand Down Expand Up @@ -383,6 +383,8 @@ class Monitor extends BeanModel {
bean.msg = "Monitor under maintenance";
bean.status = MAINTENANCE;
} else if (this.type === "group") {
let failChild = "";
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit:
please don't hamming code variable names here.
I think downChildrenName and downChildrenCount are better names

Copy link
Author

Choose a reason for hiding this comment

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

I'll go with downChildName and failureCount

Copy link
Collaborator

Choose a reason for hiding this comment

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

names are different despite close in meaning => don't think this is good for long-term maintence

Copy link
Author

Choose a reason for hiding this comment

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

Your petulant complaints have made contributing to this project far more trouble than it is worth. Good day, sir.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we are not on the same page what the job as a maintainer is. I see it as making sure that the project can and will move in the right direction.
This does mean that the primary job is to make sure that the code stays maintainable and readable.

The job is not to blindly accept any contribution.

What might be a source of miscommunication is my usage of nit: comments. These comments can be addressed, but does not have to.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would be better to collect the DOWN-children in a list instead of doing this double accounting (last name + count).
This would allow adding this to the logging output, and I think the code might look better.

Copy link
Author

Choose a reason for hiding this comment

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

Even including the count was beyond the scope of the original request. It's simple, doesn't use any extra memory, and makes the message more informative. I'll stick with what I have.

Copy link
Collaborator

@CommanderStorm CommanderStorm Jan 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I am not convinced that the extra memory of a few child-names will matter for overall performance/memory consumption.
Yes, my approach does consume a bit more memory, but assuming reasonably long names and a reasonable ammount of monitors per group this likely won't be a problem.
I do think adding a separate mechanism for accounting for the failed number is sort of a footgun => I would prefer a list for tracking this ^^

let failCount = 0;
const children = await Monitor.getChildren(this.id);

if (children.length > 0) {
Expand All @@ -395,19 +397,28 @@ class Monitor extends BeanModel {
}
const lastBeat = await Monitor.getPreviousHeartbeat(child.id);

// Only change state if the monitor is in worse conditions then the ones before
// Only change state if the monitor is in worse conditions than the ones before
// lastBeat.status could be null
if (!lastBeat) {
bean.status = PENDING;
} else if (bean.status === UP && (lastBeat.status === PENDING || lastBeat.status === DOWN)) {
bean.status = lastBeat.status;
failChild = child.name;
} else if (bean.status === PENDING && lastBeat.status === DOWN) {
bean.status = lastBeat.status;
failChild = child.name;
}
if (lastBeat.status === DOWN) {
failCount++;
}
}

if (bean.status !== UP) {
bean.msg = "Child inaccessible";
if (failCount <= 1) {
bean.msg = "Child '" + failChild + "' inaccessible";
} else {
bean.msg = "Child '" + failChild + "' and " + (failCount - 1) + " others inaccessible";
}
}
} else {
// Set status pending if group is empty
Expand Down Expand Up @@ -971,7 +982,13 @@ class Monitor extends BeanModel {
} else if (bean.status === MAINTENANCE) {
log.warn("monitor", `Monitor #${this.id} '${this.name}': Under Maintenance | Type: ${this.type}`);
} else {
log.warn("monitor", `Monitor #${this.id} '${this.name}': Failing: ${bean.msg} | Interval: ${beatInterval} seconds | Type: ${this.type} | Down Count: ${bean.downCount} | Resend Interval: ${this.resendInterval}`);
let groupText = "";
if (this.type !== "group" && this.parent !== "") {
let parentName = await this.getParentName();
groupText = ` under group '${parentName}'`;
Comment on lines +987 to +988
Copy link
Collaborator

Choose a reason for hiding this comment

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

Re https://github.com/louislam/uptime-kuma/pull/4371/files#r1452660704

(I can't add the second line to the suggestion)

I did forget to add the second line to the code suggestion.
I don't think the comparison with getPathName is fair as said function does do a lot more.

Suggested change
let parentName = await this.getParentName();
groupText = ` under group '${parentName}'`;
let parent = await Monitor.getParent(this.id);
groupText = ` under group '${parent.name}'`;

}

log.warn("monitor", `Monitor #${this.id} '${this.name}'${groupText}: Failing: ${bean.msg} | Interval: ${beatInterval} seconds | Type: ${this.type} | Down Count: ${bean.downCount} | Resend Interval: ${this.resendInterval}`);
}

// Calculate uptime
Expand Down Expand Up @@ -1526,6 +1543,23 @@ class Monitor extends BeanModel {
]);
}

/**
* Gets Parent name
* @returns {Promise<string>} Name of this monitor's parent
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* @returns {Promise<string>} Name of this monitor's parent
* @returns {Promise<string>} Name of this monitor's parent or '' if no parent exists

*/
async getParentName() {
let name = "";

if (this.parent === null) {
return name;
}

let parent = await Monitor.getParent(this.id);
name = `${parent.name}`;

return name;
}

/**
* Gets Full Path-Name (Groups and Name)
* @returns {Promise<string>} Full path name of this monitor
Expand Down
Loading