MuonTrap.Daemon new features

[alde103]

In response of #30

I added the following option in MuonTrap.Daemon:

• msg_callback: accepts anonymous functions to be executed with the daemons outputs.

[thdxr]

Looks good to me - excited to have this!

[fhunleth]

@alde103 Could you trim this PR back so that it only contains the message callback function?

[alde103]

@alde103 Could you trim this PR back so that it only contains the message callback function?

Ok, Is something wrong with MuonTrap.Daemon.Server?

[fhunleth]

Combining multiple changes in one PR makes it difficult to review. I maintain muontrap in my free time, so while I appreciate the PR, a lot, it is unlikely that I'll have a block of free time long enough to work through a big code change.

[fhunleth]

I'm looking at the :telemetry library and wondering if that's a better solution.

:telemetry would remove the need for passing an option. Interested parties could register handlers and receive events. You may already be set up to capture these events.

Second, I know that you're interested in start and exit events as well. I could imagine other potentially interesting events. If we used :telemetry, those could be added with out adding new callback options.

The downside is that it's another dependency. I usually try to avoid that, but :telemetry is getting pulled in already by so many libraries that it's probably available anyway.

Curious on thoughts.

[fhunleth]

Could you separate this typo fix out into a separate PR? This fix is an easy one for me to merge right away.

[alde103]

I'm looking at the :telemetry library and wondering if that's a better solution.

:telemetry would remove the need for passing an option. Interested parties could register handlers and receive events. You may already be set up to capture these events.

Second, I know that you're interested in start and exit events as well. I could imagine other potentially interesting events. If we used :telemetry, those could be added with out adding new callback options.

The downside is that it's another dependency. I usually try to avoid that, but :telemetry is getting pulled in already by so many libraries that it's probably available anyway.

Curious on thoughts.

Interesting, to be honest, I have never used :telemetry directly, but from what I see, it is like a PubSub behavior. I think it makes sense to use the library for this PR. The only disadvantage, apart from the one you mentioned, I think that we would force the callbacks to be non-blocking (or should it be mentioned to the final user), which as far as I understand is a :telemetry requirement (described as an important note in the readme).

[fhunleth]

Regarding non-blocking callbacks, I'm pretty sure we have the same requirement here. I.e., if MuonTrap calls a function that blocks, the GenServer could be delayed from handling OS process exits and then that holds up a restart of the failed OS process.

Regarding this patch, could you describe your use case? I'm re-reading our dialog and wondering if I'm misunderstanding what you're trying to accomplish and whether there's a really easily alternative.

[alde103]

Regarding non-blocking callbacks, I'm pretty sure we have the same requirement here. I.e., if MuonTrap calls a function that blocks, the GenServer could be delayed from handling OS process exits and then that holds up a restart of the failed OS process.

Regarding this patch, could you describe your use case? I'm re-reading our dialog and wondering if I'm misunderstanding what you're trying to accomplish and whether there's a really easily alternative.

I think you are right, my first impressions of telemetry is that something handles the callbacks globally so any call to telemetry would block the library, which no longer makes sense to me, now I'm guessing that the only part that is blocked (or invoked) is the one that relates to the topic (event), which leads me to think, how would we handle an application with multiple daemons?

I think we should associate the event to the PID, but it seems that telemetry only accepts a list of atoms as events. We could also relate it to the daemon's name (MounTrap.Daemon option) but we would have to put a default (and warn the final user).

[thdxr]

I'm a fan of implementing via telemetry. The event name should be static but the payload can contain metadata about the running process (pid, cmd, args, etc)

[fhunleth]

After thinking about it the past two weeks, my opinion on adding additional callbacks (non-telemetry ones) to the daemon options is not something I want. Part of this is due to a desire to keep the Daemon module as short and simple as possible and this was starting to add a decent amount of code (especially the start/exit callback one). I think that if you want to do something custom with callbacks that the code will be less subtle if you make your own Daemon GenServer. They also invite adding additional options. For example, if I could rewind to when I first wrote this, I could have chosen an :on_output callback and made a logging callback be the default. Since Daemon breaks output up based on lines, then if you don't what that, that's another option PR. And is stdout the best way for a long running program to communicate anyway? I keep on thinking of issues. Admittedly they might be small, but the line for where to stop adding options and encourage writing a new module needs to be drawn somewhere.

As for :telemetry, I'm still learning on a different project so my opinions aren't set yet. It does seem useful to define a :telemetry API that sends events when the daemon starts and exits and on line output. I'm pretty sure that I'd be interested in monitoring these events if they existed in my own programs. Since I haven't formed strong opinions on :telemetry, I'm going to wait on adding this myself, but if someone wants to start a PR to experiment with an API, then I'd be for that.

So in summary, I'd like to push forward on getting experience with :telemetry. It seems promising. If no one sends a PR up in the next month or so, I'll start one.

I hope this makes sense and thanks for hanging through this as we figure it out together.