Skip to content
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

V style logging #112

Merged
merged 5 commits into from
Nov 14, 2016
Merged

V style logging #112

merged 5 commits into from
Nov 14, 2016

Conversation

ivan4th
Copy link
Contributor

@ivan4th ivan4th commented Nov 14, 2016

Virtlet logging settings:
-v=0: no info logging. V(0) can be used for some important info messages but as of now it isn't used in such way.
-v=1: minimal info logging (startup message)
-v=2: same logging as before this PR. This level is set in CMD in Dockerfile
-v=3: added back some of the logging removed in #73 on this log level. Complex objects are dumped using go-spew

Level 4 and higher isn't used at the moment.

It's now possible to set log level for docker-compose like this:

VIRTLET_LOGLEVEL=3 docker-compose up

Besides, VirtletManager.ListImages() was not returning nil response in case of error,
fixed it.

Fixes #74


This change is Reviewable

@jellonek
Copy link
Contributor

Small nits.


Reviewed 7 of 7 files at r1.
Review status: all files reviewed at latest revision, 5 unresolved discussions.


pkg/manager/manager.go, line 151 at r1 (raw file):

  }
  response := &kubeapi.PodSandboxStatusResponse{Status: status}
  glog.V(3).Infof("PodSandboxStatus response: #%v", status)

As this status does not have an useful Stringer it's IMO pointless to add such log. A bit better would be use of status.GetStatus().GetState() but AFAIR it also has some dummy crap there.


pkg/manager/manager.go, line 157 at r1 (raw file):

func (v *VirtletManager) ListPodSandbox(ctx context.Context, in *kubeapi.ListPodSandboxRequest) (*kubeapi.ListPodSandboxResponse, error) {
  filter := in.GetFilter()
  glog.V(3).Infof("Listing sandboxes with filter: %#v", filter)

Same as above for status.


pkg/manager/manager.go, line 244 at r1 (raw file):

  }
  response := &kubeapi.ListContainersResponse{Containers: containers}
  glog.V(3).Infof("ListContainers rseponse: %#v", response)

Typo. Also same as status, response in this manner is unusable/unreadable.


pkg/manager/manager.go, line 269 at r1 (raw file):

  if err != nil {
      glog.Errorf("Error when listing images: %v", err)
      return nil, err

Good catch.


pkg/manager/manager.go, line 272 at r1 (raw file):

  }
  response := &kubeapi.ListImagesResponse{Images: images}
  glog.V(3).Infof("ListImages response: %#v", err)

There should be simple %v.


Comments from Reviewable

@ivan4th
Copy link
Contributor Author

ivan4th commented Nov 14, 2016

Review status: all files reviewed at latest revision, 5 unresolved discussions.


pkg/manager/manager.go, line 157 at r1 (raw file):

Previously, jellonek (Piotr Skamruk) wrote…

Same as above for status.

Are filter details really useless? (Id, State, LabelSelector)? Maybe you meant response output below?

pkg/manager/manager.go, line 272 at r1 (raw file):

Previously, jellonek (Piotr Skamruk) wrote…

There should be simple %v.

Are you sure that image list is readable enough when displayed via plain `%v`? That's spammy logging level, maybe it's ok to have more output there?

Comments from Reviewable

@jellonek
Copy link
Contributor

Review status: all files reviewed at latest revision, 5 unresolved discussions.


pkg/manager/manager.go, line 157 at r1 (raw file):

Previously, ivan4th (Ivan Shvedunov) wrote…

Are filter details really useless? (Id, State, LabelSelector)?
Maybe you meant response output below?

AFAIR they look like "random pointers" in this output. Am I wrong? I have to recheck produced output... Instead of getting real values for Id, State, LabelSelector in effect you will see only "some pointers" as values.

pkg/manager/manager.go, line 272 at r1 (raw file):

Previously, ivan4th (Ivan Shvedunov) wrote…

Are you sure that image list is readable enough when displayed via plain %v? That's spammy logging level, maybe it's ok to have more output there?

You are returning there `err`, not image list - so we are both wrong :) `ListImagesResponse` (as other similar responses) have only stub for `Stringer` interfaces which is not recursive and so, it only shows pointer to list which it have inside, so IMO it's useless info there...

Comments from Reviewable

@jellonek
Copy link
Contributor

:lgtm:


Reviewed 10 of 10 files at r2.
Review status: all files reviewed at latest revision, 5 unresolved discussions.


Comments from Reviewable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add V-style logging to virtlet
2 participants