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

Ethtool Input Plugin (#5864) #5865

Merged
merged 4 commits into from
Nov 8, 2019
Merged

Ethtool Input Plugin (#5864) #5865

merged 4 commits into from
Nov 8, 2019

Conversation

philippreston
Copy link
Contributor

@philippreston philippreston commented May 16, 2019 •
edited by glinton
Loading

Resolves #5864

Added new input plugin that will support pulling of data from
ethernet devices via ethtool

Required for all PRs:

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

@philippreston philippreston changed the title Ethtool Input Plugin (Issue #5864) Ethtool Input Plugin (#5864) May 16, 2019
@philippreston philippreston mentioned this pull request May 16, 2019
Closed
@smalenfant
Copy link

I would love to see this make it in. We have broadcom 10Gbps NIC that don't report CRC errors in the standard net interfaces which would like to catch with this.

@philippreston
Copy link
Contributor Author

Fixed conflict with master for Gopkg.toml following the 1.11 release

@smalenfant
Copy link

Again, would love to see this in 1.11. We have netword card drivers that are not reporting back to proper RX CRCs errors into their drivers. Working on that front as well. Thanks @philippreston, really useful.

glinton
glinton suggested changes Jul 23, 2019
View reviewed changes
Copy link
Contributor

@glinton glinton left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks like a pretty good start to a useful plugin

plugins/inputs/ethtool/ethtool_linux.go Outdated Show resolved Hide resolved
plugins/inputs/ethtool/ethtool_linux.go Outdated Show resolved Hide resolved
plugins/inputs/ethtool/ethtool_linux.go Outdated Show resolved Hide resolved
plugins/inputs/ethtool/ethtool_linux.go Outdated Show resolved Hide resolved
plugins/inputs/ethtool/ethtool.go Outdated Show resolved Hide resolved
plugins/inputs/ethtool/ethtool.go Outdated Show resolved Hide resolved
plugins/inputs/ethtool/README.md Outdated Show resolved Hide resolved
plugins/inputs/ethtool/README.md Outdated Show resolved Hide resolved
plugins/inputs/ethtool/README.md Outdated Show resolved Hide resolved
@philippreston
Copy link
Contributor Author

Thanks Greg - I'll rattle through feedback later today / tomorrow and push up changes.

philippreston
philippreston commented Aug 1, 2019
View reviewed changes
Copy link
Contributor Author

@philippreston philippreston left a comment

Choose a reason for hiding this comment

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

Hey

Slightly later than planned. I have updated as per code review. Let me know if there is anything else you need.

Again - many thanks

Phil

@philippreston
Copy link
Contributor Author

@glinton not able to push this back for review - so just a ping to indicate the changes are complete.

Thanks

Phil

@philippreston
Copy link
Contributor Author

Hey @glinton

Just wanting to confirm that you saw changes ok?

Many thanks

Phil

@philippreston
Copy link
Contributor Author

Hey

Just checking in on the updates to the pull request.

Thanks

Phil

@danielnelson danielnelson self-requested a review August 24, 2019 05:00
@danielnelson
Copy link
Contributor

Thanks @philippreston, we'll take another look as soon as we can. Please ping us again if we haven't updated the review before the 6th.

@glinton
Copy link
Contributor

glinton commented Aug 26, 2019

Also, try not to force push once a review has started as re-reviewing the entire pr is more time consuming. thanks.

@philippreston
Copy link
Contributor Author

Apologies - noted for future.

Thanks

Phil

glinton
glinton suggested changes Sep 6, 2019
View reviewed changes
Copy link
Contributor

@glinton glinton left a comment

Choose a reason for hiding this comment

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

Thanks

plugins/inputs/ethtool/ethtool_linux.go Outdated Show resolved Hide resolved
plugins/inputs/ethtool/ethtool_linux.go Show resolved Hide resolved
@philippreston
Copy link
Contributor Author

Hey

Apologies took so long. Changes made.

Many thanks

Phil

glinton
glinton approved these changes Sep 19, 2019
View reviewed changes
Copy link
Contributor

@glinton glinton left a comment

Choose a reason for hiding this comment

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

Thanks

@philippreston
Copy link
Contributor Author

Hey

Just wanted to check in and see whether this was going to make the 1.13 release, not that the PR is ok?

Many thanks

Phil

@glinton
Copy link
Contributor

glinton commented Oct 10, 2019

It doesn't look like it's on a milestone yet, let me mark this so we can review and put it on one. Thanks for the reminder

@glinton glinton added the ready label Oct 10, 2019
danielnelson
danielnelson suggested changes Oct 15, 2019
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.

Looking good, should be no problem to complete for 1.13.0.

plugins/inputs/ethtool/ethtool.go Outdated Show resolved Hide resolved
plugins/inputs/ethtool/ethtool_linux.go Outdated Show resolved Hide resolved
@danielnelson danielnelson added this to the 1.13.0 milestone Oct 15, 2019
@philippreston
Copy link
Contributor Author

Cool - thanks for that.

I'll get changes made before end of week.

Phil

@philippreston
Copy link
Contributor Author

Hey

Sorry was caught up in other stuff last few weeks.

Have committed those last few bits of feedback.

Many thanks

Phil

philippreston and others added 4 commits November 8, 2019 11:14
@philippreston @danielnelson
Ethtool Input Plugin (Issue influxdata#5864)
83db958 
Added new input plugin that will support pulling of data from
ethernet devices via ethtool
@philippreston @danielnelson
Ethtool Input Plugin
6daa5a8 
Moved waitGroup out of struct and separated std and non-std imports.
@philippreston @danielnelson
Ethtool Input Plugin
ef4c641 
Added Init function and moved EthTool initialisation to that allowing
passing of error into telegraf.

Removed DriverName as a flag.
@danielnelson
Remove driver_name_tag from sample config
7119757
@danielnelson
Copy link
Contributor

FYI I did a little bit of fumbling resolving the merge conflict, and needed to rebase your branch. Merging as soon as the tests complete.

@danielnelson danielnelson merged commit d9ddd95 into influxdata:master Nov 8, 2019
athoune pushed a commit to bearstech/telegraf that referenced this pull request Apr 17, 2020
@philippreston
Add ethtool input plugin (influxdata#5865)
072f4ef
idohalevi pushed a commit to idohalevi/telegraf that referenced this pull request Sep 29, 2020
@philippreston
Add ethtool input plugin (influxdata#5865)
efc2b48
arstercz pushed a commit to arstercz/telegraf that referenced this pull request Mar 5, 2023
@philippreston @danielnelson
Add ethtool input plugin (influxdata#5865)
a5ce43b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@glinton glinton glinton approved these changes

@danielnelson danielnelson danielnelson approved these changes

Assignees

@philippreston philippreston

Labels
new plugin
Projects
None yet
Milestone
1.13.0
Development

Successfully merging this pull request may close these issues.

Add Ethtool Input Plugin
4 participants
@philippreston @smalenfant @danielnelson @glinton