-
Notifications
You must be signed in to change notification settings - Fork 77
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Proposal] Standardize errors and Logs with Middlewares #214
Conversation
1fe06f5
to
815b684
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👏
pkg/server/k8s/client.go
Outdated
if _, err := k.kc.CoreV1().Services(namespace).Create(srvSpec); err != nil { | ||
return errors.Wrap(err, "create service failed") | ||
} | ||
return nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess you can write:
_, err := k.kc.CoreV1().Services(namespace).Create(srvSpec)
return errors.Wrap(err, "create service failed")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does errors.Wraps
returns nil
for non errors (i.e. nil
) args ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome
pkg/server/deploy/handlers.go
Outdated
@@ -54,7 +55,7 @@ func (s *Service) Make(stream dpb.Deploy_MakeServer) error { | |||
rs := bytes.NewReader(content.Bytes()) | |||
rc, err := s.ops.Deploy(u, appName, rs, description, s.options.RevisionHistoryLimit) | |||
if err != nil { | |||
return err | |||
return teresa_errors.Get(err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought you could return err
here because the log middleware does the unwrapping, am I right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you're right!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔁
pkg/server/middlewares.go
Outdated
return teresa_errors.Get(err) | ||
} | ||
return nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe Error -> WithError
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I added the user of the request. Check some examples below and let know if you liked.
time="2017-07-18T14:32:31Z" level=error msg="Log Interceptor got an Error" error="namespaces \"hashpass2\" already exists" request=name:"hashpass2" team:"arquitetura" limits:<default:<quantity:"500m" resource:"cpu" > default:<quantity:"512Mi" resource:"memory" > default_request:<quantity:"200m" resource:"cpu" > default_request:<quantity:"512Mi" resource:"memory" > > auto_scale:<cpu_target_utilization:70 max:2 min:1 > route="/app.App/Create" user="diego.garcia@luizalabs.com"
time="2017-07-18T14:33:17Z" level=error msg="Log Interceptor got an Error" error="rpc error: code = NotFound desc = App not found: get annotation failed: namespaces \"hashpass3\" not found" route="/deploy.Deploy/Make" user="diego.garcia@luizalabs.com"
ps: In the unary interceptor, I'm logging request content.
b9c6b84
to
ac5e728
Compare
Only applied for
Deploy
pkg.This change is