Log error at error level for 500 errors#894
Conversation
|
See also sigstore/fulcio#2393 |
AlCutter
left a comment
AlCutter
left a comment
There was a problem hiding this comment.
Most of these seem like a good call - they should be rare but you'd want to know about them.
| err = marshalAndWriteAddChainResponse(sct, w) | ||
| if err != nil { | ||
| // reason is logged and http status is already set | ||
| slog.ErrorContext(ctx, "Failed to write response", slog.Any("error", err)) |
There was a problem hiding this comment.
I'd be tempted to remove this one. All(?) the others are rare and indicative of something local going wrong (either local infra, or bugs).
This one, I suspect, is almost always just going to be a badly behaved client closing their socket early and so likely not actionable.
|
I think that error messages should already propagate to warnings on line 249. Is there a scenario in which they wouldn't? |
We are seeing 500 errors frequently with no logs at any level to indicate what's going on. |
When I searched explicitly for "handler error" at the warning level I finally found it, it had been lost in a sea of other warnings. Since this caused a 500 I think this should be logged at the Error level rather than Warning so that it would be easier to find. |
An HTTP 500 should be logged as an error since an operator likely needs to take action to fix the problem.
cmurphy
changed the title
|
I've changed this to not log within the individual handler but to log at an error level when the handler returns. |
3 participants
An HTTP 500 should be logged as an error since an operator likely needs
to take action to fix the problem.