feat(agent): Print plugins source information #16270

neelayu · 2024-12-06T19:11:45Z

Summary

Print the source information for the plugins.

Put this feature behind a flag to preserve backward compatibility in systems relying on Loaded inputs, Outputs strings for validations.
Flag introduced: --print-plugin-config-source Default: false

If required, we can eliminate this flag.

Sample output on telegraf init

2024-12-06T19:05:36Z I! Loaded inputs: cpu disk (2x) internal interrupts mem (2x) net (3x)
+--------------+------------------------------------------------------+
| Name         | Source(s)                                            |
+--------------+------------------------------------------------------+
| net (3x)     | /Users/user-of-unix/telegraf/configs1/config3.conf   |
|              | /Users/user-of-unix/telegraf/configs1/config1.conf   |
|              | /Users/user-of-unix/telegraf/configs1/config2.conf   |
+--------------+------------------------------------------------------+
| disk (2x)    | /Users/user-of-unix/telegraf/configs1/config2.conf   |
|              | /Users/user-of-unix/telegraf/configs1/config2.conf   |
+--------------+------------------------------------------------------+
| mem (2x)     | /Users/user-of-unix/telegraf/configs1/config4.conf   |
|              | /Users/user-of-unix/telegraf/configs1/config2.conf   |
+--------------+------------------------------------------------------+
| internal     | /Users/user-of-unix/telegraf/configs1/config2.conf   |
+--------------+------------------------------------------------------+
| interrupts   | /Users/user-of-unix/telegraf/configs1/config2.conf   |
+--------------+------------------------------------------------------+
| cpu          | /Users/user-of-unix/telegraf/configs1/config5.conf   |
+--------------+------------------------------------------------------+

Checklist

No AI generated code was used in this PR

Related issues

resolves #16269

srebhan · 2024-12-09T18:01:37Z

@neelayu I really like this! Can we please make the flag being something like --print-plugin-config-source or --print-plugin-origin? This would be a bit more speaking I think and avoids the next flag being called --even-newer-plugin-print-behavior. ;-)

Furthermore, please fix the linter issues.

neelayu · 2024-12-10T13:26:39Z

@srebhan Thanks. I have made the changes. The test failure seems unrelated to my change though.

srebhan

Thanks @neelayu for the update. Some more comments from my side.

srebhan · 2024-12-13T10:23:17Z

config/config.go

