Change config reloading options in the short config #6883
Conversation
| @@ -14,11 +14,11 @@ data: | |||
| # Mounted `filebeat-prospectors` configmap: | |||
| path: ${path.config}/prospectors.d/*.yml | |||
| # Reload prospectors configs as they change: | |||
| reload.enabled: false | |||
| reload.enabled: true | |||
There was a problem hiding this comment.
@exekias Is this is a breaking change from your perspective? If yes, will remove it from the PR.
There was a problem hiding this comment.
I would say it is. +1 to keep it to false by default, rest of the PR LGTM
There was a problem hiding this comment.
Sorry, I realize now this is the configmap 🤦♂️ , I'm ok with having this enabled by default
| @@ -14,7 +14,7 @@ metricbeat.config.modules: | |||
| path: ${path.config}/modules.d/*.yml | |||
|
|
|||
| # Set to true to enable config reloading | |||
| reload.enabled: false | |||
| reload.enabled: true | |||
There was a problem hiding this comment.
This is a breaking change too, I'm wondering if we should recommend this mode by default or not. It may make things less predictable, wdyt?
There was a problem hiding this comment.
+1 for not enabling reload by default. If I were a user who is getting started and editing the configuration just to see what a Beat can do, I would not like it to restart without my explicit permission.
There was a problem hiding this comment.
Let's separate two discussions here:
-
Breaking change: I would argue it is not a breaking change as any user that keeps his config file from previous 6.x release will have the exact same behaviour. The behaviour only changes if someone uses the new "example" config.
-
On by default: It's valid point that it could be less predictable. The part I like most about having it on by default is that things like
metricbeat modules enable *work out of the box without the user having to restart the Beat which I think is really powerful to get users started with Beats modules.
I will open a separate PR with making the reload GA so we can continue the discussion about the defaults. As I don't see it as a breaking change, it could still happen in any other 6.x release.
There was a problem hiding this comment.
@exekias Now that we have the making it GA out of the way, lets continue the discussion here. Any thoughts to the above?
There was a problem hiding this comment.
Good point about the breaking change, I guess it's not. I still don't like to have it enabled by default, I don't know many (or any) projects doing live changes without the user requesting them, it's probably unexpected.
There was a problem hiding this comment.
Ok, will close the PR then :-D
This PR makes config reloading GA. A follow up PR with potential config changes is in elastic#6883.
|
Here is the PR to only make config reloading GA without config changes: #6891 Let's get this in and continue the discussion here about the defaults. |
This PR makes config reloading GA. A follow up PR with potential config changes is in #6883.
Config reloading has gone through several release cycles and no major issues have shown up. We should make reloading GA. There are several changes in this PR: * Config reloading is made GA but in the code it's disabled by default as otherwise this would be a breaking change * Enabling config reloading in our short config. This will make it easier for new users to use commands like `metricbeat modules enable system` as they will work out of the box. * Change reloading to true in K8s files. Not sure if we treat this as a breaking change?
|
Closing as we decided not to change the defaults in the short config as it could be an unexpected behaviour. |
Config reloading has gone through several release cycles and no major issues have shown up. We should make reloading GA.
There are several changes in this PR:
metricbeat modules enable systemas they will work out of the box.