Fix log adapter in Prometheus receiver #1493
Conversation
nilebox
requested review from
bogdandrutu,
dmitryax,
james-bebbington,
owais,
pjanotti and
tigrannajaryan
as code owners
Codecov Report
@@ Coverage Diff @@
## master #1493 +/- ##
==========================================
+ Coverage 90.98% 91.00% +0.02%
==========================================
Files 239 239
Lines 16744 16793 +49
==========================================
+ Hits 15235 15283 +48
- Misses 1080 1083 +3
+ Partials 429 427 -2
Continue to review full report at Codecov.
|
| // a corresponding zap log level | ||
| func matchLogLevel(key interface{}, val interface{}) (*zapcore.Level, bool) { | ||
| strKey, ok := key.(string) | ||
| if !ok || strKey != "level" { |
| } else { | ||
| // in case something goes wrong | ||
| w.l.Info(keyvals...) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| // extract go-kit log level from key value pairs, convert it to zap log level | ||
| // and remove from the list to avoid duplication in log message |
There was a problem hiding this comment.
Is it idiomatic to start the comment with the function name even for non-exported functions? I've tended to do that anyway, but maybe it's not important
| func toZapLevel(value level.Value) zapcore.Level { | ||
| // See https://github.com/go-kit/kit/blob/556100560949062d23fe05d9fda4ce173c30c59f/log/level/level.go#L184-L187 | ||
| switch value.String() { | ||
| case "error": |
There was a problem hiding this comment.
Make these all consts. Also consider just returning the zap logging functions here (could make this a type) so you don't need the levelToFunc function below
| case zapcore.ErrorLevel: | ||
| return logger.Errorf, nil | ||
| } | ||
| return nil, fmt.Errorf("unrecognized level: %q", lvl) |
There was a problem hiding this comment.
I think it'd be more consistent with the other code if this just returned Infof
| "level", | ||
| "warn", // Field is preserved | ||
| }, | ||
| }, |
There was a problem hiding this comment.
It would be better to test the Log function directly imo. You could hook into zap logs and test what is actually logged. That would also get your coverage up 😄
There was a problem hiding this comment.
@nilebox maybe consider this in a followup PR
There was a problem hiding this comment.
I have already added a test with zap hooks actually:
I kept the original tests as well, because zap hook entries don't have custom fields so I can't assert them there unfortunately.
| "level", | ||
| "warn", // Field is preserved | ||
| }, | ||
| }, |
There was a problem hiding this comment.
@nilebox maybe consider this in a followup PR
Description:
Fixing a bug:
InfoLink to tracking Issue: Fixes #1486
Testing: Added unit tests
Documentation: -