chore: Forbids "log" package only for aggregators, inputs, outputs, parsers and processors

[zak-pawel]

It is possible to inject telegraf.Logger (instead of log) only into aggregators, inputs, outputs, parsers and processors so there is no point in forbidding usages of log in other places.

[srebhan]

Hey @zak-pawel, can we be a bit more specific in plugins/common? I only see a valid use in the shim and logrus subdirs. Everything else either already uses a telegraf.Logger or can (and should) be fixed in my view... What do you think?

[srebhan]

Can't we even disable the blacklist in those files directly instead of the config file to make people aware of the exception?

[zak-pawel]

@srebhan

Can't we even disable the blacklist in those files directly instead of the config file to make people aware of the exception?

Do you want to place //nolint ... in every file which is not in aggregators, inputs, outputs, parsers or serializers directory?

Hey @zak-pawel, can we be a bit more specific in plugins/common? I only see a valid use in the shim and logrus subdirs. Everything else either already uses a telegraf.Logger or can (and should) be fixed in my view... What do you think?

If I understand this document correctly, telegraf.Logger should be used only in places where it is automatically injected, so for these types of plugins: aggregators, inputs, outputs, parsers and serializers. So for common package log should be used. Am I right?

[srebhan]

So for common package log should be used. Am I right?

I have no idea, maybe that's for discussion with the core-devs...

Do you want to place //nolint ... in every file which is not in aggregators, inputs, outputs, parsers or serializers directory?
No, but maybe only in the ones in plugins/common that really need it. The background is that people tend to take all code as examples and then copy the behavior to other places, spreading the word. :-) So in my ideal world, everything under plugins/ is 'no-log' land except for the few places where it is really needed. And in those locations we should be explicit in stating that this is an exception (like in common/shim and common/logrus). That is two files IIRC...

[reimda]

So for common package log should be used. Am I right?

I have no idea, maybe that's for discussion with the core-devs...

Makes sense to me only to blacklist using the log package where the Log is injected.

[zak-pawel]

So for common package log should be used. Am I right?

I have no idea, maybe that's for discussion with the core-devs...

Makes sense to me only to blacklist using the log package where the Log is injected.

@reimda Exactly this happens thanks to current changes in this PR :)

[srebhan]

First, let me be clear, my only concern is about plugins/common here.

Makes sense to me only to blacklist using the log package where the Log is injected.

I guess the question is: Should we blacklist plugins/common in general or only the two places (shim and logrus) that need log? For example, http and starlark use the telegraf logger passed in by their users, so I'm a bit hesitant to disable checks for those parts.
Wouldn't the following be sufficient in the two files

	"log" //nolint:revive // Allow exceptional but valid use of log here.

[zak-pawel]

First, let me be clear, my only concern is about plugins/common here.

Makes sense to me only to blacklist using the log package where the Log is injected.

I guess the question is: Should we blacklist plugins/common in general or only the two places (shim and logrus) that need log? For example, http and starlark use the telegraf logger passed in by their users, so I'm a bit hesitant to disable checks for those parts. Wouldn't the following be sufficient in the two files

	"log" //nolint:revive // Allow exceptional but valid use of log here.

If @reimda is OK with that, I will make a change :)

[powersj]

I'm also +1 on adding limited exceptions via //no lint:revive comments in specific files versus adding exceptions to a global file.

[Hipska]

Same with Sven

[zak-pawel]

@srebhan @reimda @powersj @Hipska Exclusion of plugins/common package for importing "log" rule was removed.
Moreover I corrected path filter (in previous commit it also ruled out wrong files)

[telegraf-tiger]

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

📦 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