Check for removed settings on startup #4726
Conversation
urso
force-pushed
the
enh/warn-removed-cfg
branch
from
ddc45d7 to
87c6438
Compare
| ) | ||
|
|
||
| // Beta logs the usage of an beta feature. | ||
| func Beta(format string, v ...interface{}) { |
There was a problem hiding this comment.
Seems to me this part would be more belong into logp?
There was a problem hiding this comment.
I'm fine with both locations, but I'm also curious why it got moved.
There was a problem hiding this comment.
I introduced package cfgwarn to combine all configuration related warning messages in. Plus I find logp kind of messy right now.
libbeat/common/cfgwarn/removed.go
Outdated
| "github.com/joeshaw/multierror" | ||
| ) | ||
|
|
||
| func CheckRemoved5xSettings(cfg *common.Config, settings ...string) error { |
There was a problem hiding this comment.
Do we need the 5x in the name? looks liek this is a generic function.
There was a problem hiding this comment.
I named it 5x so we can remove all references to 5.x at once (Just delete the function and compiler will tell you all places you have to modify).
There was a problem hiding this comment.
Works for me for the moment. i think we can use such a method in a more generic way as this is no the only time we remove settings. Something like CheckRemoveSettings() and the one here with 5x is just a proxy for it.
There was a problem hiding this comment.
Yeah, it's not the only time we remove settings. But at some point in time I want to remove the messages (e.g. on 7.x release remove the 5.x checks). Being able to remove messages/warnings is the motivation for naming this 5x
| "metricbeat.modules.0.filters.0.include_fields='field1,field2'" | ||
| ]) | ||
| assert exit_code == 1 | ||
| assert self.log_contains("setting 'metricbeat.modules.0.filters'" |
urso
force-pushed
the
enh/warn-removed-cfg
branch
from
d363445 to
f056919
Compare
| # - include_fields: | ||
| # fields: ["stats"] | ||
| # fields: ["beat", "metricset", "redis.info.stats"] |
There was a problem hiding this comment.
The publisher pipeline right now applies processors after having constructed the complete event. The disadvantage is, when using include_fields, one must configure all event fields. Having to configure the full field name to keep is quite an advantage I think, as we also document the full field names in our reference.
Considering a change in the processors order, by applying the processors before adding beats fields like beat or metricset. For metricbeat this would require some refactoring such that each metricbeat has it's own beat.Client instance (right now all metricsets share one beat.Client by forwarding events to another go-routine). Then constant fields like metricset can be added by the publisher pipeline by configuring the ClientConfig.Fields = common.MapStr { "metricset": ... }.
There was a problem hiding this comment.
This is quite a breaking change i think. Meaning if someone use fields: ["abc"] before and keeps the config, it will now remove many more fields.
There was a problem hiding this comment.
Right. This is what I'm saying. That's why I'm considering the changes to metricbeat and processors order in the publisher. Maybe we also want to introduce some whitelisting (fields that can not be removed).
urso
force-pushed
the
enh/warn-removed-cfg
branch
3 times, most recently
from
c2f2654 to
08ebd20
Compare
- introduce cfgwarn package:
- use Beta, Experimental, Deprecated from cfgwarn package
- use cfgwarn.CheckRemoved5xSettings for all settings removed by
publisher pipeline refactoring
- introduce system tests checking for removed settings being correctly
reported on startup
urso
force-pushed
the
enh/warn-removed-cfg
branch
from
08ebd20 to
ad0a3d2
Compare
logptocfgwarn