rejeep / prodigy.el

Manage external services from within Emacs
GNU General Public License v3.0
550 stars 39 forks source link

Custom service status #28

Closed rejeep closed 10 years ago

rejeep commented 10 years ago

See https://github.com/rejeep/prodigy.el/issues/11 and https://github.com/rejeep/prodigy.el/issues/10.

@magnars, I see your two issues above as the same issue, namely custom service status. Hope you agree with this.

Basically, a process status does not tell much about what actually happens in the process. If you start a Rails server, it takes a few seconds, but the process is considered running almost instantly after.

This issue is about finding out a good API for controlling a service status and then implement it. Please comments with your thoughts below.

rejeep commented 10 years ago

How about this API. Can you guys find any limitations?

(prodigy-define-service
  :name "Rails Project"
  :cwd "~/Code/rails-project"
  :command "bundle"
  :args '("exec" "rails" "server" "--port" "3001")
  :tags '(rails)
  ;; Called each time Prodigy needs to find out the status of the
  ;; process. If this property is specified, it is called with the
  ;; service. If return value is symbol, use that as process
  ;; status. If return nil, check process status.
  :status (lambda (service)
             ;; The :started property is nothing related to
             ;; Prodigy. Only a way to ignore the status updates if
             ;; the process is (really) started.
            (unless (plist-get service :started)
              (with-current-buffer (prodigy-buffer-name service)
                (if (s-contains? ">> Listening on 0.0.0.0:3001, CTRL+C to stop")
                    (progn (plist-put service :started t) 'running)
                  'waiting)))))
magnars commented 10 years ago

Agreed about #10 and #11.

Called each time Prodigy needs to find out the status of the process.

When is this? Can I keep the prodigy status buffer open and watch things change?

unless (plist-get service :started)

What happens when this service is restarted?

Using the entirety of with-current-buffer to determine status is OK, but has some easy traps to fall into if you're doing more than one status. You have to check from (point-max) and backwards. It'll also be slow in large buffers.

Maybe the lambda could take a second argument containing new lines of output since the last call?

magnars commented 10 years ago

How about this:

Prodigy sets the status of a service when it starts and stop, like today. In addition you can use on-output from #3 to set the status yourself.

rejeep commented 10 years ago

I like the simplicity! A problem with that approach is that the service will be "running" for a short while (until the first output).

magnars commented 10 years ago

Could be solved with an on-start hook, maybe?

magnars commented 10 years ago

Or I would be entirely happy to have "Running" as a default status, and switch it with "Ready" or something when it's actually ready.

Especially if I could set up my own colors for different statuses. :)

rejeep commented 10 years ago

FYI regarding hooks: https://github.com/rejeep/prodigy.el/issues/25

Or I would be entirely happy to have "Running" as a default status, and switch it with "Ready" or something when it's actually ready.

Yeah, but I think that is somewhat of a hack. I would love for this feature to become a natural and simple part of Prodigy.

Especially if I could set up my own colors for different statuses. :)

There are a lot of statuses today. Perhaps it would be better to only have a few Prodigy statuses, each with it's own color. I've only seen my processes in a "running" of "exit".

magnars commented 10 years ago

Yeah, but I think that is somewhat of a hack. I would love for this feature to become a natural and simple part of Prodigy.

Really?

:on-output (lambda (service output)
             (when (s-contains? "Ready on port 3000" output)
               (prodigy-set-status service "Ready")))

This looks really nice to me. If it was configuration, it would be slightly shorter, but no clearer.

rejeep commented 10 years ago

This looks really nice to me. If it was configuration, it would be slightly shorter, but no clearer.

The API is fine, but in Prodigy it will look like this:

0s: Running
1s: Running
2s. Running
3s. Ready

Or if you set the status to "starting" in a before start hook:

0s: Starting
1s: Running
2s. Running
3s. Ready

Another idea is that if the status has been set manually once, Prodigy will not do it automatically. Perhaps it will start doing it automatically again if you set it to nil. That way you would set it to "starting" in the :before-start hook and to "running" if the output matches something:

:before-start (lambda (service)
                (prodigy-set-status service "Starting"))
:on-output (lambda (service output)
             (when (s-contains? "Ready on port 3000" output)
               (prodigy-set-status service "Ready")))

We could also add a :status hook so that you get information when the process state changes. For example if the process crashes, it allows you to set the status to nil or whatever.

magnars commented 10 years ago

This is exactly what I was expecting:

0s: Running
1s: Running
2s. Running
3s. Ready

But what is going on here?

0s: Starting
1s: Running
2s. Running
3s. Ready

Does this mean prodigy is polling and setting the status every second, and not hook based?

Wouldn't that also mean the first example would be

0s: Running
1s: Running
2s. Running
3s. Ready
4s. Running

?

In that case, I would think this is best:

1) Set status to "Running" when starting. 2) Poll every second, but only set status if the process has died.

rejeep commented 10 years ago

Those examples are based on what the value of (prodigy-status-name) would be. The status in the view updates only when repainted, so this might (not might, depending on if you move the cursor for example) not be what you see.

So for you "running" does not say that the process is actually running. If you think there should be a difference between "running" and "ready", I guess you want "running" to be yellow and "ready" to be green? But that would only be the case for services where you manually set it to "running". If you don't, you would want "running" to be green and not red.

Communication is hard... :)

How about we start over and first get a list of the possible states Prodigy should allow? Just to make sure we agree on them. How about:

These states would still have the color issue!

magnars commented 10 years ago

The status in the view updates only when repainted

Why doesn't prodigy status buffer repaint when I call (prodigy-set-status service "Ready")?

How about we start over and first get a list of the possible states Prodigy should allow?

I guess we're thinking quite differently about this. :-) So yeah, communicating is hard.

In my mind, the status is a UI element in the status buffer. And I want to control it.

But reading what you're saying, then a process' status is something prodigy cares deeply about.

Maybe there's a difference between what prodigy cares about (started/stopped) - and what I care about (all kinds of weird stuff probably)?

If it's not, then we're bound to start worrying about all kinds of statuses that Prodigy should support, and what color all of them should have. Which in my mind should be up to the user.

rejeep commented 10 years ago

Why doesn't prodigy status buffer repaint when I call (prodigy-set-status service "Ready")?

It will, when it exists! :)

