Decouple debug logging behaviour from fail_on_error value #12451
Conversation
libbeat/processors/actions/rename.go
Outdated
| @@ -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()) | |||
There was a problem hiding this comment.
Is keeping this debug log necessary when the same message is logged on error level?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
This is exactly what I've asked for. Thank you for the clear explanation. :)
|
@AndyHunt66 Thanks for your additional comment. I think I misread initially on what you wanted to do in this PR. Re-reading the PR (code) I know get the following:
Sorry for the initial confusion. Could you perhaps update the PR description to clarify this more (we only talk about debug log level changes) and also add, that you not only change the rename processor? Could you add a changelog entry? |
AndyHunt66
changed the title
|
@ruflin - Thanks - done : commit Edit: that didn't actually work out too well - apologies for the commit spaghetti Please advise on how you would like me to resolve this. The cleanest way that I can see is to simply create a new PR . |
|
Strangely above it shows "unkown repository" and is probably the reason the commit does not show up. Did you remove your clone, delete the branch or something like that? If we find an easy option to revive this PR, great (for push is an option from my perspective), if not, please open a new PR and reference this one there. |
Yes - exactly that. Sorry, my git skills are .... er... still growing. I've created PR #12496 - will close this. Apologies for the messing around |
When using the
rename,copy_fieldsortruncate_fieldsprocessors, and specifying multiple fields to be processedfail_on_erroris true .Logging of the error should be independent of the
fail_on_errorflag.