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

Jump to bottom

Perf: Fully batch sendImportantHeartbeatList #3463

Closed
wants to merge 1 commit into from
Closed

Perf: Fully batch sendImportantHeartbeatList #3463

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
19 changes: 15 additions & 4 deletions server/client.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,22 +70,33 @@ async function sendHeartbeatList(socket, monitorID, toUser = false, overwrite =
/**
* Important Heart beat list (aka event list)
* @param {Socket} socket Socket.io instance
* @param {number} monitorID ID of monitor to send heartbeat history
* @param {number} monitorID ID of monitor to send heartbeat history, or null for all monitors
* @param {boolean} [toUser=false] True = send to all browsers with the same user id, False = send to the current browser only
* @param {boolean} [overwrite=false] Overwrite client-side's heartbeat list
* @returns {Promise<void>}
*/
async function sendImportantHeartbeatList(socket, monitorID, toUser = false, overwrite = false) {
const timeLogger = new TimeLogger();

let list = await R.find("heartbeat", `
let list = [];

if (monitorID == null) {
// Send important beats for all monitors
list = await R.find("heartbeat", `
important = 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think these are functionally identical:
This is finding the top 5k important heartbeats while the other one is finding the top 500 important heartbeats for each monitor.

=> Lets assume a monitor runs every 20s and every heartbeat is important => 4320 beats
=> have 2 monitors which produce beats like this and one which pings once per day and the results will not be identical
=> I think this is missing a group by

Copy link
Collaborator Author

@chakflying chakflying Jul 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I see your point that if the important beat list grew too long, it would lead to different behavior. I'd say for the initial dashboard load, 5000 is more than enough. That's 250 pages and no one will bother going through the unsearchable list. I think I have two options here:

  • Add a function to send the important heartbeat list when the user clicks on a monitor in the dashboard, in case there are any missing, or
  • Modify the SQL to replicate the original behavior. A quick search and the only way to do it seems to be a nested SELECT, looks a bit ugly.

Why would GROUP BY be needed here when we are not using an aggregate function?

ORDER BY time DESC
LIMIT 5000
`);
} else {
list = await R.find("heartbeat", `
monitor_id = ?
AND important = 1
ORDER BY time DESC
LIMIT 500
`, [
monitorID,
]);
monitorID,
]);
}

timeLogger.print(`[Monitor: ${monitorID}] sendImportantHeartbeatList`);

Expand Down
4 changes: 1 addition & 3 deletions server/server.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1681,9 +1681,7 @@ async function afterLogin(socket, user) {
await sendHeartbeatList(socket, monitorID);
}

for (let monitorID in monitorList) {
await sendImportantHeartbeatList(socket, monitorID);
}
await sendImportantHeartbeatList(socket, null);

for (let monitorID in monitorList) {
await Monitor.sendStats(io, monitorID, user.id);
Expand Down
10 changes: 10 additions & 0 deletions src/mixins/socket.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,16 @@ export default {
});

socket.on("importantHeartbeatList", (monitorID, data, overwrite) => {
if (monitorID == null) {
data.forEach(heartbeat => {
if (this.importantHeartbeatList[heartbeat.monitorID] === undefined) {
this.importantHeartbeatList[heartbeat.monitorID] = [ heartbeat ];
} else {
this.importantHeartbeatList[heartbeat.monitorID].push(heartbeat);
}
});
return;
}
if (! (monitorID in this.importantHeartbeatList) || overwrite) {
this.importantHeartbeatList[monitorID] = data;
} else {
Expand Down