Set default metricset in windows module to service
#6675
Conversation
jsoriano
force-pushed
the
windows-default-metricset
branch
2 times, most recently
from
4e719da to
ac04a7c
Compare
metricbeat/metricbeat.reference.yml
Outdated
| period: 10s | ||
| perfmon.counters: | ||
|
|
||
| - module: windows |
There was a problem hiding this comment.
Reference file should definitiveyl contain both options. Probably a config.reference.yml has to be added.
jsoriano
force-pushed
the
windows-default-metricset
branch
from
ac04a7c to
4c75863
Compare
|
Added reference config and updated doc. Regarding why chosing perfmon as default option I don't have a strong opinion on that, I only though that maybe by default the user may not want to monitor all services. |
| @@ -20,13 +21,8 @@ in <<configuration-metricbeat>>. Here is an example configuration: | |||
| ---- | |||
| metricbeat.modules: | |||
| - module: windows | |||
| metricsets: ["perfmon"] | |||
| period: 10s | |||
| perfmon.counters: | |||
There was a problem hiding this comment.
perfmon requires configuration so maybe it shouldn't be a "default". You must configure perfmon.counters in order for it to be useful so I think this example needs improvement.
The service metricset does not require any configuration so it might be a good default. But TBH the default period of 10s is a excessive. I'd probably increase it 60s at a minimum or drop events for healthy services.
There was a problem hiding this comment.
+1 on the proposal. As we have now also a config.reference.yml we can set the period to 60s (non default) in the config.yml. Also we can add a processor to drop the healthy events with a processor in the config.yml.
There was a problem hiding this comment.
Agree with the proposals, thanks @andrewkroh
Would it be ok to set a different default for the period in the initialization of this module?
There was a problem hiding this comment.
It would be possible but as the default period is possible, I would not do that and use the config.yml for it. This keeps the code and defaults simple, but still allows us to overwrite them.
There was a problem hiding this comment.
Oh, period is module-wide 🤔 it shouldn't be set from the metricset.
There was a problem hiding this comment.
You can define a module multiple times in the config if you want to have different periods for different metricsets.
jsoriano
force-pushed
the
windows-default-metricset
branch
from
4c75863 to
ab0f878
Compare
jsoriano
changed the title
perfmonservice
|
Changed default metricset to |
|
|
||
| - module: windows | ||
| metricsets: ["service"] | ||
| enabled: true | ||
| period: 60s |
There was a problem hiding this comment.
Now it becomes interesting. In the reference config we should use defaults and only in config.ymloverwrite the defaults. At least that is what we did so far.
There was a problem hiding this comment.
This is how periods were in the reference config before this PR :)
I think that supporting different default periods can make a lot of sense (this module is a good example), but this gives for a bigger discussion out of the scope of this PR, wdyt?
There was a problem hiding this comment.
That we had different periods in the reference config was more an accident as the values were taken from the short config. Now that we have a reference file we could use the default one.
There are a lot of interesting discussion popping up with our changes here. We can take these offline. Will merge as is for now.
jsoriano
force-pushed
the
windows-default-metricset
branch
from
ab0f878 to
fec1a94
Compare
See #6668