Add proposal for logging changes #381
Conversation
|
|
||
| var ( | ||
| ... | ||
| log = lgging.MustGetLogger("broker") |
| ### Work Items | ||
| We need to decide how we are going to change this. I have put together 3 options that I think are all pretty good options that significantly help our current logging woes. | ||
|
|
||
| 1. We can keep go-logging and use package level loggers. An example is the example go program [here](https://github.com/op/go-logging/blob/master/examples/example.go). Or an example that would look more like what we would do is select a file in each package and initialize the log. example changes to `broker.go` |
There was a problem hiding this comment.
I kind of like the package level logging. That's actually how I did it in the unit tests by accident :) This I think would be the least change. Add a logger to every package, then remove log from all structs. Done.
|
|
||
| func Error(args ...interface{}) {] | ||
| log.Error(args) | ||
| } |
There was a problem hiding this comment.
This would give us more control but not sure if it's worth the effort. I'd have to think about it.
There was a problem hiding this comment.
I don't think it is either, package level seems to fit our needs.
There was a problem hiding this comment.
The only use case that I can think of is if we want to pull out data from a context on every call to the logger. The use case is that we have a unique id for each request to the broker and would like to print that ID with each log statement or the principal from auth. That is a bigger change as we would need to start passing around a ctx and everything but is what I was thinking about when adding this option.
| } | ||
| ``` | ||
|
|
||
| 3. We could abandon go-logging and move to something more familiar to [kubernetes](https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/job/job_controller.go#L132) and use the [glog](https://github.com/golang/glog) package. |
There was a problem hiding this comment.
I guess we could :) I'd actually go package level first. if that still is annoying then we can look at change to glog as a second resort.
| 3. We could abandon go-logging and move to something more familiar to [kubernetes](https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/job/job_controller.go#L132) and use the [glog](https://github.com/golang/glog) package. | ||
|
|
||
|
|
||
| The next steps will be a combination of removing logging from all the structs, updating each log call to use the new log syntax. |
There was a problem hiding this comment.
Glog would actually be a bigger change than package level.
There was a problem hiding this comment.
I don't really see any value in switching to $NEWLOGGER unless it helps us solves our architectural problems for some reason, which is the real issue here.
|
|
||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
probably don't need all these blank lines at the end :)
|
@shawn-hurley @jmrodri So the motivation for passing around a logging reference was ostensibly to get dependency injection benefits around testing, and also to leave open the possibility that the If we were to choose opt 1, I don't see any issues with testing. WRT vendoring apb, I still see this as a problem, but the package itself also hasn't been maintained at all with this in mind, so I'm not sure we care? Food for thought: I love the way Rust handles this. The |
No description provided.