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.

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

Register kubernetes field_format matcher and remove logger in Encode API #4888

Merged
merged 1 commit into from
Aug 15, 2017

Conversation

vjsamuel
Copy link
Contributor

field_format matcher was not registered. Fixing that.
There was a continue missing when an unknown matcherFunc was invoked, causing to get nil pointer exception.

Removing logger from the Encode API as the error is returned to the caller, lets leave it to the caller to decide if its an error or a graceful failure.

@elasticmachine
Copy link
Collaborator

Can one of the admins verify this patch?

Copy link
Contributor

@exekias exekias left a comment

Choose a reason for hiding this comment

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

LGTM, want to hear from @urso on the logp Err

logp.Err("Fail to format event (%v): %#v", err, event)
}
return serializedEvent, err
return e.Format.RunBytes(event)
Copy link
Contributor

Choose a reason for hiding this comment

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

@urso, any opinion on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what will happen when we use field_format matcher, is that whenever there is no such fields in the event, it would keep logging this error and the number of such errors can be massive. @urso @exekias

Copy link

Choose a reason for hiding this comment

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

The formatter first tries to extract the fields used in the format string from the event. And only then it applies the event to the format string. One can use default values in the format string.

On error the event will be dropped (even for filebeat). That is, we have to trade excessive logging vs. 'silently' dropping events. The error can be investigated easily. Just dropping without error might confuse users potentially thinking filebeat is super unstable, just cause the configuration is not matching the event.

I checked the outputs and all but redis do log the error as err/warning/critical (depends on output and sending guarantees). I'm ok with removing the log.Err here, but then the error must be logged in the redis output. At some point we need to collect and log potentially excessive logs in a more 'sane' manner.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@urso, i have logged it as per your request.

@exekias
Copy link
Contributor

exekias commented Aug 14, 2017

jenkins test it

@exekias exekias merged commit 38aae1e into elastic:master Aug 15, 2017
@exekias exekias added bug libbeat needs_backport PR is waiting to be backported to other branches. v6.0.0-beta2 labels Aug 15, 2017
@vjsamuel vjsamuel deleted the fix_kubernetes_processor branch August 15, 2017 15:38
exekias pushed a commit to exekias/beats that referenced this pull request Aug 16, 2017
@exekias exekias removed the needs_backport PR is waiting to be backported to other branches. label Aug 16, 2017
exekias pushed a commit to exekias/beats that referenced this pull request Aug 17, 2017
andrewkroh pushed a commit that referenced this pull request Aug 17, 2017
leweafan pushed a commit to leweafan/beats that referenced this pull request Apr 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants