search

Hexagon / pup

Universal process manager built in Deno
https://pup.56k.guru
MIT License
151 stars 5 forks source link

Feedback/ideas/thoughts #50

Open josephrocca opened 7 months ago

josephrocca commented 7 months ago

Awesome project! Thanks for your work on this. I'm in the process of attempting to switch from pm2 to pup, and I thought it might be useful to share some thoughts given that it looks like you're heading toward v1.0. Please take each of these suggestions with a grain of salt. And I'll pre-apologise here for any suggestions/thoughts that have stemmed from misunderstandings in the way Pup works.

Confusion around pup run vs pup start and pup terminate vs pup stop - and "apps" vs "ecosystems" vs "processes"

Intuitively, here's how I think pup should work:

  1. Install pup
  2. Enable it on startup with sudo pup startup enable or something
  3. Create a pup.json file in your app/server directory
  4. Run pup start in the same directory to start app (and causes pup to track that config file - e.g. for startup stuff)
  5. To stop, run pup stop in that directory (or run pup stop the-app-id), and it stops all the processes for that app/ecosystem (and untracks config file)

Controlling individual processes within an app is something that should be possible, but (at least in my experience) is not as common, so it should not use up the precious/intuitive start and stop commands. By far the most common commands I use with pm2 are start/stop/restart/logs, and they all apply to apps, not processes within an app.

I think this is the most important change that should be made for v1.0. If the current CLI design decisions were made to support more complex use cases that I'm not considering here, then I think "simple things should be simple, complex things should be possible" might be worth applying here. Need to avoid making simple things unsimple in order to make complex things simple-ish. Instead, move the burden to people doing complex things, because there are far fewer of them, and they (basically by definition) have the time + skill to go a little deeper in the config docs, or whatever.

The other huge advantage of this is that I think 90% of people will be migrating here from pm2, and IIUC this is basically how pm2 does it: https://pm2.keymetrics.io/docs/usage/application-declaration/ It's much easier to adopt new software if things work just like you expect - both intuitively, and from experience with related, popular software. Diverging from that is a cost that would need to give significant returns to be justified (Deno team realized this recently with deno install, for example). It's probably only worth diverging from pm2 in aspects where it's kinda confusing/bad.

Also perhaps worth mentioning: With the current CLI commands, I'm not actually sure how I'm supposed to use pup run, since it runs in the foreground instead of starting all the app's processes in the background like I expected. Also, when I try exiting with Ctrl+C, sometimes it hangs:

joe@test:~/app$ pup run
2024-04-11T07:15:50.617Z [LOG] [core:processes] Process 'app' loaded
2024-04-11T07:15:50.617Z [LOG] [app:starting] Process starting, reason: autostart
2024-04-11T07:15:50.651Z [LOG] [app:stdout] Listening on http://localhost:80/
2024-04-11T07:15:50.656Z [LOG] [app:stdout] HTTP server running. Access it at: http://localhost:80/
^C2024-04-11T07:16:00.685Z [LOG] [core:terminate] Termination requested
2024-04-11T07:16:00.689Z [LOG] [app:block] Process blocked, reason: terminating
2024-04-11T07:16:00.690Z [INFO] [core:cleanup] /home/joe/app/.pup.jsonc-tmp removed.
2024-04-11T07:16:00.687Z [LOG] [app:errored] Process exited with error: Error: Exited with code: 130
^C^C^C^C^C^C^C^C^C^C^C^C^C^C^C^C

and I need to close the terminal to actually stop it. Other times it exits fine. But in both cases the pup status tells me No running instance found..

Where is Pup's "ground truth" for config?

IIUC, the pm2 save command snapshots the configs and uses that saved snapshot to e.g. determine whether an app should start on machine startup. I'm wondering how pup handles this? The pm2 save command was always a little bit confusing/annoying to me. Ideally, in my mind, pup would use the actual config files in the app directories as ground truth. So pup start would just add that config file's path to pup's internal "known config files" list, which it uses at machine startup to fetch the actual config file contents, and determine whether the app should be started. And I think pup stop should remove the config file from pup's internal "known config files" list.

