Refactor logging and expose ability to toggle level in running process #195
Conversation
service/server_grpc.go
Outdated
| @@ -26,7 +26,6 @@ func parseUUID() grpctransport.ServerOption { | |||
|
|
|||
| func NewGRPCServer(endpoints KolideClient, logger log.Logger) pb.ApiServer { | |||
| options := []grpctransport.ServerOption{ | |||
| grpctransport.ServerErrorLogger(logger), | |||
There was a problem hiding this comment.
Note I killed this because the docs say
ServerErrorLogger is used to log non-terminal errors. By default, no errors are logged.
and I think we should leave it at the default.
There was a problem hiding this comment.
I'm not sure I agree. This method is for server side implementations so it would not affect the launcher client. Is there a reason we wouldn't want to log if a grpc error happened (unable to decode, request failed etc)
IMO if you're removing it, the NewGRPCServer should accept a list of grpctransport.ServerOption because there's no way to add a logger if it's needed.
log/log.go
Outdated
| } | ||
|
|
||
| func NewLogger(w io.Writer) Logger { | ||
| base := log.NewJSONLogger(log.NewSyncWriter(w)) |
There was a problem hiding this comment.
I often prefer logfmt for reading logs locally. This hardcodes the output format too.
service/server_grpc.go
Outdated
| grpctransport "github.com/go-kit/kit/transport/grpc" | ||
| "github.com/kolide/launcher/log" |
There was a problem hiding this comment.
This is a breaking change for servers. since the log.Logger in this file has changed to a much larger interface, any NewGRPCServer will require to instantiate a new launcher/log.Logger rather than the go-kit one. I'm not sure that's a good change.
service/logging.go
Outdated
| @@ -25,7 +25,7 @@ type logmw struct { | |||
| func (mw logmw) RequestEnrollment(ctx context.Context, enrollSecret, hostIdentifier string) (errcode string, reauth bool, err error) { | |||
| defer func(begin time.Time) { | |||
| uuid, _ := uuid.FromContext(ctx) | |||
| mw.logger.Log( | |||
| mw.logger.Debug( | |||
There was a problem hiding this comment.
These logging middleware are also used by the server and should not be enforcing a level. The level was enforced when the logger is created.
See the pull request that removed Debug for more info #181
log/log.go
Outdated
| AllowInfo() | ||
| } | ||
|
|
||
| type logger struct { |
There was a problem hiding this comment.
Do you think it would be possible to scale this package back in this way?
- Export the
loggerstruct. - Return the struct.
- Don't declare an interface in this package.
- Only make any necessary changes in main, leaving other packages to only consume a go-kit logger?
The reason I'm asking is because the logger is now required by all the packages in this repo, including ones meant to be imported by server-side implementations.
I wouldn't be opposed to discussing/iterating on the proposed interface here, but I think it should be more carefully considered before it propagates.
There was a problem hiding this comment.
For example, for GCP servers I have created this helper https://github.com/kolide/kit/blob/566c8f56a6ff7daba204818fbab0f2cb854b3310/logutil/logutil.go#L22 to rewrite the level key. I'm not sure how that will work with this logger.
zwass
force-pushed
the
refactor_logging
branch
from
b47b2ca to
414d2b8
Compare
|
@groob I scoped this back in line with your comments and I feel better about it. Please let me know what you think. |
|
🚢 |
Based on the work done by @zwass in kolide/launcher#195
Based on the work done by @zwass in kolide/launcher#195
…er (#29) Based on the work done by @zwass in kolide/launcher#195
SIGUSR2.