Added simple logging capability /w test

Mostly inspired from goetveld

https://code.launchpad.net/~mathieu-lonjaret/juju/juju-go-log/+merge/84074

(do not edit description out of merge proposal)

[niemeyer, 2011-12-01 10:09:45]

Pretty nice, thanks for pushing this Mathieu.

A few comments:

https://codereview.appspot.com/5448072/diff/1/juju/log.go
File juju/log.go (right):

https://codereview.appspot.com/5448072/diff/1/juju/log.go#newcode10
juju/log.go:10: // often the logger is plugged in, it would be worth using the
type instead.
Please drop both of the comments above. I know they are my own, since they come
from the file I suggested, but they're bogus by now. The main reason we're using
that interface now is to enable simpler loggers for testing purposes (e.g. you
can pass *gocheck.C to SetLogger).

https://codereview.appspot.com/5448072/diff/1/juju/log.go#newcode11
juju/log.go:11: type log_Logger interface {
Let's make that interface external please: s/log_Logger/Logger/.

https://codereview.appspot.com/5448072/diff/1/juju/log.go#newcode31
juju/log.go:31: 
// Logf logs the formatted message onto the Logger set via SetLogger.

https://codereview.appspot.com/5448072/diff/1/juju/log.go#newcode37
juju/log.go:37: 
// Debugf logs the formatted message onto the Logger set via SetLogger, as long
as debugging was enabled with SetDebug.

https://codereview.appspot.com/5448072/diff/1/juju/log_test.go
File juju/log_test.go (right):

https://codereview.appspot.com/5448072/diff/1/juju/log_test.go#newcode33
juju/log_test.go:33: buf := bytes.NewBuffer(make([]byte, 0))
buf := &bytes.Buffer{}

https://codereview.appspot.com/5448072/diff/1/juju/log_test.go#newcode47
juju/log_test.go:47: }
Nice test.

[mpl, 2011-12-01 10:30:25]

https://codereview.appspot.com/5448072/diff/1/juju/log.go
File juju/log.go (right):

https://codereview.appspot.com/5448072/diff/1/juju/log.go#newcode10
juju/log.go:10: // often the logger is plugged in, it would be worth using the
type instead.
On 2011/12/01 10:09:45, niemeyer wrote:
> Please drop both of the comments above. I know they are my own, since they
come
> from the file I suggested, but they're bogus by now. The main reason we're
using
> that interface now is to enable simpler loggers for testing purposes (e.g. you
> can pass *gocheck.C to SetLogger).

Done.

