Honeybadger.logger (semantic logger)

[shalvah]

First pass at implementing Honeybadger.logger with Semantic Logger. Notes:

1. This won't work reliably until the bug with plugin loading is fixed. (Plugin loading limitation #461 )

2. With this approach, Honeybadger.logger is always available, and will send logs directly to Honeybadger. It does not need to be enabled via any config. (This also means semantic_logger becomes a dependency).

3. With the config setting semantic_logger.enabled, we will auto-add the HB appender to the user's SemanticLogger config, allowing the user to log via their app logger, or via Semantic Logger directly.

4. For Rails, the user has to install rails_semantic_logger if they want the auto-add feature. I'm skeptical of implementing the ActiveSupport notifications subscription ourselves because I feel it would be re-implementing what SL already does, which is (by the maintainer's admission) a hack. (I even had to tweak some of my local Rails to get SL to work properly.) I think it's better the user rely on the hack with more experience.

   We could handle the subscriptions part fairly easily, but we'd also have to replace the Rails logger, which is difficult to do consistently without introducing some unexpected behaviour for users. For instance, simply reassigning Rails.logger would mean an event would get logged twice, once by the default Rails subscriber in a non-semantic format, and once by ours. And we'd have to maintain an stdout or stderr output for the user as well, and provide a way for them to specify the desired format, etc.

[stympy]

I've played with this a bit, and here are some changes I want to make:

1. The destination URL will now include a password, so it will look like https://username:password@host:port/v1/events
2. Logs should be sent asynchronously and in batches as newline-delimited json. The queue should be flushed on shut-down.
3. At least for now, I don't want to interact with any pre-existing SemanticLogger config. We'll just provide our own logger, and anything to be reported to our API will have to be sent via that logger.
4. If a Rails environment is detected and logging is configured, subscribe to the process_action.action_controller event, with something like this:

if (url = ENV["HONEYBADGER_LOGGING_URL"]).present?
  Honeybadger::Logger.create(url: url)
  ActiveSupport::Notifications.subscribe("process_action.action_controller") do |name, started, finished, unique_id, payload|
    Honeybadger::Logger.log(payload: payload.slice(:controller, :action, :params, :format, :method, :path, :status, :view_runtime, :db_runtime).merge({event: name}))
  end
end

[stympy]

#462 (comment)

[shalvah]

Got it.

[shalvah]

Updated according to your requirements (the username and password being in the URL is effectively the same as the Basic Auth, which is already in place). Some UX thoughts:

1. I named the Rails automatic feature auto_logger. Couldn't think of a better name for now.
2. The configuration settings are in the logger. namespace, but I worry users might confuse them with the logging.* settings, for configuring Honeybadger's internal logs.
3. Minor: I switched to your example API of Honeybadger::Logger, though I think Honeybadger.logger may be more intuitive. Required fully namespacing some of our references to the global Logger (to ::Logger).

[stympy]

3. Minor: I switched to your example API of Honeybadger::Logger, though I think Honeybadger.logger may be more intuitive. Required fully namespacing some of our references to the global Logger (to ::Logger).

I agree that Honeybadger.logger is better :)

[stympy]

This is looking good to me, thought I want to leave this on a branch for now. We might want to change the namespace, as you noted.

[shalvah]

Cool, I've added Honeybadger.logger as an accessor for that class, which we may or may not rename later 😀 . Also added a retry queue, so logs which fail to be sent are kept in memory and retried when some other logs succeed.

(Btw, implementation note: since we're using the existing Server backend, the logs are sent compressed with Zlib, same as errors.)

Also, I realised that the logger.* settings don't work with Honeybadger.configure, because the config object passed to the block already has a logger field, so I moved them into a features.logger.* namespace instead.

[stympy]

Correct me if I'm wrong, but since the logging URL will likely be at a different domain — that is, it won't be api.honeybadger.io — wouldn't it be better to not use the backend's notify method to send the payload?

[shalvah]

that is, it won't be api.honeybadger.io

Ah, I didn't know this. In that case, maybe I'll create a new Backend. What I was aiming for was minimizing the API surface by reusing the existing integration as much as possible. Switched to wrapping the Semantic Logger HTTP appender

[stympy]

I'm sorry, I should have been clear about that earlier. This is one reason why I suggested using SemanticLogger, which has a HTTPS transport.

[stympy]

FYI, I might be changing my mind on the endpoint... it may be api.honeybadger.io/v1/events after all. :)

[joshuap]

Closing this because we decided to go a different direction. We'll implement the logger API after #512.