Bug 1245063 - Add logrus logging package to be used by runtime/plugins/engines

[gregarndt]

The scope of this has changed a bit but I preserved commits just in case we want to take something that ended up getting deleted.

This implementation makes use of logrus, which is used by popular projects such as Docker. It has an easy to use interface with multiple formatters depending on if you're running this in a terminal, writing somewhere else, etc.

[gregarndt]

Fixing up some stuff, adding a mutex, docs, etc

[jonasfj]

I'm curious have you considered doing something like logrus.... ie. declare a Tags type like they have Fields... and accept a ...interface{} as message (var args are nice)...
Using something like WithFields is also nice, as we might not always have fields...

Honestly, I'm not a fan of doing our own logger, just using an existing library to have loggers available in various context where they are useful.

---

Note: I'm not opposed to using logrus, I haven't really looked at it... but it seems to support child loggers...

Ie. it would be possible to do something like:

log := logrus.New();

log.WithFields(Fields{"tag": "value"}).Debug("Hello", "world", 5)
log.Debug("Hello", "world", 5)

// Create task log for TaskContext (this is for system messages, not the livelog that is uploaded to queue)
taskLog := log.WithFields(Fields{"taskId": "...", "runId": 1})
taskLog.WithFields(Fields{"pulling": "img", "time": 100}).Debug("Pulling an image: ", img)
taskLog.Debug("Pulled an image: ", img)

If we think that logrus exposes too many methods.. And we want to do some hiding...
We can just define an interface, containing only the methods we want plugins and engines to use.
For example I see no need to expose log.Reader()...

[jonasfj]

Proposed interface to hide logrus behind:

https://public.etherpad-mozilla.org/p/logrus-simplified

[gregarndt]

Had a discussion with Jonas on IRC and he finally came around to my idea of using logrus it seems :) It looks like we reallyw ould be reinventing the wheel with some niceties we want and what logrus already offers. It's used by the docker projects and I suspect some other large ones out ther as well so it won't be going away anytime soon. Also as shown in this PR, it's trivial to add the basic functionality we're looking for.

[gregarndt]

Logging has been moved to logrus and a logger interface creates so engines/plugins can just reference the interface even if the underlying logging package changes (as long as the new logging package maintains that interface).

[jonasfj]

I propose we make this a non-pointer type. See Note in the definition of EngineOptions.

Basically, I think when we have <...>Options where the struct is only used as input parameters for a method, then it makes sense to pass it by value.
It when we do struct grouping just because grouping the input parameters into a struct allows us to add new parameters without breaking old code.

I see this <...>Options pattern as passing key-word arguments in python. Ie. you'll always call EngineProvider as:

engine, err := engineProvider(EngineOptions{
  Environment: ...
  ....
})

So it's just a pattern used to pass arguments, not an actual struct that we recommend people pass around as a value. Not a struct that will have have methods or anything like that.

---

I think doing so keeps the pattern and usage of these <...>Options structs very simple.

[jonasfj]

@petemoore, I haven't coded much go, maybe I'm wrong to suggest this pattern... It just seems like a good way to solve the problem of adding additional input paramters... Without introducing something that is inherently complicated.

Note: This should all be documented in comments inside EngineOptions and similar options structs.

[gregarndt]

Ok, I can change it back to not passing around a pointer. Changing between a pointer and a value didn't change the behavior we're trying to accomplish though....passing around a struct that can have fields that change without needing to change the method signature of the receivers of it.

[jonasfj]

Start docs comments like this with CreateLogger .... I think go fmt or one of its friends keeps telling me so...

[petemoore]

go vet does that (which is currently in the make test target)

[gregarndt]

is there something I should enable for it to warn me about things like this? 'make test' nor running 'go vet' directly has indicating any problems.

[gregarndt]

Good call on changing this documentation by the way, still getting used to how things should be documented.

[petemoore]

My bad, I think it is https://github.com/golang/lint

[jonasfj]

I like this r+ for sure (we should have merged this last week)... Comments are minor can be ignored... We should just move forward. Feel free to fix any minor comments you agree with...