feat: Implement deprecation infrastructure by srebhan · Pull Request #9857 · influxdata/telegraf

srebhan · 2021-10-04T17:48:34Z

Required for all PRs:

Updated associated README.md.
Wrote appropriate unit tests.
Pull request title or commits are in conventional commit format (e.g. feat: or fix:)

relates to #9478

This PR adds the possibility to deprecate plugins in a machine-readable form by adding an entry to the deprecations.go file in the plugin category (e.g. inputs). This entry needs to at least contain the version since when the plugin was deprecated and a notice for the user e.g. suggesting replacements. Furthermore, you can also deprecate plugin config-options by adding a deprecated tag like

	Address         string   `toml:"address" deprecated:"1.12.0;use 'urls' instead"`

also providing a "since" version and a hint separated by a colon.
Both, plugin and option deprecation, can also specify a 'removal in' version, specifying the version when removal of the plugin or option is planned. For the deprecated tag you simply add it as a second version after since, e.g.

	Address         string   `toml:"address" deprecated:"<since>;<removal in>;<note for the user>"`

The new framework is then used to print deprecation warnings (or errors) during startup of telegraf or to print a list of deprecated options or plugins via telegraf --deprecation-list.

powersj

This is pretty sweet, thanks for putting this together. I have some high-level comments about what this looks like below. Thanks again!

Makefile

config/deprecation.go

telegraf-tiger · 2021-10-05T09:33:28Z

Looks like new artifacts were built from this PR. Get them here!

Artifact URLs

cmd/telegraf/telegraf.go

Hipska · 2021-10-07T09:13:50Z

Adding an information message here if user uses deprecated options/plugins would be nice:

telegraf/cmd/telegraf/telegraf.go

Lines 267 to 271 in 06edde6

    
           log.Printf("I! Loaded inputs: %s", strings.Join(c.InputNames(), " ")) 
        
           log.Printf("I! Loaded aggregators: %s", strings.Join(c.AggregatorNames(), " ")) 
        
           log.Printf("I! Loaded processors: %s", strings.Join(c.ProcessorNames(), " ")) 
        
           log.Printf("I! Loaded outputs: %s", strings.Join(c.OutputNames(), " ")) 
        
           log.Printf("I! Tags enabled: %s", c.ListTags())

(This will go into any configured logfile, instead of the pre-config-completely-loaded stdout/stderr messages)

Hipska · 2021-10-14T07:33:59Z

You might also update docs/COMMANDS_AND_FLAGS.md

srebhan · 2021-10-14T08:03:26Z

@Hipska good catch. Done.

docs/COMMANDS_AND_FLAGS.md

telegraf-tiger · 2021-10-15T11:23:31Z

📦 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

powersj

@srebhan,

@reimda and I were talking through this during pairs. One of the scenarios we ran across is what happens when a plugin is removed. When we remove a plugin it would be nice to be able to remove the source code and delete it from the all.go file and be done with it. As this is currently written, we would need to keep the plugin around for the deprecation code to get called.

Also, a similar question for the removal of fields. In order to finally remove the field but also notify the user, how will this work without leaving the field in?

Dave and I tried to fake this with a _ or a _foobar and neither worked as we expected. Have you tried this out?

Thanks!

srebhan · 2021-10-22T14:49:07Z

@powersj, @reimda I'm not sure I do understand your examples. IMO a plugin/plugin option is deprecated in a certain version (e.g. 1.20). In this example I do expect the user to receive a warning starting from version 1.20 on. The next step is, after a grace time of e.g. 4 minor updates (>=1.24) or one major + 4 minor (>= 2.4), the warning turns into an error (we might want to allow starting nonetheless with a cmdline switch or similar). Now, maybe after another gracetime (e.g. >= 1.28 or 2.8) the plugin or option can be removed at any time without further notice.
The time-periods are subject to discussion, but in the example that would give a user ~1 year from deprecation to react to the warning and another ~year to override the error. So in total a user has ~2 years to migrate...

Of course the deprecation warning is then gone when the plugin or plugin option is removed, but that's correct as there is no deprecated plugin/option any longer. What am I missing here? Is your intention to remove the plugin/plugin option and still get a warning? If so why? A plugin removed will trigger config errors, so there is no silent fail anyway...

plugins/inputs/deprecations.go

config/config.go

config/deprecation.go

telegraf-tiger · 2021-11-11T11:34:08Z

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

📦 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

powersj

This is looking awesome! I have some questions and one issue when trying this out setting the build_version to 2.0.0.

config/config.go

plugins/inputs/deprecations.go

config/deprecation.go

This reverts commit 3b95f31.

…irect) dependency.

…ons.

go.mod

…s a (indirect) dependency." This reverts commit 184d745.

telegraf-tiger · 2021-11-22T08:01:13Z

☺️ 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

powersj

+1 - need Dave to sign off, so that will happen early next week. I do know that he will probably comment on the behavior of when RemovalIn is not set. I think his desire, and I agree with it, is that we are explicit anytime we deprecate a plugin and that means setting the RemovalIn value.

Hipska · 2021-11-23T11:43:15Z

Just stumbled on this; What about deprecation of agent/global config options?

srebhan · 2021-11-23T13:53:14Z

@Hipska this is out of scope for this PR. Getting this to the current state was already a lot of (non-technical) discussions and decision-making, so I suggest to postpone more features to future PRs.
Anyway, this will not be a standard mechanism and probably needs a manual source of information...

srebhan · 2021-11-23T14:01:26Z

@powersj if requiring a RemovalIn is already "set in stone" I can implement it before Dave's review... :-)

