Skip to content
This repository was archived by the owner on Mar 9, 2022. It is now read-only.

Conversation

@Random-Liu
Copy link
Member

Move all grpc handler logs into a instrumented service, so that we don't need to care about it in the real service.

Signed-off-by: Lantao Liu [email protected]

@abhi
Copy link
Member

abhi commented Aug 22, 2017

Should we convert those logs to something like debugf in logrus. I see that glog has severity level may be print those logs only if the logging level is set appropriately ? Instead of an instrumented code.

@Random-Liu
Copy link
Member Author

Random-Liu commented Aug 22, 2017

@abhinandanpb I did this not for the log level. :) Just extract the log logic into an outside wrapper, so that we don't need to see this everywhere. :)

More of a cleanup.

glog.V(2).Infof("RunPodSandbox with config %+v", r.GetConfig())
defer func() {
if err != nil {
glog.Errorf("RunPodSandbox for %+v returns error %v", r.GetConfig().GetMetadata(), err)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

glog.Errorf("RunPodSandbox for %+v failed, err: %v", r.GetConfig().GetMetadata(), err)
would be better ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will update all.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Signed-off-by: Lantao Liu <[email protected]>
@Random-Liu Random-Liu force-pushed the add-instrumented-service branch from f935cd4 to 195b525 Compare August 23, 2017 07:02
@abhi
Copy link
Member

abhi commented Aug 23, 2017

LGTM

@abhi abhi added the lgtm label Aug 23, 2017
Copy link
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/LGTM nice

@Random-Liu
Copy link
Member Author

This touches many files, and relatively lower priority. I'll hold this until #138 and #128 get merged.

/cc @abhinandanpb

@k8s-ci-robot k8s-ci-robot requested a review from abhi August 23, 2017 23:41
@Random-Liu Random-Liu merged commit 73bb969 into containerd:master Aug 24, 2017
@Random-Liu Random-Liu deleted the add-instrumented-service branch August 24, 2017 18:26
lanchongyizu pushed a commit to lanchongyizu/cri-containerd that referenced this pull request Sep 3, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants