use knapsack and slogger in kolide service #1436
Conversation
| ) | ||
|
|
||
| type Middleware func(KolideService) KolideService | ||
|
|
||
| func LoggingMiddleware(logger log.Logger) Middleware { | ||
| func LoggingMiddleware(k types.Knapsack) Middleware { |
There was a problem hiding this comment.
Could just pass slogger, but I think this is fine too.
pkg/service/publish_results.go
Outdated
| logger.Log( | ||
| "method", "PublishResults", | ||
|
|
||
| mw.knapsack.Slogger().Log(ctx, levelForError(err), "publish results", |
There was a problem hiding this comment.
This is not quite right. -- the first argument should be the message from line 189. The method should remain a KV pair. (I think true for all of these)
There was a problem hiding this comment.
Okay, I've added the message val as the msg for the logs.
Should we drop the kvp with the message key (now it would be msg) ... I don't think anything should rely on log output, but I have an urge to leave it in to maintain parity.
There was a problem hiding this comment.
I think there's no value in duplicating the message KV. It's fine that it changed from message to msg. (I just don't want to have method become msg which is semantically different)
eg: You should strike message.
…ng and implies you need to set the new multislogger
| } | ||
| logger.Log( | ||
|
|
||
| mw.knapsack.Slogger().Log(ctx, levelForError(err), message, |
There was a problem hiding this comment.
For this one and PublishResults, it looks like the message is empty (at least in the successful case) -- do we want to do "publish logs" and "publish results" for the log message instead, like how you've got it for e.g. RequestConfig?
There was a problem hiding this comment.
I'm not quite sure what is correct. Possibly using "publish logs" when message is empty. see previous conversation on subject #1436 (comment)
There was a problem hiding this comment.
So these cases are a successful completion of the thing, right? And we have method there. So maybe just "success"?
There was a problem hiding this comment.
I'm fine with success or publish logs!
There was a problem hiding this comment.
The case on "publish logs" feels wrong. Could be "published logs" or even "published"
Consider something like scoping the view down to just showing the publish logs method.
There was a problem hiding this comment.
I've updated all the publish methods to either have success or* failure as the message
James-Pickett
dismissed stale reviews from RebeccaMahany and zackattack01
via
96956dc
No description provided.