Skip to content
This repository has been archived by the owner on Nov 8, 2022. It is now read-only.

Add detection of plugin child process log level #1736

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add detection of plugin child process log level #1736

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 21 additions & 5 deletions control/plugin/execution.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,10 +131,10 @@ func (e *ExecutablePlugin) Run(timeout time.Duration) (Response, error) {
respReceived = true
close(doneChan)
} else {
execLogger.WithFields(log.Fields{
logCapturedLine(execLogger.WithFields(log.Fields{
"plugin": e.name,
"io": "stdout",
}).Debug(stdOutScanner.Text())
}), stdOutScanner.Text())
}
}

Expand Down Expand Up @@ -202,10 +202,10 @@ func (e *ExecutablePlugin) captureStderr() {
go func() {
for {
for stdErrScanner.Scan() {
execLogger.
logCapturedLine(execLogger.
WithField("plugin", e.name).
WithField("io", "stderr").
Debug(stdErrScanner.Text())
WithField("io", "stderr"),
stdErrScanner.Text())
}

if errScanner := stdErrScanner.Err(); errScanner != nil {
Expand All @@ -228,3 +228,19 @@ func (e *ExecutablePlugin) captureStderr() {
}
}()
}

// logCapturedLine logs a captured stderr/stdout line from a plugin, after choosing the
// appropriate log level to use based on the line's contents.
func logCapturedLine(entry *log.Entry, text string) {
lc := strings.ToLower(text)
switch {
case strings.Contains(lc, "error"):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cce I think it is not a good idea to detect log level in this way.
For example, the message 'warn: cannot collect metric /xx/xx/xx/error_rate from xxx platform' will be an error instead of warning in your case.

Copy link
Contributor Author

@cce cce Oct 2, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right that your example line would be considered an error — I was looking for a quick solution to ensure that a plugin's error logs are not logged at "DEBUG" level. What do you think is the best approach? Could we assume logrus format and look for level=error?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cce There's a similar issue already opened.
#1186

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cce I think parsing "level=error" like strings approach is acceptable. Though it will not be perfect (may be confused if this phrase occurred in log message), but the probability is rather low and the cost is not too high.

entry.Error(text)
case strings.Contains(lc, "warn"):
entry.Warn(text)
case strings.Contains(lc, "level=info") || strings.Contains(lc, `"level":"info"`):
entry.Info(text) // be a little more choosy about matching the word "info"
default:
entry.Debug(text)
}
}