Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs(processors.ifname): Document better agent tag name #14117

Merged

Conversation

Hipska
Copy link
Contributor

@Hipska Hipska commented Oct 16, 2023

Required for all PRs

Related to #8011 and #4413

@telegraf-tiger telegraf-tiger bot added feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin plugin/processor labels Oct 16, 2023
Copy link
Contributor

@powersj powersj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Hipska,

As you know the config should show the default, and you are essentially changing the default if someone takes the default config and modifies it. I don't think we want to do this. If you want to document (i.e. add a comment) about using "source" instead of agent, I think that is cool, but not changing the default.

Thoughts?

@Hipska
Copy link
Contributor Author

Hipska commented Oct 17, 2023

Well, the default isn't actually changed (it is still agent), so current users aren't affected. But as discussed in the original #8011, that default doesn't make much sense as most (if not, all) users need to change it anyway.

FYI, I'm planning to make a new processor which will make the ifname processor deprecated as it will have the same functionality + more.

@powersj
Copy link
Contributor

powersj commented Oct 17, 2023

@Hipska,

This will create confusion. The sample config option no longer shows the default value. The uncommented value is not the default.

What I was trying to say above is that we do have users who use the telegraf config command and/or the sample config to generate their config. They would pick up this change unconsciously, they would not be aware of the change, and it would cause confusion.

@Hipska
Copy link
Contributor Author

Hipska commented Oct 17, 2023

You prefer to have it the other way round? (having example: ... in the comments)

@powersj
Copy link
Contributor

powersj commented Oct 17, 2023

yes

@Hipska Hipska marked this pull request as ready for review October 17, 2023 15:42
@Hipska Hipska requested a review from powersj October 17, 2023 15:43
@telegraf-tiger
Copy link
Contributor

@powersj powersj 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 17, 2023
Copy link
Member

@srebhan srebhan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Hipska!

@srebhan srebhan changed the title feat(processors.ifname): Document better agent tag name docs(processors.ifname): Document better agent tag name Oct 18, 2023
@srebhan srebhan added docs Issues related to Telegraf documentation and configuration descriptions and removed feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin labels Oct 18, 2023
@srebhan srebhan merged commit c142f48 into influxdata:master Oct 18, 2023
@github-actions github-actions bot added this to the v1.28.3 milestone Oct 18, 2023
powersj pushed a commit that referenced this pull request Oct 23, 2023
@Hipska Hipska deleted the feat/processors/ifname/source_tag branch October 27, 2023 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/snmp docs Issues related to Telegraf documentation and configuration descriptions plugin/processor ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants