Merge lp:~thumper/juju-core/update-loggo into lp:~juju/juju-core/trunk

[Tim Penhey]

Updates for latest loggo.

Since we don't yet have dependency management, the latest
changes to loggo are unpushed until I get this agreed upon.

https://codereview.appspot.com/10125046/

[Tim Penhey (thumper)]

Reviewers: mp+168342_code.launchpad.net,

Message:
Please take a look.

Description:
Updates for latest loggo.

Since we don't yet have dependency management, the latest
changes to loggo are unpushed until I get this agreed upon.

https://code.launchpad.net/~thumper/juju-core/update-loggo/+merge/168342

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/10125046/

Affected files:
   A [revision details]
   M cmd/builddb/main.go
   M cmd/juju/main_test.go
   M cmd/logging.go
   M cmd/logging_test.go
   M log/log.go
   M testing/log.go
   M worker/provisioner/provisioner.go
   M worker/provisioner/provisioner_task.go
   M worker/uniter/jujuc/juju-log_test.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: <email address hidden>
+New revision: <email address hidden>

Index: cmd/logging.go
=== modified file 'cmd/logging.go'
--- cmd/logging.go 2013-06-07 01:00:23 +0000
+++ cmd/logging.go 2013-06-10 04:25:51 +0000
@@ -57,9 +57,9 @@
    if l.Debug {
     level = loggo.DEBUG
    }
- logger := loggo.GetLogger("juju")
- logger.SetLogLevel(level)
+ // Set the level on the root logger.
+ loggo.GetLogger("").SetLogLevel(level)
   }
- loggo.ConfigureLogging(l.Config)
+ loggo.ConfigureLoggers(l.Config)
   return nil
  }

Index: cmd/logging_test.go
=== modified file 'cmd/logging_test.go'
--- cmd/logging_test.go 2013-06-06 03:55:22 +0000
+++ cmd/logging_test.go 2013-06-10 04:25:51 +0000
@@ -46,8 +46,7 @@
   err := l.Start(ctx)
   c.Assert(err, IsNil)

- logger := loggo.GetLogger("juju")
- c.Assert(logger.GetLogLevel(), Equals, loggo.INFO)
+ c.Assert(loggo.GetLogger("").LogLevel(), Equals, loggo.INFO)
  }

  func (s *LogSuite) TestDebugSetsLogLevel(c *C) {
@@ -56,8 +55,7 @@
   err := l.Start(ctx)
   c.Assert(err, IsNil)

- logger := loggo.GetLogger("juju")
- c.Assert(logger.GetLogLevel(), Equals, loggo.DEBUG)
+ c.Assert(loggo.GetLogger("").LogLevel(), Equals, loggo.DEBUG)
  }

  func (s *LogSuite) TestStderr(c *C) {

Index: cmd/builddb/main.go
=== modified file 'cmd/builddb/main.go'
--- cmd/builddb/main.go 2013-05-29 05:29:57 +0000
+++ cmd/builddb/main.go 2013-06-10 04:25:51 +0000
@@ -57,13 +57,13 @@
    return err
   }

- logger.Info("Waiting for unit to reach %q status...",
params.StatusStarted)
+ logger.Infof("Waiting for unit to reach %q status...",
params.StatusStarted)
   unit := units[0]
   last, info, err := unit.Status()
   if err != nil {
    return err
   }
- logger.Info("Unit status is %q: %s", last, info)
+ logger.Infof("Unit status is %q: %s", last, info)
   for last != params.StatusStarted {
    time.Sleep(2 * time.Second)
    if err := unit.Refresh(); err != nil {
@@ -74,7 +74,7 @@
     return err
    }
    if status != last {
- logger.Info("Unit status is %q: %s", status, info)
+ logger.Infof("Unit status is %q: %s", status, info)
     last = status
    }
   }
@@ -82,7 +82,7 @@
   if !ok {
    return fmt.Errorf("cannot retrieve files: build unit lacks a
public-address...

[John A Meinel (jameinel)]

You didn't really summarize what the changes are. I'm guessing it is
mostly changing names from "Info" to "Infof" to follow the common idiom
that things which take a format + args end in an extra 'f'. (printf,
etc).

I think I'm fine with that, but I haven't seen it discussed.

LGTM

https://codereview.appspot.com/10125046/

[Roger Peppe (rogpeppe)]

LGTM

could I suggest also adding the various log functions to
.lbox.check ? perhaps as a separate CL

(I believe the version check should change there - currently
it checks that the Go version is devel, but 1.1 is also
sufficient now).

On 10 June 2013 05:31, Tim Penhey <email address hidden> wrote:
> Reviewers: mp+168342_code.launchpad.net,
>
> Message:
> Please take a look.
>
> Description:
> Updates for latest loggo.
>
> Since we don't yet have dependency management, the latest
> changes to loggo are unpushed until I get this agreed upon.
>
> https://code.launchpad.net/~thumper/juju-core/update-loggo/+merge/168342
>
> (do not edit description out of merge proposal)
>
>
> Please review this at https://codereview.appspot.com/10125046/
>
> Affected files:
> A [revision details]
> M cmd/builddb/main.go
> M cmd/juju/main_test.go
> M cmd/logging.go
> M cmd/logging_test.go
> M log/log.go
> M testing/log.go
> M worker/provisioner/provisioner.go
> M worker/provisioner/provisioner_task.go
> M worker/uniter/jujuc/juju-log_test.go
>
>
> Index: [revision details]
> === added file '[revision details]'
> --- [revision details] 2012-01-01 00:00:00 +0000
> +++ [revision details] 2012-01-01 00:00:00 +0000
> @@ -0,0 +1,2 @@
> +Old revision: <email address hidden>
> +New revision: <email address hidden>
>
> Index: cmd/logging.go
> === modified file 'cmd/logging.go'
> --- cmd/logging.go 2013-06-07 01:00:23 +0000
> +++ cmd/logging.go 2013-06-10 04:25:51 +0000
> @@ -57,9 +57,9 @@
> if l.Debug {
> level = loggo.DEBUG
> }
> - logger := loggo.GetLogger("juju")
> - logger.SetLogLevel(level)
> + // Set the level on the root logger.
> + loggo.GetLogger("").SetLogLevel(level)
> }
> - loggo.ConfigureLogging(l.Config)
> + loggo.ConfigureLoggers(l.Config)
> return nil
> }
>
>
> Index: cmd/logging_test.go
> === modified file 'cmd/logging_test.go'
> --- cmd/logging_test.go 2013-06-06 03:55:22 +0000
> +++ cmd/logging_test.go 2013-06-10 04:25:51 +0000
> @@ -46,8 +46,7 @@
> err := l.Start(ctx)
> c.Assert(err, IsNil)
>
> - logger := loggo.GetLogger("juju")
> - c.Assert(logger.GetLogLevel(), Equals, loggo.INFO)
> + c.Assert(loggo.GetLogger("").LogLevel(), Equals, loggo.INFO)
> }
>
> func (s *LogSuite) TestDebugSetsLogLevel(c *C) {
> @@ -56,8 +55,7 @@
> err := l.Start(ctx)
> c.Assert(err, IsNil)
>
> - logger := loggo.GetLogger("juju")
> - c.Assert(logger.GetLogLevel(), Equals, loggo.DEBUG)
> + c.Assert(loggo.GetLogger("").LogLevel(), Equals, loggo.DEBUG)
> }
>
> func (s *LogSuite) TestStderr(c *C) {
>
>
> Index: cmd/builddb/main.go
> === modified file 'cmd/builddb/main.go'
> --- cmd/builddb/main.go 2013-05-29 05:29:57 +0000
> +++ cmd/builddb/main.go 2013-06-10 04:25:51 +0000
> @@ -57,13 +57,13 @@
> return err
> }
>
> - logger.Info("Waiting for unit to reach %q status...",
> params.StatusStarted)
> + logger.Infof("Wai...

[Roger Peppe (rogpeppe)]

This CL should give some context: https://codereview.appspot.com/10043045/

On 10 June 2013 09:28, John A Meinel <email address hidden> wrote:
> You didn't really summarize what the changes are. I'm guessing it is
> mostly changing names from "Info" to "Infof" to follow the common idiom
> that things which take a format + args end in an extra 'f'. (printf,
> etc).
>
> I think I'm fine with that, but I haven't seen it discussed.
>
> LGTM
>
>
> https://codereview.appspot.com/10125046/
>
> --
> https://code.launchpad.net/~thumper/juju-core/update-loggo/+merge/168342
> Your team juju hackers is requested to review the proposed merge of lp:~thumper/juju-core/update-loggo into lp:juju-core.

[Tim Penhey (thumper)]

*** Submitted:

Updates for latest loggo.

Since we don't yet have dependency management, the latest
changes to loggo are unpushed until I get this agreed upon.

R=jameinel
CC=
https://codereview.appspot.com/10125046

https://codereview.appspot.com/10125046/

=== modified file 'cmd/builddb/main.go'
--- cmd/builddb/main.go	2013-05-29 05:29:57 +0000
+++ cmd/builddb/main.go	2013-06-10 04:33:28 +0000
@@ -57,13 +57,13 @@
 		return err
 	}
 
-	logger.Info("Waiting for unit to reach %q status...", params.StatusStarted)
+	logger.Infof("Waiting for unit to reach %q status...", params.StatusStarted)
 	unit := units[0]
 	last, info, err := unit.Status()
 	if err != nil {
 		return err
 	}
-	logger.Info("Unit status is %q: %s", last, info)
+	logger.Infof("Unit status is %q: %s", last, info)
 	for last != params.StatusStarted {
 		time.Sleep(2 * time.Second)
 		if err := unit.Refresh(); err != nil {
@@ -74,7 +74,7 @@
 			return err
 		}
 		if status != last {
-			logger.Info("Unit status is %q: %s", status, info)
+			logger.Infof("Unit status is %q: %s", status, info)
 			last = status
 		}
 	}
@@ -82,7 +82,7 @@
 	if !ok {
 		return fmt.Errorf("cannot retrieve files: build unit lacks a public-address")
 	}
-	logger.Info("Built files published at http://%s", addr)
-	logger.Info("Remember to destroy the environment when you're done...")
+	logger.Infof("Built files published at http://%s", addr)
+	logger.Infof("Remember to destroy the environment when you're done...")
 	return nil
 }

=== modified file 'cmd/juju/main_test.go'
--- cmd/juju/main_test.go	2013-06-04 21:43:25 +0000
+++ cmd/juju/main_test.go	2013-06-10 04:33:28 +0000
@@ -73,7 +73,7 @@
 }
 
 func (s *MainSuite) TestTearDown(c *C) {
-	loggo.ResetLogging()
+	loggo.ResetLoggers()
 }
 
 func (s *MainSuite) TestRunMain(c *C) {

=== modified file 'cmd/logging.go'
--- cmd/logging.go	2013-06-07 01:00:23 +0000
+++ cmd/logging.go	2013-06-10 04:33:28 +0000
@@ -57,9 +57,9 @@
 		if l.Debug {
 			level = loggo.DEBUG
 		}
-		logger := loggo.GetLogger("juju")
-		logger.SetLogLevel(level)
+		// Set the level on the root logger.
+		loggo.GetLogger("").SetLogLevel(level)
 	}
-	loggo.ConfigureLogging(l.Config)
+	loggo.ConfigureLoggers(l.Config)
 	return nil
 }

=== modified file 'cmd/logging_test.go'
--- cmd/logging_test.go	2013-06-06 03:55:22 +0000
+++ cmd/logging_test.go	2013-06-10 04:33:28 +0000
@@ -46,8 +46,7 @@
 	err := l.Start(ctx)
 	c.Assert(err, IsNil)
 
