Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Creating another agent for seperate project stuff is difficult and cumbersome #231

Open
Senjai opened this issue Jul 5, 2017 · 6 comments

Comments

@Senjai
Copy link

Senjai commented Jul 5, 2017

Here's an example on how I've accomplished this for our rails project. (sanitized some NDA stuff out)

Problem 1 - Configuration

module ProjectNamespace
  module Netsuite
    Honeybadger = ::Honeybadger::Agent.new
  end
end

This is the reporter I will be using for our Netsuite interactions. The documentation tells me I should then configure it. Easily done, here's what is done in config/initializers/netsuite.rb (after NS config)

ProjectNamespace::Netsuite::Honeybadger.configure do |config|   
  config.api_key = Figaro.env.honeybadger_erp_key       
end                                                     

Ideally this would be it. But this gives me a notifier without the rails dressing like Honeybadger does. I get it, but in order to find out how to replace it I had to clone down the project and dig into the railtie to see what it was doing differently. This is as of yet undocumented (possibly unsupported). I ended up with this:

ProjectNamespace::Netsuite::Honeybadger.init!({
  root:      ::Rails.root.to_s,
  env:       ::Rails.env,
  logger:    ::Honeybadger::Logging::FormattedLogger.new(::Rails.logger),
  framework: :rails
})

ProjectNamespace::Netsuite::Honeybadger.configure do |config|
  config.api_key = Figaro.env.honeybadger_erp_key
end

Done. This gives me the exact same behavior as the global agent. I ignore the yaml configuration here on purpose since I don't need it.

Problem 2 - Usage

Now, when I use my error reporter, I have something that looks like the following pseudocode:

def do_things
  attempt_thing
rescue StandardError => e
  ProjectNamespace::Netsuite::Honeybadger.notify(e)
  raise e
end

We re raise because we want the exception to actually occur here. The problem is that the re raised exception then gets caught by the global honeybadger middleware and is reported to the main project.

This means I now have to add context or wrap my notify calls to include something special so I can tell the main honeybadger via ::Honeybadger.exception_filters to ignore the ones that are handled by the Netsuite exception reporter.

This is a poor experience :(

Suggestion

def do_things
  ::Honeybadger.with_agent(ProjectNamespace::Netsuite::Honeybadger) do
    attempt_thing
  end
end

I think sentry exposes something similar to this via Raven.capture do ... end Is this possible at all? Ideally honeybadger would use the appropriate agent to capture any exceptions raised within the block.

@joshuap
Copy link
Member

joshuap commented Jul 17, 2017

Hey @Senjai, thanks for the feedback/suggestions here. Just wanted to let you know that I'm in the process of evaluating this and will get back to you.

@Senjai
Copy link
Author

Senjai commented Jul 17, 2017

@joshuap Thanks!

In case you're curious, this is largely what we've settled on, if you wanted to see what it's kind of like to have a seperate agent coexist with a global one.

module ProjectNamespace
  module Netsuite
    class Honeybadger < SimpleDelegator
      class NSCaughtException < StandardError
        def initialize(*data)
          @exception_data = data
        end

        def message
          "NS Exception caught and reported: #{@exception_data}"
        end
      end

      def self.agent
        return @agent if @agent

        @agent = ::Honeybadger::Agent.new
        @agent.init!(
          root:      ::Rails.root.to_s,
          env:       ::Rails.env,
          logger:    ::Honeybadger::Logging::FormattedLogger.new(::Rails.logger),
          framework: :rails
        )

        @agent.configure do |config|
          config.api_key = Figaro.env.honeybadger_erp_key
        end

        @agent.exception_filter do |notice|
          notice[:exception].class == ProjectNamespace::Netsuite::Honeybadger::NSCaughtException
        end

        @agent
      end

      def initialize
        @agent = self.class.agent
        super(@agent)
      end

      def notify(exception, opts = {})
        opts = opts.dup

        if exception.respond_to?(:honeybadger_context)
          opts[:context] ||= {}
          opts[:context].merge!(exception.honeybadger_context)
        end

        super(exception, opts)
      end

      # See https://github.com/honeybadger-io/honeybadger-ruby/issues/231
      def notify_with_reraise!(exception, opts = {})
        notify(exception, opts)

        raise NSCaughtException.new(exception, opts)
      end
    end
  end
end

@joshuap
Copy link
Member

joshuap commented Oct 10, 2017

An update on this:

I have played with adding something like Honeybadger.with_agent(other_agent) (which swaps out the global agent instance during its block's execution). The issue I've run into is that it doesn't always work as expected; for instance, if you use it in a Rails controller in the around_action callback and an exception is raised in the controller, the exception will be reported to the default agent because the Honeybadger.with_agent block will have already returned by the time the Rack middleware calls Honeybadger.notify.

We could add something like:

Honeybadger.capture(agent) do
  # Any exceptions here will be reported to `agent`
end

That's different, though, because you'd still need to handle the exception afterwards so that it isn't reported to the default agent later in the stack. So I don't think that's an ideal solution to multiple agents, either.

Another option could be to store the alternate agent in the thread local Context store that we use for the context hash and rack environment. Not quite sure how that would work.

I don't love any of these ideas, but I'm hoping that thinking about them will help the right interface evolve. :)

@michaelherold
Copy link
Contributor

I'm also interested in this as part of what I'm trying to do in #246.

Since it seems like the API key is really what matters for swapping agents, would it make sense to make it so you can override the API key within Honeybadger.context or Honeybadger.notify? For my use case, I could get by with just switching the API key instead of the whole agent.

Would that give you enough of a handle for your use case, @Senjai?

@joshuap
Copy link
Member

joshuap commented Nov 6, 2017

@michaelherold we actually do support api_key as an option to Honeybadger.notify, so you can do: Honeybadger.notify(err, api_key: 'alternate').

@michaelherold
Copy link
Contributor

Oo, I did not know that - I hadn't got that far into a code dive. Good to know, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants