feat: Unify graphics SMI implementations #9815

srebhan · 2021-09-24T14:08:04Z

Required for all PRs:

Updated associated README.md.
Wrote appropriate unit tests.
Pull request title or commits are in conventional commit format (e.g. feat: or fix:)

Both SMI implementations, nvidia_smi as well as amd_rocm_smi, share a common code structure: Try first execute a binary that produces the data and then parse the data into metrics using a kind of mapping definition. The present PR unifies this common structure and merges both plugins into a new generic ReceiveAndParse plugin. This new plugin provides a common infrastructure allowing to split the processing into a transport which receives the raw data and a parser transforming this raw data to metrics. Additional post-processing capabilities are provided to allow further metric transformation impossible in the parser. The whole change by this PR is transparent to the user.

To generalize more, a common/transport package is introduced that abstracts the way of receiving the data. For now only an Exec receiver is provided sufficient to unify the two graphics plugins. However, in the future we might want to extend this to e.g. HTTP transports or similar.
For the parsing part, we uses telegraf's plugin power to instantiate a suitable parser. We configure this parser with a config file embedded into the telegraf binary.

Please note that more such common structures exist in telegraf e.g. for web-plugins that do a HTTP request and then parse the response (usually JSON) to metrics. Those plugins might also be unified in a similar way.

reimda · 2021-09-29T14:53:13Z

Hi Sven, I like how this PR lets the two plugins become configuration instead of code. (in NewAMDSMI() and NewNvidiaSMI())

What do you think about making the graphics_smi class more generic so it can be used by plugins that aren't gpu related, and then putting it in plugins/common? Then adm_rocm_smi and nvidia_smi would stay in their separate directories but use the common code.

I would prefer to keep telegraf's convention of each input plugin having its own directory in plugins/inputs, where users can find the readme.md, the plugin implementation and unit tests, and shared code in plugins/common.

telegraf-tiger · 2021-10-03T09:59:23Z

Looks like new artifacts were built from this PR. Get them here!

Artifact URLs

plugins/inputs/amd_rocm_smi/amd_rocm_smi_parser.conf

Hipska · 2021-10-11T09:52:25Z

List of plugins that might also benefit from such makeover.

…ralizes the usual 'get data from a data-source and parse the result' pattern.

…at is embedded into the binary.

…that is embedded into the binary.

plugins/common/receive_parse/receive_parse.go

reimda · 2021-10-14T21:22:35Z

plugins/inputs/amd_rocm_smi/amd_rocm_smi.go

+	if err := toml.Unmarshal(cfgfile, &cfg); err != nil {
+		panic(fmt.Errorf("cannot unmarshal 'amd_rocm_smi_parser.conf': %v", err))
 	}
-


I don't see the benefit to putting the config in a .toml file and calling Unmarshall at runtime. We don't do that for other configuration data in the codebase.

Would it be ok with you to move the toml data directly into go struct literals here in the go source code?

Sure I can do this by just reverting the last few (3 IIRC) commits.

Let's do revert those commits. It's clearer to put them in static structs in the code, and has the benefit of failing at compile time instead of runtime if there's a typo.

reimda · 2021-10-19T20:37:49Z

plugins/common/transport/transport.go

+type Receiver interface {
+	// Receive data from the endpoint(s)
+	Receive() ([]byte, error)
+	Transport
+}


Does this fit better in package receive_parse? Referring to it as transport.Receiver was confusing to me.

Hmm I was thinking of also adding other transports (http etc.) here for covering more items and maybe also add Sender later to cover output plugins.
So my view was that it is a property of a transport to be able to Receive (and later to also Send)... But if you prefer to have this interface definition in the generic plugin, I can do it. What is your decision? :-)

It seems better in receive_parse to me but I'm ok with it either place.

Co-authored-by: reimda <reimda@users.noreply.github.com>

telegraf-tiger · 2021-10-20T07:27:51Z

📦 Looks like new artifacts were built from this PR.

Expand this list to get them here! 🐯

Artifact URLs

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

Hipska · 2022-09-07T16:09:39Z

Can we finalise this one?

reimda · 2022-09-08T21:43:53Z

Can we finalise this one?

I'm happy to merge once we revert the toml.Unmarshal changes and switch back to static struct initialization.

Hipska · 2022-11-25T10:11:02Z

@srebhan Do you know it that has been done already?

powersj · 2023-02-15T20:21:35Z

next steps: @srebhan to look into reverting toml.Unmarshal changes and switch back to static struct initialization

srebhan · 2023-03-08T21:07:04Z

This only pays off if we convert more plugins to the infrastructure introduced in this PR. As time currently does not permit this, I will close the PR for now. If anyone is willing to work on this (and convert more plugins) please feel free to reopen or ping me.

Hipska · 2023-03-09T09:37:41Z

I would be able to migrate some if the framework is already there.. Now there is nothing..

srebhan · 2023-03-09T10:18:38Z

@Hipska as I wrote myself does not have time to convert them, but I can finish up this PR if you commit to convert others. ;-)

telegraf-tiger bot added fix pr to fix corresponding bug new plugin plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins labels Sep 24, 2021

srebhan requested a review from reimda September 25, 2021 15:02

srebhan marked this pull request as ready for review September 25, 2021 15:02

srebhan force-pushed the graphics_smi branch from fbc2baf to e751c8e Compare October 3, 2021 09:41

helenosheaa added feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin and removed fix pr to fix corresponding bug new plugin plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins labels Oct 4, 2021

powersj reviewed Oct 5, 2021

View reviewed changes

plugins/inputs/amd_rocm_smi/amd_rocm_smi_parser.conf Show resolved Hide resolved

Sven Rebhan added 12 commits October 13, 2021 13:24

Extend the Xpath-parser such that it can ignore invalid numbers.

376781d

Add testcase for GT-1030 Nvidia card.

f973f2b

Add implementation for a generic transport i.e. a data receiver.

c99832d

Add implementation for a generic 'receive and parse' plugin that cent…

83921b7

…ralizes the usual 'get data from a data-source and parse the result' pattern.

Move nvidia_smi to new generic 'ReceiveAndParse' plugin.

4f97a60

Move amd_rocm_smi to new generic 'ReceiveAndParse' plugin.

ab5285b

Remove debug code.

9c06fbb

Make 'ignore_nan' a config parameter instead of using it parser-wide.

a4c04f7

Make 'xpath_xml' a valid data_format for parsers.

dca0ca3

Move parser configuration for nvidia_smi to a separate config file th…

1f170f9

…at is embedded into the binary.

Move parser configuration for amd_rocm_smi to a separate config file …

f60c00c

…that is embedded into the binary.

Add link to documentation for the parser settings.

f7807e7

srebhan force-pushed the graphics_smi branch from 9155eef to f7807e7 Compare October 13, 2021 11:24

srebhan requested a review from powersj October 13, 2021 11:24

reimda suggested changes Oct 19, 2021

View reviewed changes

Fix comment

e73715c

Co-authored-by: reimda <reimda@users.noreply.github.com>

Hipska mentioned this pull request Oct 26, 2021

The default value is not set correctly (nvidia_smi plugin) #9979

Closed

srebhan mentioned this pull request Nov 8, 2021

Added hep protocol support as parser in telegraf #10039

Closed

3 tasks

Hipska self-assigned this Feb 20, 2022

powersj self-assigned this Dec 12, 2022

powersj 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 Dec 12, 2022

powersj removed 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 Jan 25, 2023

powersj assigned srebhan and unassigned Hipska and powersj Feb 15, 2023

srebhan closed this Mar 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Unify graphics SMI implementations #9815

feat: Unify graphics SMI implementations #9815

srebhan commented Sep 24, 2021 •

edited

Loading

reimda commented Sep 29, 2021

telegraf-tiger bot commented Oct 3, 2021

Artifact URLs

Hipska commented Oct 11, 2021

reimda Oct 14, 2021

srebhan Oct 20, 2021

reimda Sep 8, 2022

reimda Oct 19, 2021

srebhan Oct 20, 2021

reimda Sep 8, 2022

telegraf-tiger bot commented Oct 20, 2021

Artifact URLs

Hipska commented Sep 7, 2022

reimda commented Sep 8, 2022

Hipska commented Nov 25, 2022

powersj commented Feb 15, 2023

srebhan commented Mar 8, 2023

Hipska commented Mar 9, 2023

srebhan commented Mar 9, 2023

feat: Unify graphics SMI implementations #9815

feat: Unify graphics SMI implementations #9815

Conversation

srebhan commented Sep 24, 2021 • edited Loading

Required for all PRs:

reimda commented Sep 29, 2021

telegraf-tiger bot commented Oct 3, 2021

Artifact URLs

Hipska commented Oct 11, 2021

reimda Oct 14, 2021

Choose a reason for hiding this comment

srebhan Oct 20, 2021

Choose a reason for hiding this comment

reimda Sep 8, 2022

Choose a reason for hiding this comment

reimda Oct 19, 2021

Choose a reason for hiding this comment

srebhan Oct 20, 2021

Choose a reason for hiding this comment

reimda Sep 8, 2022

Choose a reason for hiding this comment

telegraf-tiger bot commented Oct 20, 2021

Artifact URLs

Hipska commented Sep 7, 2022

reimda commented Sep 8, 2022

Hipska commented Nov 25, 2022

powersj commented Feb 15, 2023

srebhan commented Mar 8, 2023

Hipska commented Mar 9, 2023

srebhan commented Mar 9, 2023

srebhan commented Sep 24, 2021 •

edited

Loading