louislam / uptime-kuma

A fancy self-hosted monitoring tool
https://uptime.kuma.pet
MIT License
56.82k stars 5.11k forks source link

On Page reload monitors do not load #3494

Closed charlieporth1 closed 1 year ago

charlieporth1 commented 1 year ago

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

πŸ›‘οΈ 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

CommanderStorm commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 https://github.com/louislam/uptime-kuma/issues/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 https://github.com/louislam/uptime-kuma/pull/3463 (unclear with no context other than "not loading" πŸ˜…)

charlieporth1 commented 1 year ago

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 commented 1 year ago

So likely this is one of the symptoms of https://github.com/louislam/uptime-kuma/pull/3463? (I am not a IO-expert when it comes to hardware)

chakflying commented 1 year ago

That specific error message can be solved with #3443.

But yes the root cause is most likely #3463.

CommanderStorm commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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:

vishalsabhaya commented 1 month ago

@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 commented 1 month ago

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 commented 1 month ago

@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 commented 1 month ago

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 commented 1 month ago

@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 commented 1 month ago

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.

https://github.com/louislam/uptime-kuma/blob/bab771df056e317b2361e9a5102c204dfc89a8db/server/uptime-kuma-server.js#L222-L224

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 commented 1 month ago

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

should we register new issue?

vishalsabhaya commented 1 month ago

@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 commented 1 month ago

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 commented 1 month ago

@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 commented 1 month ago

@CommanderStorm @louislam

I have generated pr. https://github.com/louislam/uptime-kuma/pull/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