feat(inputs.snmp): Hint to use source tag

[Hipska]

Required for all PRs

• Updated associated README.md.
• Wrote appropriate unit tests.
• Pull request title or commits are in conventional commit format

Related to #8011 and #4413

Add hint to use the source tag to standardise the snmp input by marking others as deprecated.

[srebhan]

@Hipska while I love the PR, the issue is that you double the cardinality (at least for InfluxDB) users with this change. Can you please add an option to switch between the existing tag and the new source tag!

[Hipska]

The added tag isn't holding high cardinality, it is one of the values of the agents config.

[powersj]

While it is not a potential source of lots of different values, it does affects a user's cardinality unnecessarily. Hence the request for a config value to opt-in to the change, rather than force this on users and tell them how to opt-out after they realize the change.

[Hipska]

Meaning this PR is useless, as you can opt-in with the agent_host_tag setting already.

The intention of this PR was to have a source tag by default and not opt-in.

[powersj]

@Hipska what do you want to do with this PR then? Make it a doc change to suggest using "source" or close this?

[Hipska]

I still would like to deprecate the agent_host_tag setting..

[powersj]

ok happy to see that, let us know when the PR is updated and ready for review again please

[Hipska]

It is already done..

[powersj]

Yes, but you have not changed anything with respect to our previous concerns around a new tag.

[Hipska]

Correct.

The intention of this PR was to have a source tag by default and not opt-in.

[powersj]

Thomas,

I am not going to go around with you on this. Both Sven and I are in agreement that we are not making this change. As-is this PR will not be accepted.

If you wish to still deprecate the option, that is fine, but any new option must be opt-in.

[Hipska]

Is it okay to have a warning in the log when the value of agent_host_tag is not "source" or if it is still the default?

[powersj]

If you mark the agent_host_tag as deprecated, that would result in two warnings for the user, right?

[Hipska]

Sorry, I wasn't clear enough. In that case the property wouldn't be deprecated anymore, as we cannot say it is deprecated and at the same time ask to set it to a specific value.

So, I would do an if(agent_host_tag != "source"){log.warn("..")} instead of the regular deprecation message. As the current way I did by adding an additional source tag doesn't get accepted.

[srebhan]

@Hipska I agree that we need to only conditionally warn if the setting is not "source". So how about

	if s.AgentHostTag != "source" {
		models.PrintOptionDeprecationNotice(telegraf.Warn, "inputs.snmp", "agent_host_tag", telegraf.DeprecationInfo{
			Since:  "1.29.0",
			Notice: `should be set to "source" and will thus be removed`,
		})
	}

which uses the deprecation-style and thus might trigger action while a usual warning will not...

[Hipska]

Indeed, it was something like this I was hinting at. I will use PrintOptionValueDeprecationNotice instead..

[srebhan]

Thanks @Hipska!

[telegraf-tiger]

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

[powersj]

Thank you!

[Hr0bar]

Think its OK the way it is, but its still incredibly confusing for the user. We got hit by it:

2024-01-05T06:41:57Z W! DeprecationWarning: Value "agent_host" for option "agent_host_tag" of plugin "inputs.snmp" deprecated since version 1.29.0 and will be removed in : should be set to "source" for consistent usage across plugins

And had to look at this thread to understand why we are getting a deprecation for a DEFAULT value that we dont explicitly set. But I see it was the default default without setting it explicitly, so hard to find a good decision how to handle... but the docs could exaplain it better perhaps? This is quite confusing in the docs (without looking up more details elsewhere):

snmp
tags:
agent_host (deprecated in 1.29: use source instead)

If agent_host_tag should be "source" and anything else is deprecated, then perhaps the deprecation should be for the entire agent_host_tag option that should get removed entirely in the future though? Instead of deprecating ANY value other than "source". Otherwise the deprecation warning will be kept forever if the option to set it to something different is kept?

[srebhan]

@Hr0bar a PR for an explanation in the README (or config) is welcome!