Prodigy has to care about status because for most services, people will not create their own custom statuses, like you, and me :) But even you and me probably wont do it for all services and then Prodigy has to manage it. That's why I think it makes sense that Prodigy supports a few different statuses and the user can manually override and set these statuses.

Then I also suggested a custom status above, but then the user would have to control it and set its color.

magnars commented 10 years ago

Aha, so you want to bring the distinction "Starting" and "Running" to the people. That makes sense. Here's a suggestion:

Prodigy works like it does today with regards to statuses, ie. you have Running and Exit. But there's have a shorthand:

:ready-regexp "Ready on port 3000"

that gets expanded to:

:on-start (lambda (service restart?)
            (prodigy-set-status service (if restart? "Restarting" "Starting")))
:on-output (lambda (service output)
             (when (s-contains? "Ready on port 3000" output)
               (prodigy-set-status service "Ready")))

So if you just want this, then use the shorthand. If you want more, then there's a place for you to go too. Like the difference between plumbing and porcelain in git.

Thoughts?

rejeep commented 10 years ago

Aha, so you want to bring the distinction "Starting" and "Running" to the people.

Wasn't that what you wanted from the beginning? :)

Thoughts on your suggestions:

  1. There only has to be a ready-regexp and no other?
  2. Add function prodigy-set-status.
  3. Add support for hooks (see https://github.com/rejeep/prodigy.el/issues/25).
  4. Add :on-output and :ready-regexp properties.
  5. What statuses should be supported by Prodigy?
  6. What about custom status?
  7. If we don't add a ready-regexp or set status to "ready", how then will Prodigy know when the service is ready?
magnars commented 10 years ago

Wasn't that what you wanted from the beginning? :)

Yes. But I would be totally happy with setting the status myself in a hook.

  1. There only has to be a ready-regexp and no other?

If the goal is for Prodigy to support the ready status.

  1. What statuses should be supported by Prodigy?

In my mind, all statuses are equal. Just information to the end user. Just strings. With a color. Prodigy only needs to keep track of a running? flag. But of course, this is in my mind. You know better.

  1. What about custom status?

In this case, all statuses are custom. Okay, a few are special in that Prodigy itself sets them. But they're still just strings with a color attached.

  1. If we don't add a ready-regexp or set status to "ready", how then will Prodigy know when the service is ready?

It can't.

Why would it need to?

rejeep commented 10 years ago

In my mind, all statuses are equal.

They are not in my mind and I think it's best we work this out before implementing this so that as many users as possible are happy.

I see it this way. A process in Emacs can have any of the states: run, stop, exit, signal, open, closed, connect, failed, listen or nil.

As you've stated (and the reason for these issues). You don't think of your service (not process) status like this. Me neither.

Just like a process can be in any of the above states, I think like this of a service. Let's say for example that a service can be in any of the following states: stopped, starting/running, ready and custom. Prodigy handles all but the custom status by looking at the process status.

In my mind, Prodigy should handle service status by default by looking at the process status. But, the point of these issues is that Prodigy does not always do a very good job, so the user can customize what status the service has.

Most services will probably do fine with the statuses stopped, starting/running and ready. The idea with the custom status is that some services may be more complex. As a silly example, let's say a service controls water temperature. Then three custom statuses are added: ice, liquid and gas.

The big issue with this now as I see it is not the API. I think that will work out just fine. The issue that remains now, is how to mix the default status behavior that Prodigy does automatically and the manual, done by the user. How do we do that in a clear way to the user and in a way that these two don't fight with each other?

Do you agree on this or do you have a different view?

magnars commented 10 years ago

I like the distinction between process status and service status.

I don't think custom should be a status, tho. What does that even mean? It would lose all semantic for the Prodigy program. No, I think Prodigy should keep track of its own status for internal use. Maybe this is the process status? And then have a "public status" that can be anything. User customizable.

This way the statuses that the system operates on is well known. And the user is free to create their weird ice, liquid and gas statuses. Without them interfering with the operation of Prodigy.

Some changes in process status will be used to set the service status. But only when they change - along with a hook if you want to customize it.

magnars commented 10 years ago

But let me again state: You know this system. I only know what I want - the ability to set the status based on hooks. And not have that ruin how Prodigy works. :-)