However, I also want you to consider developers trying to deprecate things. Currently, it is possible but not mandatory to set the RemovalIn field, so a developer providing a replacement can add a "deprecation" which says, starting from "x.y.z" you better use the new plugin, leaving the removal "open" to the default next possible date. You can then postpone this date for any reason. The question now is, what is the default. Will you always want to set a custom version for the removal? This means you need to tell the developer in the PR what to put into the RemovalIn field. Or do you never want the dev to deprecate the "old" plugin?

reimda · 2021-12-01T19:14:12Z

@srebhan Could you resolve the conflict? Otherwise it's ready to merge.

reimda · 2021-12-01T19:43:09Z

I fixed the conflict in #10200 and merged it. Closing this. Thanks @srebhan for the PR!

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

powersj reviewed Oct 4, 2021

View reviewed changes

Makefile Show resolved Hide resolved

config/deprecation.go Outdated Show resolved Hide resolved

config/deprecation.go Show resolved Hide resolved

config/deprecation.go Outdated Show resolved Hide resolved

Hipska reviewed Oct 7, 2021

View reviewed changes

cmd/telegraf/telegraf.go Outdated Show resolved Hide resolved

srebhan requested a review from Hipska October 13, 2021 13:20

Hipska approved these changes Oct 14, 2021

View reviewed changes

srebhan force-pushed the deprecation branch from 0d94a71 to 965d19d Compare October 14, 2021 08:04

Hipska reviewed Oct 14, 2021

View reviewed changes

docs/COMMANDS_AND_FLAGS.md Outdated Show resolved Hide resolved

Hipska 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 Oct 14, 2021

powersj suggested changes Oct 21, 2021

View reviewed changes

Hipska approved these changes Oct 22, 2021

View reviewed changes

Hipska added the area/agent label Oct 22, 2021

srebhan force-pushed the deprecation branch from 8aa8eb7 to 0e92cbe Compare November 9, 2021 19:10

srebhan removed 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 Nov 9, 2021

srebhan requested review from Hipska and powersj November 9, 2021 19:11

Hipska reviewed Nov 10, 2021

View reviewed changes

plugins/inputs/deprecations.go Outdated Show resolved Hide resolved

config/config.go Show resolved Hide resolved

config/deprecation.go Outdated Show resolved Hide resolved

config/deprecation.go Show resolved Hide resolved

config/deprecation.go Show resolved Hide resolved

srebhan force-pushed the deprecation branch from 0e92cbe to 7b51960 Compare November 11, 2021 09:20

sspaink mentioned this pull request Nov 11, 2021

feat: Parser plugin restructuring #8791

Merged

2 tasks

powersj reviewed Nov 15, 2021

View reviewed changes

Sven Rebhan added 4 commits November 17, 2021 14:56

Make version available in config.

5dd278f

Define interface for declaring a plugin as deprecated.

d6ab0ca

Define telegraf version for the non-tagged case.

accd613

Implement deprecation handling.

815822d

Sven Rebhan added 12 commits November 17, 2021 14:56

Make version internal.

f9593f4

Fix linter error.

fcbcda6

Improve error message.

9a3ae7d

Formatting.

70fec20

Update deprecation information from Changelog.

56c9a3a

Revert "Remove color from deprecation output."

3d3fc20

This reverts commit 3b95f31.

Handle processor and aggregator deprecations as well.

aaedcbf

Rename 'Level' to 'LogLevel'.

f10f487

Make all versions tri-dotted format.

4dcad88

Print origin for version panic to ease debugging.

d013d33

Switch to 'github.com/blang/semver' as we already have this as a (ind…

184d745

…irect) dependency.

Drop potential pre-release tags for telegraf when comparing the versi…

d50bf5b

…ons.

srebhan force-pushed the deprecation branch from 6130c73 to d50bf5b Compare November 17, 2021 13:57

Hipska reviewed Nov 17, 2021

View reviewed changes

go.mod Outdated Show resolved Hide resolved

Revert "Switch to 'github.com/blang/semver' as we already have this a…

c5411e6

…s a (indirect) dependency." This reverts commit 184d745.

Hipska approved these changes Nov 18, 2021

View reviewed changes

Merge branch 'master' into deprecation

2e98015

srebhan requested a review from powersj November 22, 2021 07:42

Hipska 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 Nov 22, 2021

powersj approved these changes Nov 22, 2021

View reviewed changes

reimda approved these changes Dec 1, 2021

View reviewed changes

reimda mentioned this pull request Dec 1, 2021

feat: Implement deprecation infrastructure #10200

Merged

reimda closed this Dec 1, 2021

srebhan deleted the deprecation branch December 2, 2021 08:49

Conversation

srebhan commented Oct 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Required for all PRs:

Uh oh!

powersj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

telegraf-tiger bot commented Oct 5, 2021

Artifact URLs

Uh oh!

Uh oh!

Hipska commented Oct 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Hipska commented Oct 14, 2021

Uh oh!

srebhan commented Oct 14, 2021

Uh oh!

Uh oh!

telegraf-tiger bot commented Oct 15, 2021

Artifact URLs

Uh oh!

powersj left a comment

Choose a reason for hiding this comment

Uh oh!

srebhan commented Oct 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

telegraf-tiger bot commented Nov 11, 2021

Artifact URLs

Uh oh!

powersj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

telegraf-tiger bot commented Nov 22, 2021

Artifact URLs

Uh oh!

powersj left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hipska commented Nov 23, 2021

Uh oh!

srebhan commented Nov 23, 2021

Uh oh!

srebhan commented Nov 23, 2021

Uh oh!

reimda commented Dec 1, 2021

Uh oh!

reimda commented Dec 1, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

srebhan commented Oct 4, 2021 •

edited

Loading

Hipska commented Oct 7, 2021 •

edited

Loading

srebhan commented Oct 22, 2021 •

edited

Loading

powersj left a comment •

edited

Loading