fix: throwing plug properly handled and returns 500 #411
Conversation
grzuy
changed the title
grzuy
force-pushed
the
throwing-plug
branch
2 times, most recently
from
aa59606 to
e58aeb7
Compare
lib/bandit/telemetry.ex
Outdated
| * `kind`: The kind of unexpected condition, typically `:exit` | ||
| * `exception`: The exception which caused this unexpected termination | ||
| * `kind`: The kind of unexpected condition, typically `:error` or `:throw` | ||
| * `reason`: The error reason. An Exception struct or the thrown value |
There was a problem hiding this comment.
The rest of this PR looks great; these are the only two items I have issue with.
For better or worse, we've already baked in exception and exit in metadata, and downstream packages already depend on them. We can't change them without a major version bump and that's not happening for this.
I'd suggest:
- Changing
:errorback to:exitandreasonback toexception - Keeping
:throwas a second valid value forkind - Adding a new
:throw_valuekey to pass the thrown value if it's a throw
This is enough to be backwards compatible and still have sensible semantic here I think.
There was a problem hiding this comment.
Thanks for the feedback!
Sounds fair.
Related to this previous discussion, for what is worth, #398 (comment).
Will try adapt to something backwards-compatible.
There was a problem hiding this comment.
After digging into opentelemetry_bandit I realized it might be a problem to start emitting events of kind: :throw right now.
opentelemetry_bandit (actually opentelemetry_api) is expecting only exceptions structs: https://github.com/open-telemetry/opentelemetry-erlang/blob/a506d08e8439c353bbf8277a1ca619c668d125f8/apps/opentelemetry_api/lib/open_telemetry/span.ex#L138-L144.
The story is different for cowboy because it's using otel_span.erl, which actually handles gracefully different kinds (referred to as Class in there): https://github.com/open-telemetry/opentelemetry-erlang/blob/a506d08e8439c353bbf8277a1ca619c668d125f8/apps/opentelemetry_api/src/otel_span.erl#L239-L242.
Rolling back for now, to only emitting the same events as before this PR and later can tackle emitting new throw events in tandem with possible updates in opentelemetry_bandit.
There was a problem hiding this comment.
Rolling back for now, to only emitting the same events as before this PR and later can tackle emitting new throw events in tandem with possible updates in
opentelemetry_bandit.
There was a problem hiding this comment.
After digging into
opentelemetry_banditI realized it might be a problem to start emitting events ofkind: :throwright now.
opentelemetry_bandit(actuallyopentelemetry_api) is expecting only exceptions structs: https://github.com/open-telemetry/opentelemetry-erlang/blob/a506d08e8439c353bbf8277a1ca619c668d125f8/apps/opentelemetry_api/lib/open_telemetry/span.ex#L138-L144.The story is different for cowboy because it's using
otel_span.erl, which actually handles gracefully different kinds (referred to asClassin there): https://github.com/open-telemetry/opentelemetry-erlang/blob/a506d08e8439c353bbf8277a1ca619c668d125f8/apps/opentelemetry_api/src/otel_span.erl#L239-L242.Rolling back for now, to only emitting the same events as before this PR and later can tackle emitting new throw events in tandem with possible updates in
opentelemetry_bandit.
For the record: open-telemetry/opentelemetry-erlang#798.
There was a problem hiding this comment.
For the record, this PR is related to this thread.
|
Conflicts with |
|
let me know when this is ready to stand for review again! |
|
Thank you 🙏 |
|
Thank YOU! Sorry this took so long to get sorted; i've had my hands full with real world things and am finally getting back to being able to spend some time on Bandit. Thanks for the patience. |
|
No worries! |
closes #410.
A re-worked attempt after #396, trying to improve it by taking some pieces from #398.