Is it possible to have async beforeNotify handler?

[jhulme-ut]

What are the steps to reproduce this issue?

I would like to be able to do something like:

Honeybadger.beforeNotify(async (notice) => {
  if (notice) {
    notice.context = {
      state: await dumpState(),
    };
  }

  return true;
});

So that we can call async methods to add data to the notice.

What happens?

Typescript complains because the beforeNotify handler doesn't allow Promises.

What were you expecting to happen?

I would like to be able to use async functions in beforeNotify.

Any logs, error output, etc?

n/a

What versions are you using?

Package Name: honeybadger-js
Package Version: 4.8.0

[subzero10]

Hey @jhulme-ut, thank you for reporting an issue!

Let me start by saying that we have considered promise-based beforeNotifyHandlers already in the past, so I understand where you are coming from!
At that time, we were entertaining the idea of making notify return a promise as well (#327). We decided that the fire-and-forget (i.e. keep notify a void function) is the preferred approach for most use cases. And we stopped there.
However, at a later stage we introduced an async version of notifyAsync (#682), so I think we should reconsider promise-based beforeNotifyHandlers. The tricky part is to not break existing functionality, but also keep the API easy to use.

We will consider this feature once more and come back with updates.

[KonnorRogers]

So initially I didnt think this was possible. I stared at this for what felt like hours. But im realizing it doesnt have to affect the final boolean returned. That can work as normal. Technically for this particular use-case we just need to make sure all promises have resolved before we build the payload here:

honeybadger-js/packages/core/src/client.ts

Line 197 in a5fd0e9

const payload = this.__buildPayload(notice)

So ideally I think if we introduce a Promise.allSettled on the beforeNotify functions, I think we could actually be okay.

Give me a little bit and I'll see if I can whip up a PR.

[subzero10]

But im realizing it doesnt have to affect the final boolean returned.

Hmm, not sure about this. I remember discussing with @joshuap in the past that the boolean result is an indicator on whether the notice will be sent to Honeybadger. I don't know how much of that boolean flag is used, but if we introduce promise-based beforeNotifyHandlers, we should wait before we return the boolean result.
Unless we decide that it's time to remove the boolean result entirely.

Alternatively, @KonnorRogers, it may worth looking into the notifyAsync method we introduced not so long ago.
Maybe we can have promise-based beforeNotifyHandlers used only with the notifyAsync method.
And introduce some error control, such as: throw error if we have beforeNotifyHandlers that return a promise and notify was called instead of notifyAsync.

[joshuap]

We used to use the boolean approach in the Ruby gem (I forget, it may still work that way as a fall back), but we ended up going with this approach, which might be useful here:

Honeybadger.configure do |config|
  config.before_notify do |notice|
    # Use your own error grouping
    notice.fingerprint = App.exception_fingerprint(notice)

    # Ignore notices with sensitive data
    notice.halt! if notice.error_message =~ /sensitive data/

    # Avoid using all your quota for non-production errors by allowing
    # only 10 errors to be sent per minute
    notice.halt! if !Rails.env.production? && Redis.current.incr(key = "honeybadger_errors:#{(Time.now - Time.now.sec).to_i}") > 10
    Redis.current.expire(key, 120)
  end
end

https://docs.honeybadger.io/lib/ruby/gem-reference/configuration/#changing-notice-data

[KonnorRogers]

@subzero10 So this would be specific to the return value of Honeybadger.notify()

We could check the return values of all the functions prior to calling send() so if i had an async function Promise.resolve(false) then I could still halt the execution of the send() call.

Honeybadger.beforeNotify(async () => {
  return await getBool()
})

Honeybadger.notify() // => will always return true regardless of getBool, however we could stop the execution of sending to HB.

It may be easier to visualize in the form of a PR. it should be quite minimal as well and shouldnt break any existing functionality unless someone is currently returning false in a promise.

[subzero10]

@KonnorRogers I'm not following. Consider this usage:

if (Honeybadget.notify(err)) {
 // notice will be sent to hb
}
else {
 // report to a fallback database store
}

Currently notify() returns false if at least one beforeNotifyHandler returns false.

If we change this behavior and it always returns true, the else block will never execute, even if a beforeNotifyHandler resolves to false (asynchronously).
As I said above, I don't know how much of a valid use case is this, but it is possible.

That's why I suggested we attempt to implement this using the notifyAsync version, and internally we can switch to it at all times. This way, we'd be backwards compatible.

[KonnorRogers]

@subzero10 So perhaps Im not explaining correctly. It wont always return true. Itll only return true in that case.

In this case:

Honeybadger.beforeNotify(async () => {
  return await getBool()
})

Honeybadger.beforeNotify(() => false)

Honeybadger.notify() // Returns false. Dont send to HB.

Will not return true and will not send to Honeybadger. Heres the PR:

#984

The problem with "just getting it to run with notifyAsync" is that notifyAsync hooks directly into notify logic so theyre intrinsically coupled right now. It'd be a much larger refactor to split the beforeNotifyHandlers into an async / sync version.

[subzero10]

OK, I get it now!

For existing implementations, where beforeNotifyHandler returns a boolean, behavior will remain the same.
For async beforeNotifyHandlers, notify() will return true (assuming that none of the boolean beforeNotifyHandlers returns false.

For example, a modified version of above:

Honeybadger.beforeNotify(async () => {
  return await getBool() // -> ** promise returns false **
})

Honeybadger.notify() // ** returns true, but won't send to HB. **

Are we OK with this behavior? Considering the options, it's a pretty good workaround. Though, it could be confusing. We'd want to document it clearly.