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.

Please change the function to take a mandatory path or source parameter and unconditionally add the source to the plugin models. The only place that should be conditional should be the printing.

Remember this is a public, exported function and we shouldn't change the signature too often. Having the plugin source in the model is a good thing so please add it.

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

srebhan

@neelayu thanks for your update and sorry for this taking so long (due to holidays). I do have two minor comments in the code and a request to no go for the option pattern for the LoadConfigData function. Let's change the function signature once and remove any room for errors.

srebhan · 2025-01-15T19:45:18Z

config/plugin_printer.go

+
+type pluginNames []pluginPrinter
+
+func GetPluginSourcesTable(pluginNames []pluginPrinter) string {


Please do not export this function.

srebhan · 2025-01-15T19:45:55Z

go.mod

+require (
+	github.com/mattn/go-runewidth v0.0.16 // indirect
+	github.com/rivo/uniseg v0.4.7 // indirect
+)
+
 require (


Please manually fuse the two requires.

neelayu · 2025-01-16T08:24:29Z

@srebhan I have enforced the func signature. Let me know. Thanks!

telegraf-tiger · 2025-01-16T08:50:09Z

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

srebhan

Thanks a lot @neelayu!

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 force-pushed the plugin-printer branch from e0ec63d to 13fe6a0 Compare December 20, 2024 16:37

srebhan reviewed Jan 15, 2025

View reviewed changes

neelayu added 12 commits January 16, 2025 12:17

Add basic framework

ac698cb

Put this behind a flag

0bf4702

Add support for secret store

87f79bc

Minor fixes in table printer

20f593e

Changes in tests

b2f2aef

Make linter happy

10ce80a

Rename flag variable

8dd9664

Fixing the test by using filepath instead of hardcoded strings

35e9d38

Address review comments

ba0ca76

Add license information

5bc6316

Reorder licenses

f399c29

address comments and enforce path in function signature

487823e

neelayu force-pushed the plugin-printer branch from 13fe6a0 to 487823e Compare January 16, 2025 08:23

srebhan approved these changes Jan 17, 2025

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 Jan 17, 2025

srebhan assigned DStrand1 and unassigned srebhan Jan 17, 2025


		type pluginNames []pluginPrinter

		func GetPluginSourcesTable(pluginNames []pluginPrinter) string {

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

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

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

neelayu commented Jan 16, 2025

telegraf-tiger bot commented Jan 16, 2025

Artifact URLs

srebhan left a comment

Choose a reason for hiding this comment

neelayu commented Dec 6, 2024 •

edited

Loading