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

Add timeout option to IPMI sensor input plugin #2818

Merged
merged 1 commit into from
May 22, 2017

Conversation

m4ce
Copy link
Contributor

@m4ce m4ce commented May 16, 2017

Solves #2817.

Required for all PRs:

  • CHANGELOG.md updated (we recommend not updating this until the PR has been approved by a maintainer)
  • Sign CLA (if not already signed)
  • README.md updated (if adding a new plugin)

@m4ce m4ce force-pushed the 2817 branch 3 times, most recently from 0f89a81 to 8711d3c Compare May 16, 2017 21:04
@danielnelson
Copy link
Contributor

Looks good, can you add it to the CHANGELOG?

@m4ce
Copy link
Contributor Author

m4ce commented May 17, 2017

@danielnelson, I've updated the CHANGELOG as requested.

@danielnelson
Copy link
Contributor

Can you change the default to 5s as well? Since the default interval is 10s we should keep it under that.

@m4ce
Copy link
Contributor Author

m4ce commented May 18, 2017

The problem with 5s is that is just too low and users would have to often override that. If you are okay with it, I will change it back to 5s.

@danielnelson
Copy link
Contributor

closes #1565

@danielnelson
Copy link
Contributor

How much time do we need to query a single localhost server? If we only need >5s for mutliple remote servers than I am comfortable requiring the user to increase the timeout.

@m4ce
Copy link
Contributor Author

m4ce commented May 19, 2017

So, via the IPMI kernel interface (which is local), it takes more than 5 seconds on some of my boxes.

# time ipmitool sdr >/dev/null

real    0m8.026s
user    0m0.000s
sys     0m0.003s

I'd imagine over the network should be more expensive.

@danielnelson
Copy link
Contributor

So maybe we should set the timeout = 20s and also add interval = 30s, similar to what we did for the cloudwatch input.

@m4ce
Copy link
Contributor Author

m4ce commented May 21, 2017

@danielnelson - I've made the changes as requested. Let me know if there's anything that needs fixing.

@danielnelson danielnelson added this to the 1.4.0 milestone May 22, 2017
@danielnelson danielnelson merged commit 7d198f0 into influxdata:master May 22, 2017
vlamug pushed a commit to vlamug/telegraf that referenced this pull request May 30, 2017
jeichorn pushed a commit to jeichorn/telegraf that referenced this pull request Jul 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants