Plugin loading limitation

[shalvah]

Our current initialization flow has a limitation. Running ``require 'honeybadger'` loads the agent and plugins. This means that:plugins that check the config at startup are broken:

• plugins that check a config value in the requirement section (eg requirement { config[:'plugin.enabled'] }) are never loaded.
• plugins that use a value from the config will use the wrong value

In our Rails integration, we get around this by loading plugins only after the initializer has run.

https://github.com/honeybadger-io/honeybadger-ruby/blob/348ac048d33f82a92df170548b0b4173d2b006f5/lib/honeybadger/init/rails.rb#L30-L32

However, for non-Rails cases (and for Rails apps which break with convention), this is broken. Test case:

require 'honeybadger'
Honeybadger.configure do |config|
  config.breadcrumbs.enabled = false # Disable breadcrumbs
end
p ::Logger.ancestors # THe Honeybadger breadcrumbs wrapper still gets prepended

Output:

[Honeybadger::Breadcrumbs::LogWrapper, Logger, Logger::Severity, Object, PP::ObjectMixin, JSON::Ext::Generator::GeneratorMethods::Object, Kernel, BasicObject]

A workaround for this is to automatically reload plugins after config is changed. Unfortunately, there are still two problems:

1. A plugin being loaded multiple times may lead to undesired side effects. To fix this, plugins should be idempotent. We can achieve this relatively easily.
2. The specific case above is still unsolved, since breadcrumbs are enabled by default, and once you've loaded the plugin, you can't remove it.

A third problem is that this is not just about config, but also loaded gems. For instance, the Sidekiq plugin checks if defined?(::Sidekiq); if the sidekiq gem has not been loaded before Honeybadger is, it will not work.

Proposed solution

Add two hooks to plugins: one which checks if the plugin has already been loaded, and one which attempts to unload an already loaded plugin. The first hook will allow us avoid side effects, and the second will allow us remove plugins that are disabled by later config. Here's how it might look for the Sidekiq plugin:

module Sidekiq
  Plugin.register :sidekiq do
    loaded { Sidekiq.server.middleware.include?(HoneybadgerMiddleware) }

    requirement { defined?(::Sidekiq) }

    unload do
      Sidekiq.server.middleware.remove(HoneybadgerMiddleware)
    end

    execution do
      Sidekiq.server.middleware << HoneybadgerMiddleware
    end
end

For clarity, I'd prefer to rename/alias the existing methods:

module Sidekiq
  Plugin.register :sidekiq do
    loaded_if { Sidekiq.server.middleware.include?(HoneybadgerMiddleware) }

    should_load_if { defined?(::Sidekiq) }

    unload_with do
      Sidekiq.server.middleware.remove(HoneybadgerMiddleware)
    end

    load_with do
      Sidekiq.server.middleware << HoneybadgerMiddleware
    end
end

The names are a bit wordy, but clearer (and aliased to the old names to keep BC). What do you think @joshuap ?

[joshuap]

I am fine with renaming the methods in the plugin DSL to be clearer in theory, but I think it should be a separate consideration, and we'd want to consider if any 3rd parties are depending on the current method names.

As for the load/unload strategy, is every plugin capable of being cleanly unloaded? For instance, the ones that do monkey patching and/or prepend modules?

Btw, not sure if it's helpful, but we also have a plain ruby mode which is another way to get around this issue (but you have to manually call Honeybadger.load_plugins!).

cc @stympy

[stympy]

I am fine with renaming the methods in the plugin DSL to be clearer in theory, but I think it should be a separate consideration, and we'd want to consider if any 3rd parties are depending on the current method names.

I agree that it should be a separate change, if we do it, but I don't see much value in renaming the methods.

As for the load/unload strategy, is every plugin capable of being cleanly unloaded? For instance, the ones that do monkey patching and/or prepend modules?

This would be good to know... If the answer is no, then it seems the rest of this is moot.

Btw, not sure if it's helpful, but we also have a plain ruby mode which is another way to get around this issue (but you have to manually call Honeybadger.load_plugins!).

I didn't know this existed. :) It feels like this accomplishes the goal without having to make any code changes. :)

[shalvah]

As for the load/unload strategy, is every plugin capable of being cleanly unloaded? For instance, the ones that do monkey patching and/or prepend modules?

I think, if we care about it, then yes. You can't remove a prepended module, but you can at least make its behaviour dynamic.

we also have a plain ruby mode which is another way to get around this issue

Yeah, but I think that moves a lot of the work to the user.

---

We could ignore all this for now, but the main blocker is the proposed logging feature outside of Rails apps. Since it's disabled by default, the user must enable it before plugins are loaded. But we can also go with one of these workarounds:

• support an env variable for this (eg HONEYBADGER_SEMANTIC_LOGGER_APPENDER).
• always load the plugin, and check the config setting dynamically on every usage.

[stympy]

Closing this since we don't need it for the logging bit right now and there's a workaround available.