Handle panic at endpoint #458
Conversation
herainman
force-pushed
the
xuantest2
branch
2 times, most recently
from
cac6fa3
to
5a4accb
Compare
|
I finally added UTs... |
codegen/templates/endpoint.tmpl
Outdated
| zap.String("stacktrace", stacktrace), | ||
| zap.String("endpoint", h.endpoint.EndpointName)) | ||
|
|
||
| res.SendError(502, "Unexpected workflow panic, recovered at endpoint.", e) |
There was a problem hiding this comment.
You should probably not send the error to the client. The error may have debug or sensitive information.
| ctx context.Context, | ||
| headers zanzibar.Header, | ||
| ) (string, zanzibar.Header, error) { | ||
| defer func() { |
There was a problem hiding this comment.
You don't need to defer this. Just call panic() directly.
| if r := recover(); r != nil { | ||
| stacktrace := string(debug.Stack()) | ||
| e := errors.Errorf("enpoint panic: %v, stacktrace: %v", r, stacktrace) | ||
| h.Dependencies.Default.ContextLogger.Error( |
There was a problem hiding this comment.
Can you also increment a specific metric for panics so we can add alerting on specifically panics?
| h.endpointScope.Counter("endpoint.panic").Inc(1) | ||
| isSuccessful = false | ||
| response = nil | ||
| headers = map[string]string{} |
There was a problem hiding this comment.
nit: do we want to preserve some header information if any
There was a problem hiding this comment.
return nil instead of empty, this is the same behavior https://github.com/uber/zanzibar/pull/458/files/ef542b0b5925a2d569a7fc6d424379c6d39ff036#diff-7c8f7ec39bf077182fa0b404b3d49d76R124 when other error happens
| @@ -54,6 +58,9 @@ func NewBarArgNotStructHandler(deps *module.Dependencies) *BarArgNotStructHandle | |||
| "bar", "argNotStruct", | |||
| handler.HandleRequest, | |||
| ) | |||
| handler.endpointScope = deps.Default.Scope.Tagged(map[string]string{ | |||
There was a problem hiding this comment.
let's just reuse this scope: https://github.com/uber/zanzibar/blob/master/runtime/router.go#L70
|
not sure why build and coverage not showing as the gate. However, I can see it is already passed the build |
|
thank you for review |
This commits