Details for after_send_event callback are incorrect

[halostatue]

Environment

• Elixir version (elixir -v): 1.7.4
• Erlang/OTP version (erl): 21
• Sentry version (mix deps): 7.0.6
• Operating system: MacOS

Description

The description of the :after_send_event callback appears to be incorrect:

• :after_send_event - callback that is called after attempting to send an event.
  Accepts an anonymous function or a {module, function} tuple. The result of the HTTP call as well as the event will be passed as arguments. The return value of the callback is not returned.

The spec for maybe_call_after_send_event/2 is @spec maybe_call_after_send_event(send_event_result, Event.t()) :: Event.t(), and @type send_event_result :: {:ok, Task.t() | String.t() | pid()} | :error | :unsampled. So an event result of :error includes no information related to why the event failed.

Part of the reason I implemented #334 was so that I could switch Sentry logging to :debug and specifically suppress logging for Sentry rate-limit rejections but explicitly log other errors, such as that seen in #240 (we an alert in our ELK system for these errors, but these are currently swamped by spike protection 429s). I can’t do that with an :error result, but would be better served by an {:error, error} tuple result.

[mitchellhenke]

Thanks for opening this issue! That information would be very helpful and should be there. I'll take a look.

[halostatue]

The biggest blocker I see with this—and why I opened an issue rather than a PR—is that with retries, the previous error is lost meaning that only :error can be returned in that case.

[mitchellhenke]

Ah, right. Will take a bit of work, but it's information that should be available.

[mitchellhenke]

I've opened #354 to resolve this issue, but it changes the return type, so it may warrant a new major version?

[halostatue]

This looks good to me, and I agree that it’s probably worth a major bump because anyone who has implemented an after_send_event will need to update to support the new output.

[mitchellhenke]

closed in #372