There are tradeoffs here, but I think I would prefer if pup held as little "internal state" as possible - so I can just e.g. edit the config file and run pup restart in the same directory, and I know that I'm editing the actual "ground truth" that Pup is using.

I think there's a temptation here to, for example, not bundle pup's untracking of a config file with stop-ing the app because that's not always what would be desirable - but I'd appeal to the "simple things should be simple" principle again here - it's easy to add some extra (more "granular") commands like pup track and pup untrack, or whatever, which can be used in cases where devs are heading off the beaten path.

Document a few of the most common/useful commands in readme

E.g. pup status, pup logs - a quick dot-point list of these in the readme as part of the "quick start" would have been handy for me to quickly get the rough idea. And maybe add a final dot point linking them to e.g. https://github.com/Hexagon/pup/blob/main/docs/src/usage/index.md for the rest of the commands.

Document configuration defaults

This page: https://pup.56k.guru/usage/configuration/#general should have the defaults for all options documented. This one is pretty important I think. It'd also be worth mentioning a few of the most important defaults in the readme I think.

Related: I love opinionated defaults that make it easy for people to get started, but I'm a little bit weary of "overly specific" defaults like watcher.exts = ["ts", "tsx", "js", "jsx", "json"] which might not be very future proof. E.g. could probably add yaml, json5, etc. to that - there's a long tail. It might be best to just watch all extensions by default, but I'm not confident what the best approach is here. Current defaults are not unreasonable, but worth thinking about in terms of forward compatibility and future breaking changes.

pup logs should stream by default

I use pm2 logs <all|app-id> on a daily basis - it's very handy to have logs streaming in my console as I'm coding, so I can e.g. see errors during startup after saving the file, or see live logs on a production server.

Can console.warn be made orange in logs?

I've wished for a long time that pm2 did this. It sounds like a small thing, but I really appreciate that Chrome's devtools does this when I'm remotely inspecting a Deno/Node process. I just wish it were possible for terminal logs to make a visual distinction between warnings and errors. That said, I'm guessing it might not be possible due to console.warn maybe just being a stderr log without any distinction from console.error at the level at which pup "sees" those logs.

A log file with errors and normal logs interspersed

Another annoying thing with pm2 is that (AFAIK) I'm not able to see a log file that has errors and logs interspersed. The reason this is annoying is because the logs that came before an error are often quite relevant to the cause of the error. So, while debugging, I end up needing to switch back and forth between them and rely on timestamps for aligning them.

Switch from JSONC to JSON5, or support both

I hadn't heard of JSONC before - maybe I'm just out of the loop on that. I have used JSON5 before though, and seems to be more commonly used?

JSON5 is nice because you don't need quotes around keys like with JS POJO declarations - makes config files really clean.

Move .pup.jsonc-tmp, .pup.jsonc-data, etc. into .pup

This reduces clutter - especially if more pup-related files are needed in the app directory in the future.

Densify pup status, add colors for skimming, and num restarts + uptime

For pup status, "number of restarts" and "uptime" would be handy, and pm2-style density with skimmable (green=good) would be awesome:

image

Rather than having a separator between rows, I mean. I also think pm2 status could be made a lot more dense - e.g. by using emojis (which work fine in terminal, of course) for boolean/enum fields like status, watch, etc. The denser the better imo.

Also, I think pm2's "cpu" column is kind of useless since IIUC it's the CPU usage at that exact instant - it'd be much more useful for me to see average or total CPU usage over the last 30 minutes, or something.

Please don't feel the need to reply to all/any of the above points. I just hope that these thoughts have been at least somewhat useful. Thanks again for all your work on this!

josephrocca commented 7 months ago

