Make Telephone.Logger more resilient

[brentonannan]

This PR makes Telephonist.Logger.start_link start a GenServer which will add Logger to Event.

The GenServer links to Event, so that if it dies, the GenServer will be restarted by the application supervisor, and re-add the handler.

Not really a must-have, but it does more than the current GenEvent in Logger, and I just implemented this for my project, so I figured I'd share.

[danielberkompas]

Interesting idea! It might be better/easier though to just update the Telephonist.Event.start_link function to add known handlers, like so:

def start_link do
  manager = GenEvent.start_link(name: __MODULE__)
  GenEvent.add_handler(__MODULE__, Telephonist.Logger, [])
  manager
end

[brentonannan]

Yeah, but they're not the ones receiving the events from Event.notify; i.e. Logger needs to be added to Event; I'm not really clear on what the GenEvent within Logger.start_link really do.

[danielberkompas]

Right, I'm not talking about Logger.start_link, I'm talking about Event.start_link. I'll create a PR to show what I'm thinking.

[danielberkompas]

Actually, I just tested the current code in :observer, and the fault tolerance appears to be fine as it currently is. I can't permanently kill either Logger or Event.

This is because both Logger and Event are supervised by the Telephonist supervisor, so if one dies, it already automatically gets restarted.

https://github.com/danielberkompas/telephonist/blob/bc510a2c039f15caceef5724a1853643a234cd7b/lib/telephonist.ex#L118-L119

In your code, I'd recommend that you just imitate how Logger works, and add your subscribers to your supervision tree directly without a wrapper GenServer module. GenEvent handlers can be supervised just as well as GenServers can.

If this is all unclear still, you could hit the GenEvent docs or watch my videos on GenServer and GenEvent over on LearnElixir.tv.

[brentonannan]

Only problem with that, is that if you just kill the Event manager, it restarts, and Logger won't be added to it again unless the Logger manager is restarted. I don't see any reason why adding the handler in the Event.start_link call wouldn't work, but then you're coupling the two together.

The reason I went with a GenServer to replace the GenEvent is because you can do whatever you like in a GenServer.init (like link the process to Event, and add the handler), but in order to do the same thing with a GenEvent, you'd have to write another handler, and attach it to the GenEvent in Logger, then it's turtles all the way down.

PS: Hadn't seen your site before. Would be good if you made a more technical episode than the introduction available, but I'll watch that anyways =D

[danielberkompas]

@brentonannan Ah, I see your point. For the purposes of this library, I don't really see a problem with coupling Telephonist.Event and Telephonist.Logger together.

Have you seen Telephonist.Event crash in production? Given how tiny it is, it seems a little unlikely that it would crash.

[brentonannan]

Yeah, not a problem with coupling them, although it might be good to change the note about immitating the start_link in the docs (I think it was on the README).

Nah, haven't seen it crash in prod, but if I wrote a buggy handler and added it to it, causing it to crash, then I'd be left without even logs (not sure if Logger does > debug level logs, but either way, you'd then have a manager with no handlers - as it stands now, anyways).

[danielberkompas]

A handler can't cause a GenEvent manager process to crash. The handler is run in its own separate process. The handler would crash, not the manager. So, if Logger crashed, Event would be fine.

[brentonannan]

Ah, well, that's tops, thanks for filling me in on that =)