-	logger := loggo.GetLogger("juju")
-	c.Assert(logger.GetLogLevel(), Equals, loggo.INFO)
+	c.Assert(loggo.GetLogger("").LogLevel(), Equals, loggo.INFO)
 }
 
 func (s *LogSuite) TestDebugSetsLogLevel(c *C) {
@@ -56,8 +55,7 @@
 	err := l.Start(ctx)
 	c.Assert(err, IsNil)
 
-	logger := loggo.GetLogger("juju")
-	c.Assert(logger.GetLogLevel(), Equals, loggo.DEBUG)
+	c.Assert(loggo.GetLogger("").LogLevel(), Equals, loggo.DEBUG)
 }
 
 func (s *LogSuite) TestStderr(c *C) {

=== modified file 'log/log.go'
--- log/log.go	2013-05-30 06:54:40 +0000
+++ log/log.go	2013-06-10 04:33:28 +0000
@@ -13,31 +13,31 @@
 
 // Errorf logs a message using the ERROR priority.
 func Errorf(format string, a ...interface{}) error {
-	logger.Log(loggo.ERROR, format, a...)
+	logger.Logf(loggo.ERROR, format, a...)
 	return nil
 }
 
 // Warningf logs a message using the WARNING priority.
 func Warningf(format string, a ...interface{}) error {
-	logger.Log(loggo.WARNING, format, a...)
+	logger.Logf(loggo.WARNING, format, a...)
 	return nil
 }
 
 // Noticef logs a message using the NOTICE priority.
 // Notice doesn't really convert to the loggo priorities...
 func Noticef(format string, a ...interface{}) error {
-	logger.Log(loggo.INFO, format, a...)
+	logger.Logf(loggo.INFO, format, a...)
 	return nil
 }
 
 // Infof logs a message using the INFO priority.
 func Infof(format string, a ...interface{}) error {
-	logger.Log(loggo.INFO, format, a...)
+	logger.Logf(loggo.INFO, format, a...)
 	return nil
 }
 
 // Debugf logs a message using the DEBUG priority.
 func Debugf(format string, a ...interface{}) (err error) {
-	logger.Log(loggo.DEBUG, format, a...)
+	logger.Logf(loggo.DEBUG, format, a...)
 	return nil
 }

=== modified file 'testing/log.go'
--- testing/log.go	2013-05-31 01:40:31 +0000
+++ testing/log.go	2013-06-10 04:33:28 +0000
@@ -32,11 +32,11 @@
 func (t *LoggingSuite) SetUpTest(c *C) {
 	loggo.ResetWriters()
 	loggo.ReplaceDefaultWriter(&gocheckWriter{c})
-	loggo.ResetLogging()
+	loggo.ResetLoggers()
 	loggo.GetLogger("juju").SetLogLevel(loggo.DEBUG)
 }
 
 func (t *LoggingSuite) TearDownTest(c *C) {
-	loggo.ResetLogging()
+	loggo.ResetLoggers()
 	loggo.ResetWriters()
 }

=== modified file 'worker/provisioner/provisioner.go'
--- worker/provisioner/provisioner.go	2013-06-10 02:52:05 +0000
+++ worker/provisioner/provisioner.go	2013-06-10 04:33:28 +0000
@@ -95,14 +95,14 @@
 			return tomb.ErrDying
 		case <-environmentProvisioner.Dying():
 			err := environmentProvisioner.Err()
-			logger.Error("environment provisioner died: %v", err)
+			logger.Errorf("environment provisioner died: %v", err)
 			return err
 		case cfg, ok := <-environWatcher.Changes():
 			if !ok {
 				return watcher.MustErr(environWatcher)
 			}
 			if err := p.setConfig(cfg); err != nil {
-				logger.Error("loaded invalid environment configuration: %v", err)
+				logger.Errorf("loaded invalid environment configuration: %v", err)
 			}
 		}
 	}