rejeep commented 10 years ago

I don't think custom should be a status, tho. What does that even mean?

I probably was a bit unclear here. Custom is not a status. Think about it in this way:

;; prodigy.el

(prodigy-define-status 'stopped)
(prodigy-define-status 'running)
(prodigy-define-status 'ready)

;; user-specific-code.el

(prodigy-define-status 'ice)
(prodigy-define-status 'liquid)
(prodigy-define-status 'gas)

Prodigy has three by default that Prodigy can set internally based on the process status. These will probably be sufficient in most cases, but if not, just add your own. But then you would also have to set the status yourself using prodigy-set-status.

But let me again state: You know this system.

Hopefully, yes! :) I can implement a basic version of this so that we can try it out. Then we can continue the discussion from there?

magnars commented 10 years ago

just add your own

Okay, so what you're saying is that Prodigy does not use this status for anything?

I can implement a basic version of this so that we can try it out. Then we can continue the discussion from there?

Sounds like a plan.

rejeep commented 10 years ago

Okay, so what you're saying is that Prodigy does not use this status for anything?

Nope, only the built in. If you set your custom status on a service, Prodigy will update and use that status, but nothing more. I'm also thinking prodigy-define-status should take face and name as arguments.

magnars commented 10 years ago

Nope, only the built in. If you set your custom status on a service, Prodigy will update and use that status, but nothing more.

Communication, so hard. :) What I mean is: The Prodigy code itself does not care what a service's status is. Other than purely for display purposes.

I'm also thinking prodigy-define-status should take face and name as arguments.

Excellent.

rejeep commented 10 years ago

Communication, so hard. :)

No kidding. Weird that open source works so well :)

The Prodigy code itself does not care what a service's status is. Other than purely for display purposes.

Well, if the user does not set a status manually, Prodigy has to do it. So Prodigy will look at the process status and determine what status it should use.

magnars commented 10 years ago

Well, if the user does not set a status manually, Prodigy has to do it. So Prodigy will look at the process status and determine what status is suiting.

Then Prodigy will stop updating the status once I set it manually? I have to monitor the process myself for an Exit status for instance?

This is why I talked about reacting to changes, instead of "polling" the process status. :)

rejeep commented 10 years ago

Then Prodigy will stop updating the status once I set it manually? I have to monitor the process myself for an Exit status for instance?

Not sure what is best in this case. If you only use any build in status, the best would probably be for Prodigy to change the status when the process stops. But if the service has a custom status, then the desired behavior is probably to do nothing.

This is a tricky one and I'm not sure...?

This is why I talked about reacting to changes, instead of "polling" the process status. :)

What kind of changes are you talking about?

magnars commented 10 years ago

I guess what I'm saying is that there should only be one system for status. If I understand you correctly, you're talking about two different ways of finding the status. Either it's "set in stone" by some user code, or it is dynamically figured out from the process status.

Then we lose the info from the process the moment we set our own status.

Instead, the status could always be set on the service in response to some event. Like the process being started, or restarted, or killed. Or by some user defined hook. That way I can add custom statuses - and still Prodigy will change it to Exit once it exits.

rejeep commented 10 years ago

Maybe that makes sense. I mean, how often does a process change status? It basically only starts and dies.

So then I think we should be on the same page.

That should basically be it, right?

magnars commented 10 years ago

That should basically be it, right?

That sounds right to me. :)

rejeep commented 10 years ago

So 30 comments later, two probably highly educated engineers have agreed on three bullet points? :)

Keep it simple FTW!

magnars commented 10 years ago

I think we've been talking about slightly different issues this entire time. It's amazing how that works. :)

rejeep commented 10 years ago

Yes, but I'm quite happy with what we ended up with. As I said, will start cooking on this and we can discuss more later. :)

magnars commented 10 years ago

Cool :-)

magnars commented 10 years ago

Five hours ago:

How about this:

Prodigy sets the status of a service when it starts and stop, like today. In addition you can use on-output from #3 to set the status yourself.

:-)

rejeep commented 10 years ago

Indeed. Oh well... :)

rejeep commented 10 years ago

@magnars Check out https://github.com/rejeep/prodigy.el/commit/0d2530c18556ed4d0cd67ed85c27b36544c6df9b. This is just some basic work. It works so far I can tell. I'm going to continue working on this, but I figured you could try it out if you want. I will force push that commit later, so you know!

Btw, this feature is awesome with tag inheritance, for example:

(prodigy-define-tag
  :name 'rails
  :on-output (lambda (service output)
               (when (or (s-matches? "Listening on 0\.0\.0\.0:[0-9]+, CTRL\\+C to stop" output)
                         (s-matches? "Ctrl-C to shutdown server" output))
                 (prodigy-set-status service 'ready))))

I'm also thinking about adding something like this:

:status '(("Started at port: [0-9]+" ready)
          ("Already in use: [0-9]+" failed))

But that's for the next step.

magnars commented 10 years ago

Haha, that prodigy-define-tag is brilliant. :-D

rejeep commented 10 years ago

@magnars Cleaned it up a bit, can you try it out and see if it works for you!?

magnars commented 10 years ago

Will do this evening. :)

magnars commented 10 years ago

Just tried it out a little now. Works great! :)

Any way I can disable the current line thing? I'm using (global-hl-line-mode 1) and the current line is sort of flakey. Maybe you could do just a local hl-line-mode instead, come to think of it?

magnars commented 10 years ago

Good job on this one. I noticed you created a new open source project just to be able to test it too. Outrageously awesome. :)

rejeep commented 10 years ago

Just tried it out a little now. Works great! :)

Nice, will release 0.3 in a few days. One or two features I want to include.

Maybe you could do just a local hl-line-mode instead, come to think of it?

Good idea, will do!

rejeep commented 10 years ago

FYI https://github.com/rejeep/prodigy.el/commit/d20e50413b8b56d67f0253ff4af2260f35a760c3