@@ -74,7 +77,8 @@ type Config struct {
 	OutputFilters      []string
 	SecretStoreFilters []string

-	SecretStores map[string]telegraf.SecretStore
+	SecretStores      map[string]telegraf.SecretStore
+	secretStoreSource []secretStoreConfig


Why not using a map[string]string mapping the name to the source?

I had considered using a map[string]string, but it will not help if have multiple secretstores with the same names. In that case it would be map[string][]string.

The SecretStore map on line 80 stores ID to underlying interface and hence it works.

Let me know your thoughts.

Ok I see, but I would use map[string][]string in this case to collect the sources for the same type and print them in groups...

Ack. I've made the change.

srebhan · 2024-12-13T10:29:23Z

config/config.go

+func getPluginPrintString(plugins pluginNames) string {
+	output := PluginNameCounts(plugins)
+	if PrintPluginConfigSource {
+		return output + plugins.String()
+	}
+
+	return output
+}


Please merge this back to the *Names() functions! Stripping this out to save the if is not worth it.

srebhan · 2024-12-13T10:35:22Z

config/config.go

+// InputNames returns a string of configured inputs.
+func (c *Config) InputNames() string {
+	plugins := make(pluginNames, 0, len(c.Inputs))
 	for _, input := range c.Inputs {
-		name = append(name, input.Config.Name)
+		plugins = append(plugins, pluginPrinter{
+			name:   input.Config.Name,
+			source: input.Config.Source,
+		})
 	}
-	return PluginNameCounts(name)
+	return getPluginPrintString(plugins)
 }


How about keeping this as is and add a function InputNamesWithSources() and call this in telegraf.go? Same for the other plugin types. This simplifies the code I think and we can keep the short printing in telegraf.go and add printing the sources.

That makes sense. However, can you clarify the following-

We print short form as we do currently. But with this PR, we also print the source table. If that is the case, do we require the flag that I introduced? My intention of introducing the flag was to ensure no regressions on systems already using telegraf and checking for strings like Loaded inputs

We currently print a line for all loaded inputs etc. This can then still be there and use InputNames().

Then after that "header" we can print the more detailed information you add if --print-plugin-source... is provided. Keeping those lines will ensure that we don't break users if they rely on that header...

Made the changes.

srebhan · 2024-12-13T10:36:41Z

config/config.go

 // LoadConfigData loads TOML-formatted config data
-func (c *Config) LoadConfigData(data []byte) error {
+func (c *Config) LoadConfigData(data []byte, opts ...cfgDataOption) error {


I think you should just pass the source here. If we ever want to add other flags we can still refactor this later.

The reason for introducing functional ops pattern was to prevent breaking other files calling this func. And there are about 50+ across 20+ files.

I see your point but my fear is that people might forget about adding the source in future changes and we will not notice. You might keep this as is but I really would feel better if we enforce providing a source... ;-)

I am inclined to keep this as-is for this PR. I can open up a second one to enforce the func signature change. Let me know your thoughts.

srebhan · 2024-12-13T10:38:00Z

config/plugin_printer.go

+// GetPluginTableContent prints a bordered ASCII table.
+//
+// Inputs:
+//
+//	headers: slice of strings containing the headers of the table
+//	data: slice of slices of strings containing the data to be printed
+//
+// Reference: https://github.com/olekukonko/tablewriter
+func GetPluginTableContent(headers []string, data [][]any) string {


I think using https://github.com/jedib0t/go-pretty here is the better alternative. :-)

Yes. This one is a popular library. But since I wanted something lightweight. Hence, I considered writing it using the above as reference. If Telegraf authors are okay with adding a new library, I can do it :)

Yeah I'd rather add a dependency than stuffing in code here. :-)

neelayu

Thanks @srebhan

neelayu · 2024-12-13T10:52:44Z

config/plugin_printer.go

+// GetPluginTableContent prints a bordered ASCII table.
+//
+// Inputs:
+//
+//	headers: slice of strings containing the headers of the table
+//	data: slice of slices of strings containing the data to be printed
+//
+// Reference: https://github.com/olekukonko/tablewriter
+func GetPluginTableContent(headers []string, data [][]any) string {


Yes. This one is a popular library. But since I wanted something lightweight. Hence, I considered writing it using the above as reference. If Telegraf authors are okay with adding a new library, I can do it :)

neelayu · 2024-12-13T10:55:02Z

config/config.go

@@ -74,7 +77,8 @@ type Config struct {
 	OutputFilters      []string
 	SecretStoreFilters []string

-	SecretStores map[string]telegraf.SecretStore
+	SecretStores      map[string]telegraf.SecretStore
+	secretStoreSource []secretStoreConfig


I had considered using a map[string]string, but it will not help if have multiple secretstores with the same names. In that case it would be map[string][]string.

The SecretStore map on line 80 stores ID to underlying interface and hence it works.

Let me know your thoughts.

neelayu · 2024-12-13T10:59:07Z

config/config.go

+// InputNames returns a string of configured inputs.
+func (c *Config) InputNames() string {
+	plugins := make(pluginNames, 0, len(c.Inputs))
 	for _, input := range c.Inputs {
-		name = append(name, input.Config.Name)
+		plugins = append(plugins, pluginPrinter{
+			name:   input.Config.Name,
+			source: input.Config.Source,
+		})
 	}
-	return PluginNameCounts(name)
+	return getPluginPrintString(plugins)
 }


That makes sense. However, can you clarify the following-

We print short form as we do currently. But with this PR, we also print the source table. If that is the case, do we require the flag that I introduced? My intention of introducing the flag was to ensure no regressions on systems already using telegraf and checking for strings like Loaded inputs

neelayu · 2024-12-13T11:01:19Z

config/config.go

 // LoadConfigData loads TOML-formatted config data
-func (c *Config) LoadConfigData(data []byte) error {
+func (c *Config) LoadConfigData(data []byte, opts ...cfgDataOption) error {


The reason for introducing functional ops pattern was to prevent breaking other files calling this func. And there are about 50+ across 20+ files.

srebhan · 2024-12-20T08:53:04Z

@neelayu you need to update the /docs/LICENSE_OF_DEPENDENCIES.md file with the new dependencies' license information. Please check make check-deps for what needs to be done!

neelayu · 2024-12-20T13:37:38Z

@srebhan
I believe I have already added them here https://github.com/influxdata/telegraf/pull/16270/files#diff-2afa363df9dd5dc9cbf8b97542633fda73bf1747a7959b0da02e518a976db745R460

Not sure why it is asking me to remove them again?

--- /tmp/tmp.3FJLxju4ZG/LICENSE_OF_DEPENDENCIES.md      2024-12-16 06:45:42.992042502 +0000
+++ /tmp/tmp.3FJLxju4ZG/HEAD    2024-12-16 06:45:42.992042502 +0000
@@ -233,0 +234 @@
+github.com/jedib0t/go-pretty
@@ -259,0 +261 @@
+github.com/mattn/go-runewidth
@@ -333,0 +336 @@
+github.com/rivo/uniseg
@@ -455,3 +457,0 @@
-github.com/jedib0t/go-pretty
-github.com/mattn/go-runewidth
-github.com/rivo/uniseg


The docs/LICENSE_OF_DEPENDENCIES.md file does not contain the expected entries.

Lines prefixed with '+' should be added to LICENSE_OF_DEPENDENCIES.md and '-'
lines should be removed.

Include a link to the appropriate licenses for any additions.

srebhan · 2024-12-20T15:39:20Z

Seems like you got the positions wrong. The entries are in lexicographic order so you also need to insert the lines in the right places. ;-)

neelayu · 2024-12-20T16:51:52Z

Thanks @srebhan.
Made the changes now

telegraf-tiger · 2024-12-20T17:03:19Z

Download PR build artifacts for linux_amd64.tar.gz, darwin_arm64.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

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

srebhan self-assigned this Dec 9, 2024

srebhan added the area/agent label Dec 9, 2024

neelayu force-pushed the plugin-printer branch from 39f3020 to c885748 Compare December 10, 2024 10:34

srebhan reviewed Dec 13, 2024

View reviewed changes

neelayu commented Dec 13, 2024

View reviewed changes

neelayu force-pushed the plugin-printer branch from 6c83f2d to f066526 Compare December 14, 2024 13:05

neelayu added 10 commits December 20, 2024 19:03

Add basic framework

92dd074

Put this behind a flag

12fd80c

Add support for secret store

4e6bd9b

Minor fixes in table printer

74307d6

Changes in tests

9dbb5db

Make linter happy

8251557

Rename flag variable

7eee985

Fixing the test by using filepath instead of hardcoded strings

e3b533b

Address review comments

75467f9

Add license information

6e61c3c

Reorder licenses

13fe6a0

neelayu force-pushed the plugin-printer branch from e0ec63d to 13fe6a0 Compare December 20, 2024 16:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(agent): Print plugins source information #16270

feat(agent): Print plugins source information #16270

neelayu commented Dec 6, 2024 •

edited

Loading

srebhan commented Dec 9, 2024

neelayu commented Dec 10, 2024

srebhan left a comment

srebhan Dec 13, 2024

neelayu Dec 13, 2024

srebhan Dec 13, 2024

neelayu Dec 14, 2024

srebhan Dec 13, 2024

neelayu Dec 14, 2024

srebhan Dec 13, 2024

neelayu Dec 13, 2024

srebhan Dec 13, 2024

neelayu Dec 14, 2024

srebhan Dec 13, 2024

neelayu Dec 13, 2024

srebhan Dec 13, 2024

neelayu Dec 14, 2024

srebhan Dec 13, 2024

neelayu Dec 13, 2024

srebhan Dec 13, 2024

neelayu Dec 14, 2024

neelayu left a comment

neelayu Dec 13, 2024

neelayu Dec 13, 2024

neelayu Dec 13, 2024

neelayu Dec 13, 2024

srebhan commented Dec 20, 2024

neelayu commented Dec 20, 2024

srebhan commented Dec 20, 2024

neelayu commented Dec 20, 2024

telegraf-tiger bot commented Dec 20, 2024

Artifact URLs

feat(agent): Print plugins source information #16270

Are you sure you want to change the base?

feat(agent): Print plugins source information #16270

Conversation

neelayu commented Dec 6, 2024 • edited Loading

Summary

Checklist

Related issues

srebhan commented Dec 9, 2024

neelayu commented Dec 10, 2024

srebhan 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

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

neelayu 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

srebhan commented Dec 20, 2024

neelayu commented Dec 20, 2024

srebhan commented Dec 20, 2024

neelayu commented Dec 20, 2024

telegraf-tiger bot commented Dec 20, 2024

Artifact URLs

neelayu commented Dec 6, 2024 •

edited

Loading