Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Decouple debug logging behaviour from fail_on_error value #12451

Closed
wants to merge 4 commits into from
Closed

Decouple debug logging behaviour from fail_on_error value #12451

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8d72d45
Change logging behaviour with renameFields
AndyHunt66 Jun 5, 2019
862e4d1
Revert "Change logging behaviour with renameFields"
AndyHunt66 Jun 5, 2019
764a008
Separate logging behaviour from fail_on_error
AndyHunt66 Jun 6, 2019
4f59679
Decouple debug logging behaviour from fail_on_error value
AndyHunt66 Jun 7, 2019
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
10 changes: 6 additions & 4 deletions libbeat/processors/actions/rename.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,12 +76,14 @@ func (f *renameFields) Run(event *beat.Event) (*beat.Event, error) {

for _, field := range f.config.Fields {
err := f.renameField(field.From, field.To, event.Fields)
if err != nil && f.config.FailOnError {
if err != nil {
errMsg := fmt.Errorf("Failed to rename fields in processor: %s", err)
logp.Debug("rename", errMsg.Error())
Copy link
Contributor

Choose a reason for hiding this comment

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

Is keeping this debug log necessary when the same message is logged on error level?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Part of the wider problem is that the message is not "logged on error level".
This whole issue only arose because there was no "failure to rename" error thrown unless you were running at debug.

IMHO this Debug should actually be Error

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I saw the issue you have opened. With this change, an error is logged, as we agreed previously. I asked the question because I think it is unnecessary to log the same error on both Debug and Error level when fail_on_error is set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then I think I'm misunderstanding you completely - my apologies.
I think I misunderstood what you meant by logging.
There is no "Error level" logging to the log file going on at all - either before or after this fix.
There is an error message inserted into the event itself (and thus ES), fail_on_error is true

Assuming an error condition, with this fix, the following behaviour happens

fail_on_error Log level Message in log file Message in event/ES
true default No Yes
true debug Yes Yes
false default No No
false debug Yes No

Which, I think is what you are both @kvch and @ruflin asking for?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is exactly what I've asked for. Thank you for the clear explanation. :)

event.Fields = backup
event.PutValue("error.message", errMsg.Error())
return event, err
if f.config.FailOnError {
event.Fields = backup
event.PutValue("error.message", errMsg.Error())
return event, err
}
}
}

Expand Down