Also, minor point - in watcher default options: "match": ["**/_._"] - is this underscore syntax standard? I haven't seen that before - but it could just be due to my inexperience. I actually thought that "**/*.*" would do the same thing. If it's special / non-standard syntax then it might be worth considering sticking to standard syntax if only for the benefit of e.g. IDE code autocompletion (but also for user expectations of course). If there are reasonable use cases that the standard syntax doesn't support, then maybe picking a different standard syntax would be better than extending an existing one with custom syntax.

(But again, maybe I just haven't seen this underscore syntax before - I'm not very experienced in this stuff.)

Hexagon commented 7 months ago

A lot of thanks for your support and this detailed feedback!!

I've focused very much on "getting it to work" and never really sat down and looked at the big picture, so this is exactly what i need to get it ready for 1.0!

Lots of great points here, and I especially agree with separating the system service from the processes in the way you described as "apps" vs "ecosystems" vs "processes", I will have a look at this asap 👍

About the ground truth for config, i reckon a ~/.pup folder in the home directory could be used, with a ~/.pup/global.json which could be used to track each configuration.

I'll review your feedback in detail and make changes as i find time. Thanks again!

Hexagon commented 7 months ago

Changes connected to next release are tracked in https://github.com/Hexagon/pup/pull/51

albertosantini commented 7 months ago

Generally speaking, in my context, I have been using a lot about deploy feature. https://pm2.io/docs/runtime/guide/easy-deploy-with-ssh/

From dev machine, we can deploy the code to the remote machine. This simplifies the use of a more robust deploy pipeline.

Just my two cents. Thanks for the efforts.

Leokuma commented 6 months ago

when I try exiting with Ctrl+C, sometimes it hangs

Let me tackle this one. I'll also refactor some code.

I'm not actually sure how I'm supposed to use pup run, since it runs in the foreground instead of starting all the app's processes in the background like I expected.

I saw that Hexagon has just renamed pup run to pup foreground. I'd like to propose this: revert back to pup run, but make it run in the background by default. Only run in the foreground with argument -a, like pup run -a or pup run --attach. That's how Docker works. I find run prettier than foreground as a command name. What do we think about that? FWIW, currently I run Pup in the background with $ nohup pup run > /dev/null 2>&1 &.

Hexagon commented 6 months ago

Actually, i agree that pup run is a better option... But had to give it a go :)

pup run is back in it's original form from 1.0.0-rc.19

Hexagon commented 6 months ago

Open question; Should an instance have a name? The name could be pup by default, but could be set by passing --name to init or by being specified in pup.json through name:.

The name would be used to ...

Leokuma commented 6 months ago

LGTM.

Just a few questions/remarks/drawbacks below. Basically my only concern is that I like to use Pup without installing it as a service sometimes and without the new global.json. I'd like to still be able to do that because it's so simple, stateless, portable and unintrusive.

  1. ~/.pup/global.json will only be created/updated the moment you run pup enable-service
  2. You can't use pup --instance if you don't install the service
  3. If you change the name in pup.json after installing it as a service, the already installed service name is not changed, and pup --instance is not changed either: they keep the old name from when you ran enable-service. You have to reinstall the service in order to apply the new name
  4. Maybe call it id instead of name so that it aligns with processes' id? Not sure, though
josephrocca commented 6 months ago

I'm not sure if this is already available, or on the roadmap, but just in case: one thing I use regularly in pm2 is pm2 reload, which restarts processes in a cluster one-by-one for zero-downtime restarts.

(One slightly annoying thing is that it always restarts one-by-one, even if you've e.g. got 100 nodes in the cluster - so it can be much slower than it needs to be. I think it should be a percentage, like e.g. 10% at a time by default would make sense for my use cases, at least.

Also, I don't really like the naming reload, since I always forget whether it's restart or reload that does one-by-one. Though I haven't thought about what a better name would be. Maybe just an option on restart command.)