https://codereview.appspot.com/5448072/diff/1/juju/log.go#newcode11
juju/log.go:11: type log_Logger interface {
On 2011/12/01 10:09:45, niemeyer wrote:
> Let's make that interface external please: s/log_Logger/Logger/.

Done.

https://codereview.appspot.com/5448072/diff/1/juju/log.go#newcode31
juju/log.go:31: 
On 2011/12/01 10:09:45, niemeyer wrote:
> // Logf logs the formatted message onto the Logger set via SetLogger.

Done.

https://codereview.appspot.com/5448072/diff/1/juju/log.go#newcode37
juju/log.go:37: 
On 2011/12/01 10:09:45, niemeyer wrote:
> // Debugf logs the formatted message onto the Logger set via SetLogger, as
long
> as debugging was enabled with SetDebug.

Done.

https://codereview.appspot.com/5448072/diff/1/juju/log_test.go
File juju/log_test.go (right):

https://codereview.appspot.com/5448072/diff/1/juju/log_test.go#newcode33
juju/log_test.go:33: buf := bytes.NewBuffer(make([]byte, 0))
On 2011/12/01 10:09:45, niemeyer wrote:
> buf := &bytes.Buffer{}

Done.

[niemeyer, 2011-12-01 10:33:31]

LGTM, thank you.

[rog, 2011-12-01 10:42:24]

i'm not sure i see where this package is going.
it seems to me that we could just have a package like the following and leverage
the functionality of the existing log package. but i'm probably missing
something.

package log
import "log"

// one variable for each debug level
var Event *log.Logger
var Debug *log.Logger

func init() {
    // set up the various loggers.
}

https://codereview.appspot.com/5448072/diff/4002/juju/log.go
File juju/log.go (right):

https://codereview.appspot.com/5448072/diff/4002/juju/log.go#newcode6
juju/log.go:6: Output(calldepth int, s string) error
Print?

https://codereview.appspot.com/5448072/diff/4002/juju/log.go#newcode10
juju/log.go:10: var globalDebug bool
could just make these global variables to avoid the need for SetLogger and
SetDebug.

https://codereview.appspot.com/5448072/diff/4002/juju/log.go#newcode29
juju/log.go:29: func Logf(format string, v ...interface{}) {
Printf?

[gustavo_niemeyer.net, 2011-12-01 10:54:37]

> i'm not sure i see where this package is going.
> it seems to me that we could just have a package like the following and
> leverage the functionality of the existing log package. but i'm probably
> missing something.

There are several points here:

- juju.Logf and Debugf are short and comfortable to use

- The Logger interface enables using *gocheck.C as a logger, so we
only see messages on tests that fail.

- The implemented interface allows us to call both Logf and Debugf,
irrespective of whether a Logger is set.

- Logging debug messages with the implemented interface has close to
zero cost if debugging is disabled.

> https://codereview.appspot.com/5448072/diff/4002/juju/log.go#newcode6
> juju/log.go:6: Output(calldepth int, s string) error
> Print?

No, that's not our interface. log.Logger already exists.

> https://codereview.appspot.com/5448072/diff/4002/juju/log.go#newcode10
> juju/log.go:10: var globalDebug bool
> could just make these global variables to avoid the need for SetLogger
> and SetDebug.

That sounds like a nice idea.

> https://codereview.appspot.com/5448072/diff/4002/juju/log.go#newcode29
> juju/log.go:29: func Logf(format string, v ...interface{}) {
> Printf?

It's fine to say log.Printf, but it isn't fine to say juju.Printf.
juju.Logf logs, doesn't print.

-- 
Gustavo Niemeyer
http://niemeyer.net
http://niemeyer.net/plus
http://niemeyer.net/twitter
http://niemeyer.net/blog

-- I'm not absolutely sure of anything.

[rog, 2011-12-01 11:24:40]

On 1 December 2011 10:54, Gustavo Niemeyer <gustavo@niemeyer.net> wrote:
>> i'm not sure i see where this package is going.
>> it seems to me that we could just have a package like the following and
>> leverage the functionality of the existing log package. but i'm probably
>> missing something.
>
> There are several points here:
>
> - juju.Logf and Debugf are short and comfortable to use

i can't dispute that, but using Printf means that the calls
will be automicatically checked by govet, which is a nice property.

> - The Logger interface enables using *gocheck.C as a logger, so we
> only see messages on tests that fail.

i don't see that *gocheck.C implements Output (and i'm not sure
it should). it's entirely possible to use *gocheck.C as a logger with
the standard logging package too, if we want:

type clogger struct {
   c *C
}
func (clogger) Write(buf []byte) (int, error) {
    n := len(buf)
    if n > 0 && buf[n - 1] == '\n' {
       n--
    }
    c.Logf("%s", buf[0:n])
    return len(buf), nil
}
func NewCLogger(c *C) *log.Logger {
    return log.New(clogger{c}, "", 0)
}

> - The implemented interface allows us to call both Logf and Debugf,
> irrespective of whether a Logger is set.

we can ensure that a valid logger is always set (by initialising
to a null writer if necessary).

> - Logging debug messages with the implemented interface has close to
> zero cost if debugging is disabled.

that depends if the arguments to the debug message take
time to generate (which argues for an explicitly visible DebugEnabled
flag, or some such), but i see your point.

that issue could alternatively be solved by adding an Ldisabled
flag to the log package.

BTW is performance really an issue here?

>> https://codereview.appspot.com/5448072/diff/4002/juju/log.go#newcode29
>> juju/log.go:29: func Logf(format string, v ...interface{}) {
>> Printf?
>
> It's fine to say log.Printf, but it isn't fine to say juju.Printf.
> juju.Logf logs, doesn't print.

i hadn't noticed the package name and had assumed it was an
independent package, juju/log.
any particular reason for including in the the main juju package?
by including there we preclude its use by other packages
that juju depends on.

my main concern here is that we avoid reinventing a perfectly ok wheel.

[gustavo_niemeyer.net, 2011-12-01 11:36:04]

> i don't see that *gocheck.C implements Output (and i'm not sure

It does.

> we can ensure that a valid logger is always set (by initialising
(...)
> that depends if the arguments to the debug message take
(...)
> BTW is performance really an issue here?

You are consistently advocating for doing things in a different way
despite the fact that there are benefits in the current approach. I
can't argue further.

> my main concern here is that we avoid reinventing a perfectly ok wheel.

Are you seriously suggesting that these two functions with three lines
each are reinventing log somehow?

Please take a step back and read the code. They *rely* on log.

-- 
Gustavo Niemeyer
http://niemeyer.net
http://niemeyer.net/plus
http://niemeyer.net/twitter
http://niemeyer.net/blog

-- I'm not absolutely sure of anything.

[rog, 2011-12-01 11:47:53]

On 1 December 2011 11:35, Gustavo Niemeyer <gustavo@niemeyer.net> wrote:
>> i don't see that *gocheck.C implements Output (and i'm not sure
>
> It does.

it's a while since i've updated gocheck, i see.
note to self: use goneat rather than godoc and grep!

>> my main concern here is that we avoid reinventing a perfectly ok wheel.
>
> Are you seriously suggesting that these two functions with three lines
> each are reinventing log somehow?
>
> Please take a step back and read the code. They *rely* on log.

that's a good point actually.
i was thinking of it at a different level, sorry.

i do thing that it should be in a separate package though, for
the reason i gave earlier: it would be a pity if no packages
that juju depends on could generate a juju log message.

then the standard usage could be just the same as with the
normal log, only with a different import path.

import "lauchpad.net/juju/log"
log.Printf("foo")

[gustavo_niemeyer.net, 2011-12-01 11:51:18]

> then the standard usage could be just the same as with the
> normal log, only with a different import path.
>
> import "lauchpad.net/juju/log"
> log.Printf("foo")

You have a good point too, and I'm happy with that.

-- 
Gustavo Niemeyer
http://niemeyer.net
http://niemeyer.net/plus
http://niemeyer.net/twitter
http://niemeyer.net/blog

-- I'm not absolutely sure of anything.

[niemeyer, 2011-12-13 21:00:38]

This was already merged.