agent/mongo: log version of mongo

agent/mongo: log version of mongo

Just before we start mongo, log the version so we know what we're
running.  This is mostly just to aid us in debugging log files from
other people.

https://code.launchpad.net/~jameinel/juju-core/log-mongo-version/+merge/215656

(do not edit description out of merge proposal)

[jameinel, 2014-04-14 13:04:31]

Please take a look.

[natefinch, 2014-04-16 10:47:20]

lgtm, a couple minor suggestions.

https://codereview.appspot.com/87570043/diff/1/agent/mongo/mongo.go
File agent/mongo/mongo.go (right):

https://codereview.appspot.com/87570043/diff/1/agent/mongo/mongo.go#newcode255
agent/mongo/mongo.go:255: logger.Debugf("found mongod at: %s", mongopath)
maybe put this after the err check, just on the off chance we *don't* find mongo
there?  or change the message to "checking version of mongo at <path>?

https://codereview.appspot.com/87570043/diff/1/agent/mongo/mongo.go#newcode259
agent/mongo/mongo.go:259: logger.Infof("failed to read the output from %s
--version", mongopath)
should we log the error?  it could be "mongod not found"

[rog, 2014-04-16 11:35:51]

LGTM modulo the suggestions below.

https://codereview.appspot.com/87570043/diff/1/agent/mongo/mongo.go
File agent/mongo/mongo.go (right):

https://codereview.appspot.com/87570043/diff/1/agent/mongo/mongo.go#newcode259
agent/mongo/mongo.go:259: logger.Infof("failed to read the output from %s
--version", mongopath)
On 2014/04/16 10:47:20, nate.finch wrote:
> should we log the error?  it could be "mongod not found"

the error is often not very useful actually.
it may be that the command returns a non-zero exit
status but actually produced some useful output.

worth logging the output too (with %q), I think, when there is an error.

https://codereview.appspot.com/87570043/diff/1/agent/mongo/mongo.go#newcode262
agent/mongo/mongo.go:262: logger.Debugf("mongod --version:\n%s", string(output))
I think this should be an Infof.

Also, no need for the string conversion (%s formats []byte
correctly as a string)

Additionally I'm not that keen on log messages with embedded newlines.
How about stripping space and using %q? Then the log line will look like:

mongod --version: "db version v2.4.9\nWed Apr 16 12:25:32.774 git version:
nogitversion"

which looks pretty good to me and is easily greppable.

https://codereview.appspot.com/87570043/diff/1/agent/mongo/mongo.go#newcode281
agent/mongo/mongo.go:281: logMongoVersion(mongopath)
I'm not sure this is a great place for this log message.
It seems like we're putting the log message here as
a side effect in a function that is otherwise side-effect
free.

Two possibilities:
1) make mongoUpstartService return the version string
2) make mongoUpstartService return the mongod executable
path and call logMongoVersion from EnsureMongoServer.

I think I prefer the latter.

[jameinel, 2014-04-16 11:39:24]

Please take a look.

https://codereview.appspot.com/87570043/diff/1/agent/mongo/mongo.go
File agent/mongo/mongo.go (right):

https://codereview.appspot.com/87570043/diff/1/agent/mongo/mongo.go#newcode255
agent/mongo/mongo.go:255: logger.Debugf("found mongod at: %s", mongopath)
On 2014/04/16 10:47:20, nate.finch wrote:
> maybe put this after the err check, just on the off chance we *don't* find
mongo
> there?  or change the message to "checking version of mongo at <path>?

I originally had it as one message, but then I wanted to make sure to log
something on error. But I realize we'll have that data as long as we put the
path in the output, so I'll fix it to just at the end. (Nicer not to have lots
of lines anyway.

https://codereview.appspot.com/87570043/diff/1/agent/mongo/mongo.go#newcode259
agent/mongo/mongo.go:259: logger.Infof("failed to read the output from %s
--version", mongopath)
On 2014/04/16 10:47:20, nate.finch wrote:
> should we log the error?  it could be "mongod not found"

logged