-
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
Update metricbeat to use new publisher API #4699
Conversation
c43890c
to
bf27d72
Compare
afc6c2b
to
c8bda89
Compare
metricbeat/mb/builders.go
Outdated
func NewModules(config []*common.Config, r *Register) (map[Module][]MetricSet, error) { | ||
if config == nil || len(config) == 0 { | ||
return nil, ErrEmptyConfig | ||
// returns a the Modules and its configured MetricSets or an error. |
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.
s/a the Modules/a Module/
- It should be singular in this context.
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.
argh... copy'n paste
metricbeat/mb/builders.go
Outdated
// returns a the Modules and its configured MetricSets or an error. | ||
func NewModule(config *common.Config, r *Register) (Module, []MetricSet, error) { | ||
if !config.Enabled() { | ||
return nil, nil, nil |
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.
WDYT about returning an error in this case? Without an error callers always need to check if Module is non-nil.
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.
Yep, that's on purpose, but an value would make sense.
Module string `config:"module" validate:"required"` | ||
MetricSets []string `config:"metricsets" validate:"required"` | ||
Enabled bool `config:"enabled"` | ||
Filters processors.PluginConfig `config:"filters"` |
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.
So filters
is now processors
? I think it would be beneficial to users to Validate()
that filters
is not set. If it is set then include in the error that they should be using processors
.
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.
We'll need documentation updates too for this since filters
is gone? If so please drop a note into #4540.
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.
See: #4598 . All this is already on the todo list.
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.
Great, I didn't see that.
metricbeat/mb/module/connector.go
Outdated
"github.com/elastic/beats/libbeat/publisher/beat" | ||
) | ||
|
||
// Connector configures an establishes a beat.Client for publishing events |
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.
s/an/and/
?
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.
yep
metricbeat/mb/module/event.go
Outdated
event = common.MapStr{} // TODO (akroh): do we want to send an empty event field? | ||
fields := b.Event | ||
if fields == nil { | ||
fields = common.MapStr{} // TODO (akroh): do we want to send an empty event field? |
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.
Could you please remove this comment. This seems to be well established behavior at this point.
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.
k
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.
LGTM, WFG.
filters
toprocessors
(consistent with other beats)