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.

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

On Page reload monitors do not load #3494

Closed
2 tasks done
charlieporth1 opened this issue Jul 27, 2023 · 22 comments · Fixed by #3515 or #5025
Closed
2 tasks done

On Page reload monitors do not load #3494

charlieporth1 opened this issue Jul 27, 2023 · 22 comments · Fixed by #3515 or #5025
Labels
bug Something isn't working

Comments

@charlieporth1
Copy link

charlieporth1 commented Jul 27, 2023

⚠️ Please verify that this bug has NOT been raised before.

  • I checked and didn't find similar issue

🛡️ Security Policy

Description

I'm using the docker image on a Nvidia TX2. Ubuntu 22.04. ARM64 with eMMC for storage
It should be noted I have 400+ monitors with groups. Storing data for 32 days
On page reload I get in the console log

index.560f0f4a.js:492 Uncaught (in promise) TypeError: Cannot read properties of undefined (reading 'name')
    at bw.<anonymous> (index.560f0f4a.js:492:12765)
    at Sn.emit (index.560f0f4a.js:487:15555)
    at bw.emitEvent (index.560f0f4a.js:487:42368)
    at bw.onevent (index.560f0f4a.js:487:42171)
    at bw.onpacket (index.560f0f4a.js:487:41839)
    at Sn.emit (index.560f0f4a.js:487:15555)
    at index.560f0f4a.js:487:47862

nothing useful in docker logs

On docker restart <container id> uptime kuma will load monitors

👟 Reproduction steps

1.) Use docker image,
2.) load monitors
3.) Reload page
4.) Find monitors do not load and console log message will appear every minute

👀 Expected behavior

Monitors should load

😓 Actual Behavior

They do not

🐻 Uptime-Kuma Version

1.22.1

💻 Operating System and Arch

Ubuntu 22.04

🌐 Browser

Chrome Version 115.0.5790.110 (Official Build) (64-bit)

🐋 Docker Version

Version: 20.10.21

🟩 NodeJS Version

No response

📝 Relevant log output

No response

@charlieporth1 charlieporth1 added the bug Something isn't working label Jul 27, 2023
@CommanderStorm
Copy link
Collaborator

CommanderStorm commented Jul 27, 2023

How many monitors do you have?
Running anything which is sort of IO-Heavy on EMMC is not recomented, as eMMC is dead-slow => causes database problems quickly

(might this be fixed by one of the performance fixes which have gone into 1.23?)

@charlieporth1
Copy link
Author

charlieporth1 commented Jul 27, 2023

How many monitors do you have? Running anything which is sort of IO-Heavy on EMMC is not recomented, as eMMC is dead-slow => causes database problems quickly

(might this be fixed by one of the performance fixes which have gone into 1.23?)

eMMC is faster than a harddrive and faster than an SD card.
eMMC 5.1 is 400 Mib/s
410+ montiors

@CommanderStorm
Copy link
Collaborator

You are not comparing in the right direction:
eMMC is still really slow, as is essentially is an SD-Card soldered to the board.
If compared to SSDs or NVMEs the performance difference is significant

=> you are limited to SC-Card IO speeds
=> with 410 monitors, problems are inevitable, sorry

If you like, you could help us in evaluating if the suggestions in #3286 or the performance fixes which have gone into 1.23 would help solve this problem.

Note that this might also be a symptom of #3463
(unclear with no context other than "not loading" 😅)

@charlieporth1
Copy link
Author

charlieporth1 commented Jul 27, 2023

You are not comparing in the right direction: eMMC is still really slow, as is essentially is an SD-Card soldered to the board. If compared to SSDs or NVMEs the performance difference is significant

=> you are limited to SC-Card IO speeds => with 410 monitors, problems are inevitable, sorry

If you like, you could help us in evaluating if the suggestions in #3286 or the performance fixes which have gone into 1.23 would help solve this problem.

Note that this might also be a symptom of #3463 (unclear with no context other than "not loading" 😅)

It should be noted that by viewing iotop only a couple of Mib/s are being used when the page is being reloaded
It should also be noted when restarting the docker container it loads instantly

image
image

This is a simple dd disk test
image

@CommanderStorm
Copy link
Collaborator

So likely this is one of the symptoms of #3463?
(I am not a IO-expert when it comes to hardware)

@chakflying
Copy link
Collaborator

That specific error message can be solved with #3443.

But yes the root cause is most likely #3463.

@CommanderStorm
Copy link
Collaborator

One thing that does not quite fit into the puzzle (as far as I understand it), though:
@charlieporth1 said that:

when restarting the docker container it loads instantly

=> I think after #3463 there might be another problem (I hope not) which will then become evident

