chore: Fix linter findings for `revive:exported` in `plugins/inputs/[w-z]*` by zak-pawel · Pull Request #16703 · influxdata/telegraf

zak-pawel · 2025-03-29T12:06:50Z

Summary

Address findings for revive:exported in plugins/inputs/[w-z]*.

As part of this effort for files from plugins/inputs/[w-z]*, the following actions were taken:

Type names (const, var, struct, func, etc) were changed to unexported, wherever they didn't need to be exported.
All remaining exported types were given comments in the appropriate form – this does not apply to exported methods that implement "known" plugin interfaces (Gather|Init|Start|Stop|SampleConfig|Parse|Add|Apply|Serialize|SerializeBatch|SetParser|SetParserFunc|GetState|SetState).
The order of methods was organized (exported methods first, then unexported, with init at the very end).

It is only part of the bigger work (for issue: #15813).
After all findings of this type in whole project are handled, we can enable revive:exported rule in golangci-lint.

Checklist

No AI generated code was used in this PR

…w-z]*`

srebhan

Thanks a lot @zak-pawel! Just a few comments...

srebhan · 2025-03-31T13:00:23Z

plugins/inputs/zfs/zfs.go

-type Sysctl func(metric string) ([]string, error)
-type Zpool func() ([]string, error)
-type Zdataset func(properties []string) ([]string, error)
-type Uname func() (string, error)
-
-type Zfs struct {
-	KstatPath      string
-	KstatMetrics   []string
-	PoolMetrics    bool
-	DatasetMetrics bool
-	Log            telegraf.Logger `toml:"-"`
-
-	sysctl   Sysctl   //nolint:unused // False positive - this var is used for non-default build tag: freebsd
-	zpool    Zpool    //nolint:unused // False positive - this var is used for non-default build tag: freebsd
-	zdataset Zdataset //nolint:unused // False positive - this var is used for non-default build tag: freebsd
-	uname    Uname    //nolint:unused // False positive - this var is used for non-default build tag: freebsd
-	version  int64    //nolint:unused // False positive - this var is used for non-default build tag: freebsd
-}
-


My problem with this change is that it will add redundancy for the config parameters i.e. if we add a new flag we now need to remember to all the different OS files. How about moving the "internal" variable to one OS-dependent implementation and have only one flag "nolint" member in the struct?

@srebhan

We once had a very similar discussion about ethtool: #13246 (comment)
It ended with very similar changes to what I’m proposing now: https://github.com/influxdata/telegraf/pull/13599/files#diff-949ac46ede5a4cd042a07cdba8e02ffa378666adb858898cad7d6c4bbbedc7b3

But if you insist, could you elaborate on your proposed fix? I’m not sure I fully understand it.

The issue concerns type definitions:

type sysctlF func(metric string) ([]string, error) type zpoolF func() ([]string, error) type zdatasetF func(properties []string) ([]string, error) type unameF func() (string, error)

(which, after being changed to unexported, started being detected by the unused linter.)

And struct fields:

DatasetMetrics bool `toml:"datasetMetrics"` sysctl sysctlF zpool zpoolF zdataset zdatasetF uname unameF

(which are used only in freebsd)

Well I thought about what we did in systemd_units but it seems like this looks the same... Anyway, as all other plugins do the same thing, I'm not vetoing this however, I'm questioning the usefulness of the new code. Up to now you could roll out a configuration for ZFS on all systems and would only see a log message. With your change, Telegraf will error out as some platforms now don't have certain parameters defined and are therefore failing at config-parsing state...

I see your point. Reverted to have single struct.

srebhan · 2025-03-31T13:02:49Z

plugins/inputs/zipkin/codec/jsonV1/jsonV1_test.go

 		t.Run(tt.name, func(t *testing.T) {
-			got, err := TraceIDFromString(tt.s)
+			got, err := traceIDFromString(tt.s)
 			if (err != nil) != tt.wantErr {
-				t.Errorf("TraceIDFromString() error = %v, wantErr %v", err, tt.wantErr)
+				t.Errorf("traceIDFromString() error = %v, wantErr %v", err, tt.wantErr)
 				return
 			}
 			if got != tt.want {
-				t.Errorf("TraceIDFromString() = %v, want %v", got, tt.want)
+				t.Errorf("traceIDFromString() = %v, want %v", got, tt.want)


As you already touching this, do you think it's worth directly migrating those test to require? Same for the places below.

Sure, changed in whole zipkin package

srebhan · 2025-03-31T13:04:13Z

plugins/inputs/zipkin/zipkin.go

+var (
+	// defaultNetwork is the network to listen on; use only in tests.
+	defaultNetwork = "tcp"
+)
+


If it's only used in tests, why not define it there?

It is used both in prod and test. In test it is used to overwrite network.

srebhan · 2025-03-31T13:05:18Z

plugins/inputs/zookeeper/zookeeper.go

+const (
+	defaultTimeout = 5 * time.Second
+)


I hate those definitions! They add three lines without any benefit instead of just using the value at the corresponding location...

Just use this value

…orted-inputs_w-z

…xported-inputs_w-z

srebhan

Thanks a lot @zak-pawel!

zak-pawel · 2025-04-01T17:30:31Z

@srebhan
I woke up at night screaming because I had nightmares about those //nolints.
So here's a slightly better version :)

telegraf-tiger · 2025-04-01T17:57:08Z

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

skartikey

@zak-pawel Great work on this PR!

Hipska · 2025-04-02T07:45:19Z

plugins/inputs/zfs/zfs_freebsd.go

+type helper struct {
+	sysctl   sysctlF
+	zpool    zpoolF
+	zdataset zdatasetF
+	uname    unameF
+}
+
+type sysctlF func(metric string) ([]string, error)
+type zpoolF func() ([]string, error)
+type zdatasetF func(properties []string) ([]string, error)
+type unameF func() (string, error)


Do they really need to be variable functions? Can't they be used directly?

- lines, err := z.zdataset(properties) + lines, err := zdataset(properties)

Have you tried running the tests after this change? 🤔

Okay, thanks for just nicely saying that it's needed to mock them for tests..

…w-z]*` (influxdata#16703)

…w-z]*` (#16703) (cherry picked from commit ad76475)

chore: Fix linter findings for revive:exported in `plugins/inputs/[…

4054ff9

…w-z]*`

telegraf-tiger bot added the chore label Mar 29, 2025

zak-pawel added 2 commits March 29, 2025 13:17

z

0a73740

separate config

d53d213

zak-pawel requested review from mstrandboge, skartikey and srebhan March 29, 2025 13:07

zak-pawel assigned skartikey, srebhan and mstrandboge Mar 29, 2025

zak-pawel added the linter label Mar 29, 2025

Hipska approved these changes Mar 31, 2025

View reviewed changes

srebhan reviewed Mar 31, 2025

View reviewed changes

srebhan unassigned skartikey and mstrandboge Mar 31, 2025

zak-pawel added 3 commits March 31, 2025 20:32

Merge remote-tracking branch 'origin/master' into zak-pawelrevive-exp…

c78c563

…orted-inputs_w-z

Merge remote-tracking branch 'upstream/master' into zak-pawelrevive-e…

0a2d8bc

…xported-inputs_w-z

address review comments

2338432

srebhan approved these changes Mar 31, 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 Mar 31, 2025

srebhan assigned skartikey and mstrandboge and unassigned srebhan Mar 31, 2025

better zfs

097532f

skartikey unassigned mstrandboge Apr 1, 2025

skartikey approved these changes Apr 1, 2025

View reviewed changes

skartikey merged commit ad76475 into influxdata:master Apr 1, 2025
26 of 27 checks passed

github-actions bot added this to the v1.34.2 milestone Apr 1, 2025

zak-pawel deleted the zak-pawelrevive-exported-inputs_w-z branch April 1, 2025 21:34

Hipska reviewed Apr 2, 2025

View reviewed changes

skartikey pushed a commit to skartikey/telegraf that referenced this pull request Apr 14, 2025

chore: Fix linter findings for revive:exported in `plugins/inputs/[…

c155b2b

…w-z]*` (influxdata#16703)

srebhan pushed a commit that referenced this pull request Apr 14, 2025

chore: Fix linter findings for revive:exported in `plugins/inputs/[…

0edb83b

…w-z]*` (#16703) (cherry picked from commit ad76475)

Conversation

zak-pawel commented Mar 29, 2025

Summary

Checklist

Uh oh!

srebhan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

srebhan left a comment

Choose a reason for hiding this comment

Uh oh!

zak-pawel commented Apr 1, 2025

Uh oh!

telegraf-tiger bot commented Apr 1, 2025

Artifact URLs

Uh oh!

skartikey left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Comments