Replace Glog with Zap Logger #378
Conversation
Pull Request Test Coverage Report for Build 6042045094
💛 - Coveralls |
There was a problem hiding this comment.
Reviewed half of it. Before going further few major comments.
- Approach, lets agree on the that. If we touch code, we dont want to come back again for another logging improvement. So it's better to have a fewer but complete changes, than many and come back again. Why come back again, will list below.
- glog.V6 is debug, there are many places where debug converted into info log.
- glog.v2 is info, there are places where info converted into debug.
- multi-line logging - merge into one
- log error or return error, dont do both
- statements with
foo,err := getFoo(), and then we log both log.Info(foo, err), not good, if error happened and we handle it by logging, we do if err != nil { log(err) } - Info logging with %v - we talked about it - no %v, log only fields that we care about. If not sure which fields are important, log name or id. For debugging %v can be used, carefully.
| @@ -39,7 +40,7 @@ func NewDefaultLattice(sess *session.Session, region string) *defaultLattice { | |||
|
|
|||
| latticeSess = vpclattice.New(sess, aws.NewConfig().WithRegion(region).WithEndpoint(endpoint).WithMaxRetries(20)) | |||
|
|
|||
| glog.V(2).Infoln("Lattice Service EndPoint:", endpoint) | |||
| log.Debugln("Lattice Service EndPoint:", endpoint) | |||
There was a problem hiding this comment.
Can be removed. I dont think we need log in defaultLattice, can be at controller_config since it's env_variable
| cloud: cloud, | ||
| latticeDataStore: latticeDataStore, | ||
| } | ||
| } | ||
|
|
||
| func (s *defaultListenerManager) Create(ctx context.Context, listener *latticemodel.Listener) (latticemodel.ListenerStatus, error) { | ||
| glog.V(6).Infof("Creating listener >>>> %v \n", listener) | ||
| s.log.Infof("Creating listener >>>> %v", listener) |
There was a problem hiding this comment.
I would log only a few fields from listener, not %v, most important ones, like name and may be port. Especially with info level.
| s.log.Infof("Error during create listner %s", errmsg) | ||
| return latticemodel.ListenerStatus{}, errors.New(errmsg) |
There was a problem hiding this comment.
no error logging, when we return error
| return latticemodel.ListenerStatus{}, errors.New(errmsg) | ||
| } | ||
|
|
||
| lis, err := s.findListenerByNamePort(ctx, serviceStatus.ID, listener.Spec.Port) | ||
|
|
||
| glog.V(6).Infof("findListenerByNamePort %v , lisenter %v error %v\n", listener, lis, err) | ||
| s.log.Infof("findListenerByNamePort %v , lisenter %v error %v", listener, lis, err) |
There was a problem hiding this comment.
I think we already logged listener above, no need to dump it again. Also when there is error I would log error separately with if condition. It seems in this case we drop error handling, so logging error right here is ok.
| s.log.Debugln("############req creating listner ###########") | ||
| s.log.Debugln(listenerInput) | ||
| s.log.Debugln("############resp creating listner ###########") | ||
| s.log.Debugf("create listener err :%v", err) | ||
| s.log.Debugln(resp) |
| @@ -29,28 +36,28 @@ func (r *ruleSynthesizer) Synthesize(ctx context.Context) error { | |||
|
|
|||
| err := r.stack.ListResources(&resRule) | |||
|
|
|||
| glog.V(6).Infof("Synthesize rule = %v, err :%v \n", resRule, err) | |||
| r.log.Infof("Synthesize rule = %v, err :%v", resRule, err) | |||
There was a problem hiding this comment.
if err, then log error
| glog.V(6).Infof("Failed to create rule %v, err :%v \n", rule, err) | ||
| r.log.Infof("Failed to create rule %v, err :%v", rule, err) | ||
| return err |
There was a problem hiding this comment.
no log on error return
| rule.Status = &ruleResp | ||
| } | ||
|
|
||
| // handle delete | ||
| sdkRules, err := r.getSDKRules(ctx) | ||
| glog.V(6).Infof("rule>>> synthesize, sdkRules :%v err: %v \n", sdkRules, err) | ||
| r.log.Infof("rule>>> synthesize, sdkRules :%v err: %v", sdkRules, err) |
| glog.V(6).Infof("findMatchRule, rule not found err:%v\n", err) | ||
| r.log.Infof("findMatchRule, rule not found err:%v", err) | ||
| return modelRule, errors.New("rule not found") |
| glog.V(6).Infof("no HTTPMatch ") | ||
| r.log.Infof("no HTTPMatch ") | ||
| return modelRule, errors.New("rule not found") |
|
In addition - let's remove "#####" or ">>>>>" style logs whenever possible (sorry not sure how to call these) This could be too much for one PR so we may just split the PR by components - (do controller, do manager, do modelbuild, and so on) |
|
btw I'm totally happy with replacing a few info logs with debug logs by good judgement. I would like to see minimal set of logs when debug mode is off |
|
Going to close this and use it as reference for fresh, smaller PRs. First one is up |
What type of PR is this?
cleanup
Which issue does this PR fix:
#126 partially
What does this PR do / Why do we need it:
If an issue # is not available please add repro steps and logs from aws-gateway-controller showing the issue:
Testing done on this change:
N/A
Automation added to e2e:
N/A
Will this PR introduce any new dependencies?:
No
Will this break upgrades or downgrades. Has updating a running cluster been tested?:
No. N/A
Does this PR introduce any user-facing change?:
Logs will be a bit cleaner, but nothing else.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.