improve error message #2937
improve error message #2937
Conversation
| @@ -60,7 +60,7 @@ func (c *Collection) logError(key Key, err error) { | |||
| errCount := atomic.AddInt64(&c.errCount, 1) | |||
| if errCount%errCountLogThreshold == 0 { | |||
| // log only every 'x' errors to reduce mem allocs and to avoid log noise | |||
| c.logger.Debug("Failed to fetch key from dynamic config", tag.Key(key.String()), tag.Error(err)) | |||
| c.logger.Debug("No such key in dynamic config, using default", tag.Key(key.String()), tag.Error(err)) | |||
There was a problem hiding this comment.
Key not found might be the case of what you see, but the code did not guarantee that it is only case. Any error while trying to fetch the key will end up here. For example if lookup for a key of value int and the value cannot be convert to a int will also end up this log line.
There was a problem hiding this comment.
Shouldn't we distinguish those cases and log them differently? Those shouldn't be "Debug" level logs, they represent significant coding errors, no?
There was a problem hiding this comment.
We could, the key not found is represent as sentinel error errKeyNotPresent. It might not be coding errors, it could be wrong dynamic config value, but agree that also worth a higher level of logging.
Another sentinel error is errNoMatchingConstraint which is also not a real error and should be debug level.
So if we want do some change here, we could check if err == errKeyNotPresent || err == errNoMatchingConstraint, do debug log, otherwise it should be a warning log.
There was a problem hiding this comment.
If we do so, the 2 sentinel error probably should be exported because other dynamic config client implementations will need to return the same error for same cases.
There was a problem hiding this comment.
do you not agree that (considering that the log message is only Debug anyway) that the change to the wording is still an improvement?
I agree with but don't have time to undertake the work you're proposing.
Note that I've added another file to the PR
improved error message
Because the existing message looks like an Error, but is really only a Debug "comment" and occurs with correct code.
N/A
N/A
NO