=== modified file 'worker/provisioner/provisioner_task.go'
--- worker/provisioner/provisioner_task.go	2013-06-10 02:51:28 +0000
+++ worker/provisioner/provisioner_task.go	2013-06-10 04:33:28 +0000
@@ -97,7 +97,7 @@
 }
 
 func (task *provisionerTask) loop() error {
-	logger.Info("Starting up provisioner task %s", task.machineId)
+	logger.Infof("Starting up provisioner task %s", task.machineId)
 	defer watcher.Stop(task.machineWatcher, &task.tomb)
 
 	// When the watcher is started, it will have the initial changes be all
@@ -106,7 +106,7 @@
 	for {
 		select {
 		case <-task.tomb.Dying():
-			logger.Info("Shutting down provisioner task %s", task.machineId)
+			logger.Infof("Shutting down provisioner task %s", task.machineId)
 			return tomb.ErrDying
 		case ids, ok := <-task.machineWatcher.Changes():
 			if !ok {
@@ -115,7 +115,7 @@
 			// TODO(dfc; lp:1042717) fire process machines periodically to shut down unknown
 			// instances.
 			if err := task.processMachines(ids); err != nil {
-				logger.Error("Process machines failed: %v", err)
+				logger.Errorf("Process machines failed: %v", err)
 				return err
 			}
 		}
@@ -124,7 +124,7 @@
 }
 
 func (task *provisionerTask) processMachines(ids []string) error {
-	logger.Trace("processMachines(%v)", ids)
+	logger.Tracef("processMachines(%v)", ids)
 	// Populate the tasks maps of current instances and machines.
 	err := task.populateMachineMaps(ids)
 	if err != nil {
@@ -163,7 +163,7 @@
 
 	instances, err := task.broker.AllInstances()
 	if err != nil {
-		logger.Error("failed to get all instances from broker: %v", err)
+		logger.Errorf("failed to get all instances from broker: %v", err)
 		return err
 	}
 	for _, i := range instances {
@@ -177,12 +177,12 @@
 		machine, err := task.machineGetter.Machine(id)
 		switch {
 		case errors.IsNotFoundError(err):
-			logger.Debug("machine %q not found in state", id)
+			logger.Debugf("machine %q not found in state", id)
 			delete(task.machines, id)
 		case err == nil:
 			task.machines[id] = machine
 		default:
-			logger.Error("failed to get machine: %v", err)
+			logger.Errorf("failed to get machine: %v", err)
 		}
 	}
 	return nil
@@ -194,7 +194,7 @@
 	for _, id := range ids {
 		machine, found := task.machines[id]
 		if !found {
-			logger.Info("machine %q not found", id)
+			logger.Infof("machine %q not found", id)
 			continue
 		}
 		switch machine.Life() {
@@ -202,17 +202,17 @@
 			if _, ok := machine.InstanceId(); ok {
 				continue
 			}
-			logger.Info("killing dying, unprovisioned machine %q", machine)
+			logger.Infof("killing dying, unprovisioned machine %q", machine)
 			if err := machine.EnsureDead(); err != nil {
-				logger.Error("failed to ensure machine dead %q: %v", machine, err)
+				logger.Errorf("failed to ensure machine dead %q: %v", machine, err)
 				return nil, nil, err
 			}
 			fallthrough
 		case state.Dead:
 			dead = append(dead, machine)
-			logger.Info("removing dead machine %q", machine)
+			logger.Infof("removing dead machine %q", machine)
 			if err := machine.Remove(); err != nil {
-				logger.Error("failed to remove dead machine %q", machine)
+				logger.Errorf("failed to remove dead machine %q", machine)
 				return nil, nil, err
 			}
 			// now remove it from the machines map
@@ -222,16 +222,16 @@
 		if instId, hasInstId := machine.InstanceId(); !hasInstId {
 			status, _, err := machine.Status()
 			if err != nil {
-				logger.Info("cannot get machine %q status: %v", machine, err)
+				logger.Infof("cannot get machine %q status: %v", machine, err)
 				continue
 			}
 			if status == params.StatusPending {
 				pending = append(pending, machine)
-				logger.Info("found machine %q pending provisioning", machine)
+				logger.Infof("found machine %q pending provisioning", machine)
 				continue
 			}
 		} else {
-			logger.Info("machine %v already started as instance %q", machine, instId)
+			logger.Infof("machine %v already started as instance %q", machine, instId)
 		}
 	}
 	return
@@ -254,7 +254,7 @@
 	for _, i := range instances {
 		unknown = append(unknown, i)
 	}
-	logger.Trace("unknown: %v", unknown)
+	logger.Tracef("unknown: %v", unknown)
 	return unknown, nil
 }
 
@@ -282,9 +282,9 @@
 	if len(instances) == 0 {
 		return nil
 	}
-	logger.Debug("Stopping instances: %v", instances)
+	logger.Debugf("Stopping instances: %v", instances)
 	if err := task.broker.StopInstances(instances); err != nil {
-		logger.Error("broker failed to stop instances: %v", err)
+		logger.Errorf("broker failed to stop instances: %v", err)
 		return err
 	}
 	return nil
@@ -306,7 +306,7 @@
 	// state.Info as the PA.
 	stateInfo, apiInfo, err := task.setupAuthentication(machine)
 	if err != nil {
-		logger.Error("failed to setup authentication: %v", err)
+		logger.Errorf("failed to setup authentication: %v", err)
 		return err
 	}
 	cons, err := machine.Constraints()
@@ -327,10 +327,10 @@
 		// Set the state to error, so the machine will be skipped next
 		// time until the error is resolved, but don't return an
 		// error; just keep going with the other machines.
-		logger.Error("cannot start instance for machine %q: %v", machine, err)
+		logger.Errorf("cannot start instance for machine %q: %v", machine, err)
 		if err1 := machine.SetStatus(params.StatusError, err.Error()); err1 != nil {
 			// Something is wrong with this machine, better report it back.
-			logger.Error("cannot set error status for machine %q: %v", machine, err1)
+			logger.Errorf("cannot set error status for machine %q: %v", machine, err1)
 			return err1
 		}
 		return nil
@@ -350,7 +350,7 @@
 		// encounter surprising problems.
 		return err
 	}
-	logger.Info("started machine %s as instance %s", machine, inst.Id())
+	logger.Infof("started machine %s as instance %s", machine, inst.Id())
 	return nil
 }
 

=== modified file 'worker/uniter/jujuc/juju-log_test.go'
--- worker/uniter/jujuc/juju-log_test.go	2013-05-30 06:54:40 +0000
+++ worker/uniter/jujuc/juju-log_test.go	2013-06-10 04:33:28 +0000
@@ -28,7 +28,7 @@
 }
 
 func assertLogs(c *C, ctx jujuc.Context, badge string) {
-	loggo.ConfigureLogging("juju=DEBUG")
+	loggo.ConfigureLoggers("juju=DEBUG")
 	writer := &loggo.TestWriter{}
 	old_writer, err := loggo.ReplaceDefaultWriter(writer)
 	c.Assert(err, IsNil)