@charlieporth1
Copy link
Author

charlieporth1 commented Jul 27, 2023

One thing I forgot to say as well is that docker logs while it doesn't contain any useful information- it does continue the monitors

@CommanderStorm Did you catch the first thing I said with the console log error output in the original post

@CommanderStorm
Copy link
Collaborator

CommanderStorm commented Jul 27, 2023

What do you mean? I might not have.

The Running Hypothesis from Nelson seems logical.
It fits the data, assuming on the second load the race condition from #3443 does not cause problems.
Restarting the container would not have an effect on this, though, which leads me to believe that after these problems are fixed, there might be different problems waiting for us (I hope not, but let's be real)

@charlieporth1
Copy link
Author

charlieporth1 commented Jul 27, 2023

On page reload I get in the console log when monitors do not load. To add to what I said originally the restarting the docker container will work no matter what even if on a new computer with no web page open or with the page open and then restarting

index.560f0f4a.js:492 Uncaught (in promise) TypeError: Cannot read properties of undefined (reading 'name')
    at bw.<anonymous> (index.560f0f4a.js:492:12765)
    at Sn.emit (index.560f0f4a.js:487:15555)
    at bw.emitEvent (index.560f0f4a.js:487:42368)
    at bw.onevent (index.560f0f4a.js:487:42171)
    at bw.onpacket (index.560f0f4a.js:487:41839)
    at Sn.emit (index.560f0f4a.js:487:15555)
    at index.560f0f4a.js:487:47862

@CommanderStorm
Copy link
Collaborator

CommanderStorm commented Jul 27, 2023

That is why I said it *might be (I find this unlikely) a different issue.

A better solution than accepting the current behaviour could be:

  • reduce your retention (my recommendation) or
  • shard uptime-kuma into multiple instances with fewer monitors.

@vishalsabhaya
Copy link
Contributor

@CommanderStorm @louislam
Sorry, I commented on the closed issue. I don't have expertise in nodes.js & vue.js but trying to learn.

I am facing a similar issue on my local pc. i pull latest code of git & try to run with npm run start-server-dev & npm run start-frontend-dev.

I did a load test with 410 URL monitoring including 40 URLs that are down.
it is taking more than 3 to 4 minute to load the dashboard or adding new/update URL

I debug code & I found that.

uptime-kuma-serever.js

async getMonitorJSONList(userID) {
        let result = {};

        let monitorList = await R.find("monitor", " user_id = ? ORDER BY weight DESC, name", [
            userID,
        ]);

        for (let monitor of monitorList) {
            result[monitor.id] = await monitor.toJSON();
        }

        return result;
    }

monitor.toJSON() is taking time around 3 minute.

frontend taking time around 20~30 sec after emit monitorList
because it calculate up/down total count each & every request. it should not be once only?

@chakflying
Copy link
Collaborator

I don't think the toJSON() function calculates uptime for a monitor, but there are a whole bunch of extra DB queries like configured notifications, getTags, getPath, etc. Definitely a place of optimization on reducing the number of queries as much as possible.

@vishalsabhaya
Copy link
Contributor

@chakflying
Yes you are correct.
I mean query inside tojson method in monitor modal.

i just commented query still it’s slow improve performance 2x.

Current: 1 sec it proceed 4 to 5 urls
After comment: 1 sec it proceed 10 to 11 urls

I checked with putting log

@CommanderStorm
Copy link
Collaborator

CommanderStorm commented Aug 15, 2024

Yes, a bunch of db-queries are done in that.
Maybe moving things into Promise.all where appropriate can improve latency as a simple "fix" without much work needed.

If by After comment you mean "after commit", we would appreciate a PR.
As always: if you have concrete ideas, PRs are appreciated ^^

@vishalsabhaya
Copy link
Contributor

vishalsabhaya commented Aug 15, 2024

@CommanderStorm
Sorry i am beginner. I understand the problem but i don’t know how to fix it.

Which code we have to move in promise.all?

After comment means not commit. Removed code of query & asign blank array.

i am ruby on rails developer. So i heard that it call N+ 1 query problem.

we need to take all data in single query.

@CommanderStorm
Copy link
Collaborator

CommanderStorm commented Aug 15, 2024

I would try moving the promises from this loop behind a Promise.all.
=> this way, monior n does not have to wait for monitor n-1.

for (let monitor of monitorList) {
result[monitor.id] = await monitor.toJSON();
}

All data in a single query might not be possible.
The cases where we are doing different queries, we are getting data for different tables.

@vishalsabhaya
Copy link
Contributor

@CommanderStorm
Thank you very much. I will try to understand.

should we register new issue?

@vishalsabhaya
Copy link
Contributor

vishalsabhaya commented Aug 16, 2024

@CommanderStorm
thank you for suggestion.
i have tested with 409 URLs.

total time
before: 1 min 42 sec
after: 21 sec

i have changed logic like below. it reduces time by 4.86x.

    /**
     * Get a list of monitors for the given user.
     * @param {string} userID - The ID of the user to get monitors for.
     * @returns {Promise<object>} A promise that resolves to an object with monitor IDs as keys and monitor objects as values.
     *
     * Generated by Trelent
     */
    async getMonitorJSONList(userID) {
        let result = {};

        let monitorList = await R.find("monitor", " user_id = ? ORDER BY weight DESC, name", [
            userID,
        ]);

        log.debug("monitor", ""+JSON.stringify(monitorList));
        
        // for (let monitor of monitorList) {
        //     result[monitor.id] = await monitor.toJSON();
        // }

        // Create an array of promises to convert each monitor to JSON in parallel
        const monitorPromises = monitorList.map(monitor => monitor.toJSON().then(json => {
            return { id: monitor.id, json };
        }));
        // Wait for all promises to resolve
        const monitors = await Promise.all(monitorPromises);
        // Populate the result object with monitor IDs as keys and JSON objects as values
        monitors.forEach(monitor => {
            result[monitor.id] = monitor.json;
        });
        return result;
    }

uptime-kuma/server/server.js

/**
 * Function called after user login
 * This function is used to send the heartbeat list of a monitor.
 * @param {Socket} socket Socket.io instance
 * @param {object} user User object
 * @returns {Promise<void>}
 */
async function afterLogin(socket, user) {
    socket.userID = user.id;
    socket.join(user.id);

    let monitorList = await server.sendMonitorList(socket);
    await Promise.allSettled([
        sendInfo(socket),
        server.sendMaintenanceList(socket),
        sendNotificationList(socket),
        sendProxyList(socket),
        sendDockerHostList(socket),
        sendAPIKeyList(socket),
        sendRemoteBrowserList(socket),
    ]);

    await StatusPage.sendStatusPageList(io, socket);

    // for (let monitorID in monitorList) {
    //     await sendHeartbeatList(socket, monitorID);
    // }
    // for (let monitorID in monitorList) {
    //     await Monitor.sendStats(io, monitorID, user.id);
    // }

     // Create an array to store the combined promises for both sendHeartbeatList and sendStats
     const monitorPromises = [];
     for (let monitorID in monitorList) {
         // Combine both sendHeartbeatList and sendStats for each monitor into a single Promise
         monitorPromises.push(
             Promise.all([
                 sendHeartbeatList(socket, monitorID),
                 Monitor.sendStats(io, monitorID, user.id)
             ])
         );
     }

     // Await all combined promises
     await Promise.all(monitorPromises);

     log.debug("server", "sendHeartbeatList and sendStats end");

    // Set server timezone from client browser if not set
    // It should be run once only
    if (! await Settings.get("initServerTimezone")) {
        log.debug("server", "emit initServerTimezone");
        socket.emit("initServerTimezone");
    }
}

I am trying to debug more for monitor.tojson how we can reduce it. I will generate pr after analysis & changes

@CommanderStorm
Copy link
Collaborator

tens of seconds is still unacceptably slow, but a very nice improvement for little effort 👍🏻
Would be very happy about a PR (reviewing issue-code is not quite sustainable)

@vishalsabhaya
Copy link
Contributor

@CommanderStorm

ok, i will do pr ASAP for code review.

I already optimized the query also. so it will reduce to 10 to 11 sec. but I need to test more because it will depend on network speed.

@vishalsabhaya
Copy link
Contributor

@CommanderStorm @louislam

I have generated pr.
#5025

performance improved 10.4x. I am ready to change if you guys found wrong in my pr.

still, I suggest improving add/update/delete monitor. Currently, we have to wait until all monitors are reloaded after crud. I am not debugged yet. but I will definitely spend time on it. hope to find something.

monitor url count: 415
current:
cpu: 1 memory 2 gb: 1 min 44 sec

after changes

database: aws rds marinadb db.t3.micro(memory: 1gb, cpu: 2vcpu)

with docker run on mac:
cpu: 0.5 memory 1 gb: 21 sec
cpu: 1 memory 2 gb: 11 sec
cpu: 1.5 memory 3 gb: 10 sec
cpu: 2 memory 4 gb: 10 sec

with ESC Farget(Linux/ARM64):
cpu: 0.5 memory 1 gb: 31 sec
cpu: 1 memory 2 gb: 6 sec
cpu: 2 memory 4 gb: 5 sec

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
4 participants