Attempt to improve plug call exception handling and logging

[grzuy]

Supersedes #396.

• Telementry events

  • Also emit span for throw
  • Also emit span for exit

• Logger

  • Lets logger handlers know about the original crash reason when handling bandit errors
  • References:
    • Elixir Logger crash_reason: https://github.com/elixir-lang/elixir/blob/4bbddcc053498ca28ea5ebb5039de9b3a97ddf82/lib/logger/lib/logger.ex#L130-L135
    • Broadway passing crash_reason: https://github.com/dashbitco/broadway/blob/9eaf2140b6c1f36b25f990a7c74052bece26aadc/lib/broadway/acknowledger.ex#L104-L113
    • Plug.Cowboy passing crash_reason: https://github.com/elixir-plug/plug_cowboy/blob/746d848baca3436ca74fdab2750d14070b92002b/lib/plug/cowboy/translator.ex#L42-L46

[mtrudel]

A couple of changes requested to keep things consistent and explicit; other than those changes the rest of this looks great!

[grzuy]

Fixed format.

Can't reproduce locally the protocol_test.exs failures happening here, though.

[grzuy]

@mtrudel Just realized that Bandit was originally taking the approach of re-raising plug call errors in 1.4 instead of just "logging or silence" with changes in 1.5 with https://github.com/mtrudel/bandit/pull/342/files#diff-68626306df11d3e10964941997c6cdafbf587a9ec7de087141840655a47a1855L65.

Curious about the reason not to continue re-raising?

Similar to what plug_cowboy does...

• https://github.com/elixir-plug/plug_cowboy/blob/ced253f035f2a07ebed17cfacb64d4f16357a750/lib/plug/cowboy/handler.ex#L21-L23
• https://github.com/elixir-plug/plug_cowboy/blob/ced253f035f2a07ebed17cfacb64d4f16357a750/lib/plug/cowboy/handler.ex#L52-L63

In case there's the possibility of introducing re-raise again, this pull request could be re-worked.

[grzuy]

Reading this thread (https://groups.google.com/g/elixir-lang-core/c/pWz-uTVMEVM/m/QGXncxD1AQAJ) also got me thinking about it.

[mtrudel]

Similar to what plug_cowboy does...

Interesting! It seems like they're using exactly the 'catch :error types' convention I raised above, such that exceptions are in fact re-raising. I think when I'd read the plug_cowboy implementation originally I saw the catch and assumed that handling was only for idiomatic throw/catch usage (which the current Bandit implementation handles).

The main purpose of that PR was to ensure that logging of errors takes into consideration the accompanying result code (ie: only log errors that get raised with a 500..599 response code). This generally means that we do the logging ourselves (as we do in that PR) instead of reraising and letting the runtime log it

All of which is to say that I think we're doing the right thing by not re-raising?

[grzuy]

Mmm... not sure to he honest.

I'm exploring the "re-raising approach" in a separate branch to compare.

I'll let you know how it goes.

[grzuy]

I'm exploring the "re-raising approach" in a separate branch to compare.

Opening draft with what I got from playing with that alternative approach: https://github.com/mtrudel/bandit/pull/400.

[grzuy]

Closing in favor smaller and more focused #411 for now. Will revisit and build on top of #411 if that one accepted and merged.