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.

Sign up for GitHub

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

Jump to bottom

Switch CPU from field to tag in Interrupts input plugin (#4999) #5024

Merged
merged 4 commits into from
Nov 30, 2018
Merged

Switch CPU from field to tag in Interrupts input plugin (#4999) #5024

merged 4 commits into from
Nov 30, 2018

Conversation

bitcharmer
Copy link
Contributor

@bitcharmer bitcharmer commented Nov 22, 2018 •
edited by glinton
Loading

Required for all PRs:

  • Signed CLA.
  • Associated README.md updated.
  • Has appropriate unit tests.

Resolves #4999

@glinton glinton added feature request Requests for new plugin and for new features to existing plugins feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin and removed feature request Requests for new plugin and for new features to existing plugins labels Nov 27, 2018
@danielnelson
Copy link
Contributor

Looks good, but we will need a few things to manage the change since it can break queries.

Can you add a new option to the plugin called metric_version, the default value should be metric_version = 1 and if the it is either unset or set to 1 then cpu will still be a field with the old format. If the value is greater than or equal to 2 then cpu should be a tag.

In the sample config document the option like:

## Selects the metric version to maintain backwards compatiblity.
##   recommeneded setting for new plugin use is 2; version 1 is deprecated in 1.10
# metric_version = 1

@bitcharmer
Copy link
Contributor Author

Due to your request to preserve old behaviour it was easier to go back to the original design and just add some conditional logic for gathering tags and fields.
The two parallel data formatting options increased testing complexity so I decided to restructure tests and clean up a little.
No need to deprecate original behaviour; both options can coexist.
I decided to introduce tags_as_cpus config parameter, which I think is more descriptive and clearer than version. Also it will allow you to switch the default value easily with one of the future Telegraf releases.
Thanks

@bitcharmer
Copy link
Contributor Author

@danielnelson is this PR acceptable for you guys?

@danielnelson danielnelson added this to the 1.10.0 milestone Nov 30, 2018
danielnelson
danielnelson suggested changes Nov 30, 2018
View reviewed changes
Copy link
Contributor

@danielnelson danielnelson left a comment

Choose a reason for hiding this comment

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

The option method you went for will work, just a few minor comments:

plugins/inputs/interrupts/interrupts.go Outdated Show resolved Hide resolved
plugins/inputs/interrupts/interrupts.go Outdated Show resolved Hide resolved
plugins/inputs/interrupts/interrupts_test.go Outdated Show resolved Hide resolved
plugins/inputs/interrupts/README.md Outdated Show resolved Hide resolved
@bitcharmer
Copy link
Contributor Author

Hi @danielnelson thanks for the review. Each requested change made sense to me so I followed all your suggestions, made changes and pushed.
Let me know if you find anything else.

@danielnelson danielnelson merged commit 9a637ed into influxdata:master Nov 30, 2018
@bitcharmer bitcharmer deleted the bugfix/4999 branch November 30, 2018 22:46
@danielnelson
Copy link
Contributor

I tweaked the option from cpu_as_tags -> cpu_as_tag and updated the readme which didn't have the latest style yet.

otherpirate pushed a commit to otherpirate/telegraf that referenced this pull request Mar 15, 2019
@bitcharmer @otherpirate
Switch CPU from field to tag in interrupts input plugin (influxdata#4999
336fde2 
) (influxdata#5024)
otherpirate pushed a commit to otherpirate/telegraf that referenced this pull request Mar 15, 2019
@bitcharmer @otherpirate
Switch CPU from field to tag in interrupts input plugin (influxdata#4999
2320f60 
) (influxdata#5024)
dupondje pushed a commit to dupondje/telegraf that referenced this pull request Apr 22, 2019
@bitcharmer
Switch CPU from field to tag in interrupts input plugin (influxdata#4999
ce766de 
) (influxdata#5024)
tushar2013
tushar2013 reviewed Jul 17, 2019
View reviewed changes
plugins/inputs/interrupts/README.md Outdated
@@ -8,6 +8,10 @@ The interrupts plugin gathers metrics about IRQs from `/proc/interrupts` and `/p
## To filter which IRQs to collect, make use of tagpass / tagdrop, i.e.
# [inputs.interrupts.tagdrop]
# irq = [ "NET_RX", "TASKLET" ]
#
# To report cpus as tags instead of fields use cpus_as_tags
# cpus_as_tags = false
Copy link

Choose a reason for hiding this comment

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

i am using telegraf version 1.9.2 and this is not working.
Error parsing file.conf, line 76: field corresponding to cpu_as_tag' is not defined in *interrupts.Interrupts'

Copy link
Contributor

Choose a reason for hiding this comment

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

You need at least version 1.10. We try to mark the milestone a feature will be released in on the pull request, you should be able to see it on the right sidebar.

bitcharmer added a commit to bitcharmer/telegraf that referenced this pull request Oct 18, 2019
@bitcharmer
Switch CPU from field to tag in interrupts input plugin (influxdata#4999
348d3fc 
) (influxdata#5024)
athoune pushed a commit to bearstech/telegraf that referenced this pull request Apr 17, 2020
@bitcharmer
Switch CPU from field to tag in interrupts input plugin (influxdata#4999
427651f 
) (influxdata#5024)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tushar2013 tushar2013 tushar2013 left review comments

@danielnelson danielnelson danielnelson approved these changes

Assignees
No one assigned
Labels
feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin
Projects
None yet
Milestone
1.10.0
Development

Successfully merging this pull request may close these issues.
4 participants
@bitcharmer @danielnelson @tushar2013 @glinton