Update to modern elixir

[whossname]

I'm planning on using this library, but I saw it was about 5 years out of date. I've updated it.

• updated dependencies
• use Mix.Config deprecated, replace with import Config
• Supervisor.Spec deprecated, use child_spec/1 instead
• GenStage has changed a lot since version 0.11, GenStage modules updated to use the new design
• GenEvent deprecated, use :gen_event instead
• Improved formatting. This hasn't been applied across all files, just where I saw issues
• The SlackLoggerBackend.Logger private function handle_event/3 was broken by updates to Logger. I renamed it do_handle_event/3 to distinguish it from the public function by the same name and changed it to be more robust for future changes in Logger.
• Added a test for the SLACK_LOGGER_WEBHOOK_URL environment variable overriding config.
• Updated TravisCI to use the most recent version of Elixir and OTP (NOTE: this hasn't been tested)

I'm not sure about my fix for SlackLoggerBackend.Pool. The fix works, but I'm not sure that it should be a GenStage, it's probably meant to be a supervisor. Problem is I'm not 100% on what it does.

[whossname]

@craigp do you have any interest in updating and maintaining this library? I might be interest in taking ownership if you aren't. I need to try it out in production to see if it is useful first.

The more I think about it the more confident I am that SlackLoggerBackend.Pool should be a supervisor.

[craigp]

Thanks, this is great - I've been meaning to update a few of my projects, I'll look this over shortly!

[whossname]

I've just done a quick review of my own, it will definitely need some work before it is merged. The biggest issue is that I don't think SlackLoggerBackend.Pool should be a GenStage, it should probably be a supervisor.

Other than that:

• the hook path could be more explicit in the new test
• It's probably a mistake to set the :elixir option in the mix project
• I think splitting the deps by a blank line between the different only: sections will make them a bit easier to read

[whossname]

added another commit that solves the issues described above.

[whossname]

I've noticed a security concern while playing around with this library. I think the slack webhook should be treated as a secret, but it is stored as state in the GenServers, which means that it appears in the logs if a GenServer crashes.

[whossname]

I added two commits that change some functionality. The first overrides the Poison.Encoder.List implementation. This code was crashing when it needed to encode list of the form [head | "string"]. I added some code to handle that case. This isn't the best fix as it doesn't address the under lying cause, but it works.

The second commit adds debouncing in the producer. This means that if the same message is sent multiple times it will be sent once with a count for how many times it was sent instead of being sent for every instance. I've found that the slack logger tends to spam messages so I think this will be a useful feature. I'll look at adding unit tests for it later.

We need to address the security concern above where a secret is stored in a GenServer. @craigp any preferences on how to handle that?

The way config is handled needs to be updated as well. I'm pretty sure this isn't best practice anymore:

config SlackLoggerBackend, :levels, [:debug, :info, :warn, :error]

The above code seems to trigger warnings in code that calls this library. Instead it should be something like this:

config :slack_logger_backend, :levels, [:debug, :info, :warn, :error]

I'm also thinking about changing the events from a tuple to a map in Logger. It should make them a bit simpler to handle in upstream code, such as the Producer, Formatter and FormatHelper.

[whossname]

I have done most of the stuff mentioned in the previous post and updated the documentation. I moved the get_url function into the Pool module, so at least it isn't in any GenServer state now. I think it is still quite likely to appear in logs if the line PoolWorker.post(pid, get_url(), json) fails for any reason. I know this approach can help with the HTTPoison.post/2 call:

  defimpl Inspect, for: HTTPoison.Response do
    def inspect(response, opts) do

      response
      |> Map.put(:headers, "--removed--")
      |> Map.put(:request, "--removed--")
      |> Inspect.Any.inspect(opts)
    end
  end

Not sure of the details of how to apply this yet. I'll have time to work on it next week.

[whossname]

I've figured out a better way of handling the Poison encoding issues, so I've removed the code to override the implementation for Poison.Encoder.List. It cuts the log message after the string "\nState: " to prevent state from GenServers from being posted to slack when they crash. This is useful as sensitive data can sometimes be stored in GenServer state.

I'm still working on ways of preventing the slack webhook from appearing in logs. So far I've reduced the scope that the URL appears in to just the PoolWorker. This is an improvement, but if the HTTPoison.post/2 call fails for any reason it will still appear in the logs.

[whossname]

That last commit hides the webhook in the HTTPoison.Response in the event of an error. This doesn't cover every situation where it might appear in logs, but I think I've covered the most common and important ones.

I'll have a bit of a think and another read through the code, but this might be everything.

[whossname]

Added the "SLACK_LOGGER_DEPLOYMENT_NAME" environment variable, the value of which is included in the "Deployement" field in the Slack message. It means that if multiple deployments report to the same slack thread you will know which deployment sent the message.

[whossname]

@craigp I'm pretty sure I just need to update the documentation and I am done here. In summary I added these new features:

• Debouncing in the producer. If the same message is sent multiple times the messages will be aggregated and sent with a count.
• Added the "Deployment" field to the Slack messages. It can be set in config as config :slack_logger_backend, deployment_name: "the deployment" or by using the environment variable "SLACK_LOGGER_DEPLOYMENT_NAME"

The code was updated to modern Elixir, some tests were added and these major implementation details were changed:

• The webhook is retrieved in PoolWorker instead of Logger and it's value is hidden when inspecting the HTTPoison.Response. This is to try to protect the webhook a bit more as I believe it should be treated as a secret.
• Events are passed around as maps instead of tuples. This is to make some of the code simpler.
• Log messages are truncated at the string "\nState: " to prevent state from GenServers from being posted to slack.

This pull request ended up being much larger than originally intended. Sorry about that.

[whossname]

I've removed the implementation of inspect for HTTPoison.Response. After some research it turns out that this is not good practice. This means the web hook isn't hidden if an error contains the HTTPoison.Response. Maybe some documentation should be added to explain how to do this. In general you just need to include this code somewhere in your code:

  defimpl Inspect, for: HTTPoison.Response do
    def inspect(response, opts) do
      %{response | request_url: "--redacted--", request: "--redacted--"}
      |> Inspect.Any.inspect(opts)
    end
  end