Add intel_pmu plugin #760
Add intel_pmu plugin #760
Conversation
|
Seems okay. Just needs some docs in the README and some tests. Are you able to add them? |
|
Thank you for the feedback! Just pushed some tests and docs to my branch (I believe this is still up to date so I did not rebase with master) |
manifests/plugin/intel_pmu.pp
Outdated
| # https://collectd.org/documentation/manpages/collectd.conf.5.shtml#plugin_intel_pmu | ||
| class collectd::plugin::intel_pmu ( | ||
| Enum['present', 'absent'] $ensure = 'present', | ||
| Optional[Boolean] $report_hardware_cache_events = undef, |
There was a problem hiding this comment.
What are the defaults from collected? Is it possible to default to True or False, instead of undef? This would be cleaner.
There was a problem hiding this comment.
According to the documentation, there are no defaults, therefore the defaults are an implementation detail, which is why I did not set any defaults. However, by inspecting the source, as of the latest version the default is that no events are selected at all.
Therefore I suggest to leave the boolean variables as false by default, to make this clear, and to force the behaviour of not selecting any PMU events by default.
Regarding parameters event_list and hardware_events, I believe it would be best to leave them undefined by default. According to the docs these are used together, and to define hardware_events one should also use event_list. Therefore I think it is more coherent to have them both undefined by default rather than to have event_list undefined but hardware_events as an empty list. I think that if hardware_events will not be used, it should not be defined and it should not end up in the resulting intel_pmu.conf.
One improvement that could be made to the current logic is to check that if hardware_events is defined, event_list should also be defined. I can submit a new commit with this.
manifests/plugin/intel_pmu.pp
Outdated
| Optional[Boolean] $report_kernel_pmu_events = undef, | ||
| Optional[Boolean] $report_software_events = undef, | ||
| Optional[String] $event_list = undef, | ||
| Optional[Array[String]] $hardware_events = undef, |
There was a problem hiding this comment.
Can the default be an empty array?
There was a problem hiding this comment.
See my comment above.
intel_pmu: require event_list to be defined if hardware_events is used
|
The latest commits set the boolean parameters to false by default and require event_list to be defined if hardware_events is used, as I described above. Also added a test for this case where it should fail. |
|
Thanks for the work @pllopis ! |
This PR adds support for the intel_pmu plugin.
It supports the parameters that are documented here: https://collectd.org/documentation/manpages/collectd.conf.5.shtml#plugin_intel_pmu
The implementation uses the standard pattern as seen in other plugins using modern puppet types (also seen in other plugins).
I'd be glad to get any feedback on this. Thank you!