verbose output

[xiekeyang]

close #14 and #22 .

This provides a contexted logger instance, which is exported from logrus, to print the information to stderr and stdout. It level the information through debug option.

Signed-off-by: xiekeyang [email protected]

[wking]

I'd still rather use Fatal instead of Fatalf when the format string is "%s" 1, but other than that 5deb77e looks good to me.

[xiekeyang]

@wking Fix in https://github.com/opencontainers/image-tools/pull/46/commits/7ff9b240279d98f0a3fec661a59981c51ce29da5

1. logrus.Fatalf("%s",err) -> logrus.Fatal(err)
2. logrus.Fatalf("constant string") -> logrus.Fatal("constant string")
3. For error level messages, I keep only using logrus.Errorf(), as I explained in https://github.com/opencontainers/image-tools/pull/46#discussion_r84591679.

[wking]

On Sun, Oct 30, 2016 at 11:20:43PM -0700, xiekeyang wrote:

1. logrus.Fatalf("%s",err) -> logrus.Fatal(err)
2. logrus.Fatalf("constant string") -> logrus.Fatal("constant string")
3. For error level messages, I keep only using logrus.Errorf(), as I explained in https://github.com/opencontainers/image-tools/pull/46#discussion_r84591679.

Errorf vs. Error seems the same as Fatalf vs. Fatal to me. But I'm fine landing this as it stands and filing a follow-up PR to address Errorf → Error in isolation.

[xiekeyang]

@opencontainers/image-tools-maintainers I think this PR may be ready to merged. PTAL.

[stevvooe]

@xiekeyang Make sure that logging is going to stderr to differentiate from problem output, which should go to stdout.

[xiekeyang]

@stevvooe

@xiekeyang Make sure that logging is going to stderr to differentiate from problem output, which should go to stdout.

I assume that problem output means messages consumer want to collect, like why validation failed, right? For example below use stderr,

if err := cmd.Usage(); err != nil {
        logrus.WithError(err).Errorf("usage failed")
}   

And below use stdout.

err := v.validatePath(arg) 
if err != nil { 
  fmt.Printf("error: validation failed: %s",err)
}

Is that what you want to say? If so, I'm not very clear about their difference.

[stevvooe]

@xiekeyang Yes, that is effectively what I am saying. However, the output to stdout should have some machine readable structure.

[xiekeyang]

@stevvooe I mean problem output prompted by validatePath might be open file error, json unmarshal error or so. So, I'm not sure how to differ stderr and stdout and I worry it might loss messages when consumer collect them. I honestly fell they are all stderr. I think they are printed as stderr(to use logrus.WithError(err).Errorf()), and consumer can collect all of them by console or by audit. I have no strong opinion, but could you reconsider it?

[xiekeyang]

@stevvooe After read my comment https://github.com/opencontainers/image-tools/pull/46#issuecomment-262130368, do you still want to use stdout to print "problem output", or think to use stderr to be fine? Thanks a lot!

[stevvooe]

@xiekeyang stdout should have only have errors that are part of the program output. For example, if the job of the program is to validate something, the errors in validation would go to standard out. If an error occurs trying to perform the validation, that output would go to stderr.

Simply put, I would print all of logrus to stderr and then control program output through fmt.Printfln or equivalent wrapper.

[xiekeyang]

@stevvooe PTAL https://github.com/opencontainers/image-tools/pull/46/commits/565f138efbfabadff79c3be24bc2b657cbef4bee. Here I fix the stdout and stderr to your proposal, to use logrus for stderr, and go log(Printf and Println) for stdout. I implement the new package logger, to provide public logger structure for each command. If it is what you want? Thanks a lot!

[stevvooe]

Here I fix the stdout and stderr to your proposal, to use logrus for stderr, and go log(Printf and Println) for stdout.

This is not really what I said or meant. :/

There isn't really a point to using both the standard logger and and logrus. They are redundant.

What I said was that you need to differentiate program output, ie. validation, and status of the program with stdout and stderr respectively.

Let's say we have output, tagged with the stream (this is an example, don't add it), it would look like this:

stderr: opened manifest.json for validation
stdout: ok manifest.json
stderr: opened foo.json for validation
stdout: fail foo.json
stdout:   error bad media type
stdout:   error missing digest
stderr: failed to open notexist.json

Typically, we use a logger to write to stderr, which can be logrus to provide leveled logging.

This distinction is the hallmark of a well structured unix tool. It allows us to pipe program output somewhere, while monitoring stderr for behavior.

I hope this clarifies the requirements here. (if anyone else has a link explaining this concept, it would be helpful).

[xiekeyang]

@stevvooe ,

update it in https://github.com/opencontainers/image-tools/pull/46/commits/b619fba6fd9f711de256e3b3bec65e20602a204b.

It should have coverred what you want. PTAL, thanks a lot!

[xiekeyang]

And update the PR description at https://github.com/opencontainers/image-tools/pull/46#issue-184446882

[xiekeyang]

@stevvooe @vbatts @philips PTAL.

[vbatts]

LGTM, but rebase is needed

[xiekeyang]

rebased, PTAL.

[vbatts]

LGTM

[stevvooe]

I'm still confused about the role of a full blown logger in a CLI tool. Shouldn't we do something more user friendly?

[xiekeyang]

What is the purpose of this package? However, please let me know what the intent of this package and we can get it into shape. I'm still confused about the role of a full blown logger in a CLI tool. Shouldn't we do something more user friendly?

I'm in progress for this PR. Excuse me that I might only partly understand you. docker/containerd and docker/distribution are using individual, and public package to organize log, and provide an instance. Yes I should wrapped it via go context, for avoiding racing purpose, and we can add more contexts in future. So do you only mean to introduce go context, or you have some another proposal hint? Thanks so much.

[xiekeyang]

@stevvooe

What is the purpose of this package? However, please let me know what the intent of this package and we can get it into shape. I'm still confused about the role of a full blown logger in a CLI tool. Shouldn't we do something more user friendly?

I'm in progress for this PR. Excuse me that I might only partly understand you. docker/containerd and docker/distribution are using individual, and public package to organize log, and provide an instance. Yes I should wrapped it via go context, for go racing purpose, and we can add more contexts in future. So do you only mean to introduce go context, or you have some another proposal hint? Thanks so much.

[stevvooe]

@xiekeyang I don't understand why you need a leveled logger in a command line tool. It makes no sense.

[xiekeyang]

@stevvooe You might just want xx-tool -D/--debug. However we note log-level is the way on runtime-tools and docker/containerd repo. I think consumer might have the same case on image-tools. So please reconsider it. Thank lots!

[stevvooe]

docker/containerd repo.

Don't use this as an example: this is very new code and half of it is a daemon.

[xiekeyang]

@stevvooe OK. So I use it as image-tool-xx -D/--debug for logrus.Debugf print. Is that alright?

[xiekeyang]

@stevvooe @opencontainers/image-tools-maintainers This is updated.

1. Logger instance should be retrieved through golang.org/x/net/context.
2. Set up debug mode for logrus.Debugf(), instead of log-level.
3. Fire messages to stderr and stdout devices. PTAL, thank lots!

[vbatts]

i like the idea of using context for sharing the logger

[cyphar]

I also really like it, because it means that the CAS and other functions in umoci that will move here should in principle just be able to use the context logging (I didn't even know you could do that -- which is why I'm stuck with apex in umoci :wink:).

[xiekeyang]

@opencontainers/image-tools-maintainers It is updated, PTAL, thanks.

[vbatts]

Seems like a good starting point. Thanks @xiekeyang LGTM