-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Refactor filebeat connector #12997
Refactor filebeat connector #12997
Conversation
259470a
to
f1c5eb4
Compare
84a9af8
to
4af826c
Compare
Jenkins, test this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work as always @urso, I like that we remove the util.Data
It always look a bit weird before, I think its a safe refactoring with the number of tests we have.
setOptional(meta, "pipeline", config.Pipeline) | ||
setOptional(fields, "fileset.name", config.Fileset) | ||
setOptional(fields, "service.type", serviceType) | ||
setOptional(fields, "input.type", config.Type) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: What if we centralize this operation in a single method?
meta, fields, err := withOptional(clients.Processing)
Note that the err
is not mandatory but could be used if values should not be changed or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it's worth it to have a central method that change the default values of the meta or fields in an event. I am just trying to put myself in the following mind: Where would I look for default added fields or meta?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I considered this, but not only for fields and meta. This function basically merges some default ClientConfig with the one provided by the input. So I was wondering how generic this merge should be. But yet the order might be somewhat filebeat specific, so I decided to postpone it if there is need.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am OK to postpone it, it might be premature to extract it.
mode := clientCfg.PublishMode | ||
if mode == beat.DefaultGuarantees { | ||
mode = beat.GuaranteedSend | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worth to log it in debug? it would be explicit but might scare people. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't log it here. If we log it, then we should also log the input type and maybe some config id. This information is lost here. So far GuaranteedSend is the default for everyone and no input allows users to change this.
If an input supports multiple modes in the future, then it should log the mode being used.
The filebeat channel package wraps the publisher pipeline, providing support for preparing custom user settings. The drawback is, that inputs can not configure custom fields, processors, and ACK handlers. The later is required for implementing end-to-end ACK. This refactoring modifies the channel.Factory and channel.Connector types, such that input authors are able to make use of all the capabilities of the publisher pipeline. Inputs are free to configure the PublishMode. If not set, then filebeat defaults to guaranteed sends. The channel.Outleter should become more similar to beat.Client in the future, and will be eventually removed. A side effect of moving closer to beat.Client is the removal of the filebeat/util package. The complex shutdown handling in filebeat still prevents us from removing channel.Outlet. The complex shutdown handling is still required for handling the interactions between the logs input and the registrar on shutdown.
0966542
to
1a908e8
Compare
Requires: #12996
Refactor filebeat/channel package
The filebeat channel package wraps the publisher pipeline,
providing support for preparing custom user settings. The drawback is,
that inputs can not configure custom fields, processors, and ACK
handlers. The later is required for implementing end-to-end ACK.
This refactoring modifies the channel.Factory and channel.Connector
types, such that input authors are able to make use of all the
capabilities of the publisher pipeline.
Inputs are free to configure the PublishMode. If not set, then filebeat
defaults to guaranteed sends.
The channel.Outleter should become more similar to beat.Client in the
future, and will be eventually removed. A side effect of moving closer
to beat.Client is the removal of the filebeat/util package.
The complex shutdown handling in filebeat still prevents us from
removing channel.Outlet. The complex shutdown handling is still required
for handling the interactions between the logs input and the registrar
on shutdown.