-
-
Notifications
You must be signed in to change notification settings - Fork 361
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
feat: collects all stats like cpu and mem in background for up to 5 minutes #2740
Conversation
amir20
commented
Jan 31, 2024
- chore: cleans up goroutines to bubble with errors
- feat: streams all stats in background routine
@EDIflyer @akash-ramaswamy I wonder if you two can take a moment to test this PR with |
Hi @amir20 - have tested the
Sorry not sure if that's helpful or not!! I then tried restarting |
This is actually a bug. I just pushed a fix. Try pulling again So the change is some what subtle but big if you realize what is happening. Before, Dozzle was collecting the stats in the browser. Meaning, if you closed the tab and open Dozzle again, it would have lost the history of CPU and mem. Now it should continue while the tabs are closed. |
Thanks @akash-ramaswamy try again. I think I had a bug. Look for the stats on the top right. It should have last 300 seconds of data even when the browser has been closed. I guess it's good thing that not much is noticeable because a lot is happening behind the scenes. Which by the way this might be a good start to doing a notifications 🤓 |
yeah @amir20 you are right 🤓. I noticed that the stopped containers do not have stats in older version. But this version maintains stats even if the tab is closed and reopened.
Regarding this point, I couldn't exactly feel this. But still, the stopped container's greyed out color is missing :) |
Still have this issue @amir20 :) |
Ah wait a minute, I think you are right. If the container stops, I don't update the status. Can you confirm the steps to reproduce?
Does that sound about right? I knew doing a lot of logic would be complicated. :) |
Yes! Exactly these steps reproduce the bug I am facing :) |
@amir20 this also doesn't update for me in dozzle it seems :) I meant the color of the listed container doesn't grey out. |
@akash-ramaswamy ok try again. I realized this is a little more complicated. Will do more testing on my side. |
I think something broke and I can't figure out what I did 🤔 So pause testing until I figure it out |
OK try again @akash-ramaswamy |
Haha was stuck in a meeting - looks like you've been hard at it! Still need testing or all looking good now? |
Test master now if you can. |
@EDIflyer I wasn't able to reproduce that. I did
Screen.Recording.2024-02-01.at.1.36.20.PM.movHere is a video. The container shows up and then grays out. How are you able to do that? |
Haha - I'm stopping it from within Dozzle using the new functionality from a month or so ago 😉 For the |
@akash-ramaswamy actually worked on that 🎉 @EDIflyer I tried doing exactly the same thing and it worked. Do you have possibly an older version? Can you pull master again and try again? |
Hmm I had literally just re pulled it and it seemed to fix the other issues. Will try again tomorrow. |
Hi @amir20 ! I've spent some time reviewing the code changes, and I noticed the shift from streaming stats at the time of browser requests to including them with the I understand the technical adjustments, but could you kindly provide a brief explanation of why this change was necessary? I appreciate your time and clarification! 😊 FYI: (just an immediate fact I noticed) |
Excellent question @akash-ramaswamy! Let me try to explain it and hopefully the need for this makes sense. I use Docker a lot and when a container dies, I have to investigate the cause. Sometimes the errors are shown in the log but other times it might be an "out of memory" error. So I would need to go back in time and find the container to see how much memory it was using. Unfortunately, this is not possible right now with Docker as the stats are streamed but not stored. Something like Datadog can be used but that costs money. Dozzle also doesn't help here because as you are aware it requires the browser tab to be open. You can see this happening right now if you open Dozzle, wait a couple a minutes and then refresh Dozzle. All the stats, memory, and CPU are reset because the tab is restarted. So to do fix this, it's obvious that Dozzle needs to have some kind of temporary data for stats and logs. On logs, I think you are already very familiar with this. Currently, notifications can't be sent. Logs can't be analyzed, etc... But to me, the stats was even more important. Now I have created On performance, it should actually be better. Before, for each user I would start listening on all containers. So that was something like N X M where N is users and M is the number of containers. Now, I only listen to the containers once but stream all this data to all users. So if more users connect, CPU shouldn't go up. Does this help? I am finding some little bugs and will try to release when I think it's stable. |
Thanks a lot for your detailed explanation. I can feel your intense efforts and engineering from this single PR.
Yeah @amir20 ! I read through the
This sounds great! I wasn't thinking this direction. This really reduces the CPU usage a lot. Once again, thanks a lot for your clear explanation :) |
Just done that and identical to yesterday evening. Go into Interestingly when starting the container as well as jumping back up in the list it also goes back to white text. |
You are testing master, right? Can you share a video. |
Yep I re-pulled master just now. Video is the same as in the post above - #2740 (comment) {"level":"info","msg":"Dozzle version v6.1.1","time":"2024-02-01T21:04:39Z"} |
Are you pulling |
I'm a total numpty @amir20 !! - I was so busy switching it from a |
Ha great. So it looks this is as an actual bug in latest version. Glad to know I fixed something I didn't know about. |
Eek - possibly spoke to soon - have found a bug the other way round now I'm afraid!! Screen recording below (and I promise this is definitely on
|
Ahhh good catch. I did not cover start or restart. :) |