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 optional usage_active and time_active CPU metrics #2943

Merged

Conversation

bobmshannon
Copy link
Contributor

@bobmshannon bobmshannon commented Jun 20, 2017

An approach for solving #2710. Uses the existing code, but adds two new metrics with the assumption that %busy=100-%idle, and time busy=total time-time idle.

Required for all PRs:

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

@phemmer
Copy link
Contributor

phemmer commented Jun 21, 2017

My $0.02:

  • I personally dislike computed fields being emitted. Computed fields, especially ones that are cheap to compute, just take up space in the database, and reduce its performance.
  • I think the "busy" field would be confusing. Because we're simply doing busy=100-idle, things like steal time, and iowait, end up as "busy". This really muddies the definition of what "busy" is. With steal time, busy would be high, but the CPU time isn't being spent inside the OS being monitored. With iowait, busy would be high, but the CPU is really idle as it can be scheduled to run other tasks while the IO operations are outstanding, so it's not really busy.

@bobmshannon
Copy link
Contributor Author

@phemmer Good points. What would your thoughts be on tweaking the formula for busy to be busy=100-idle-iowait, and documenting that this field is measuring meaningful work performed by the CPU?

The first point about computed fields is also a good one, and I'm not sure if this is something that is typically avoided in other input plugins as a matter of style/convention/agreed upon best practice.

@danielnelson
Copy link
Contributor

In general I agree with @phemmer, we should not have values that are computed based on other fields. However, this metric has been frequently requested since it's removal, since it is the most user friendly way of viewing cpu use. This is why I think we should probably make an exception here.

Maybe we should take a page from collectd and add an "active" state if a report_active option is set to true? The active state would be defined in the same way collectd defines it. The default value would be false. https://collectd.org/documentation/manpages/collectd.conf.5.shtml#plugin_cpu

@bobmshannon
Copy link
Contributor Author

bobmshannon commented Jun 23, 2017

I like the configurable option (false by default?) because it allows someone optionally to trade off a small amount of overhead for the convenience of having this computed field. Seems very flexible.

Anyway, I've updated this PR accordingly to reflect that approach and have also addressed the iowait issue mentioned in the comments above.

@bobmshannon bobmshannon changed the title Add usage_busy and time_busy CPU metrics Add optional usage_active and time_active CPU metrics Jun 23, 2017
@@ -116,6 +129,11 @@ func totalCpuTime(t cpu.TimesStat) float64 {
return total
}

func activeCpuTime(t cpu.TimesStat) float64 {
active := totalCpuTime(t) - t.Idle - t.Iowait
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't subtract Iowait, to match collectd.

@bobmshannon
Copy link
Contributor Author

@danielnelson the iowait subtraction issue should be addressed

@danielnelson danielnelson merged commit d217cdc into influxdata:master Jun 26, 2017
@danielnelson
Copy link
Contributor

Thanks!

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.

3 participants