-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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 process resource limits to procstat input #3231
Conversation
fields[prefix+"rlimit_"+name+"_soft"] = rlim.Soft | ||
fields[prefix+"rlimit_"+name+"_hard"] = rlim.Hard | ||
if name != "file_locks" { // gopsutil doesn't currently track the used file locks count | ||
fields[prefix+name] = rlim.Used |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The convention here is that the fields being added are rlimit_FOO_soft
and rlimit_FOO_hard
for which there is a field named FOO
is the current value. Some of these the FOO
field already existed (e.g. memory_rss
), and some it is a new field (e.g. nice_priority
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, wish we didn't use the same field with a different measurement name for everything but that is sticking with the current style.
|
||
Signals related measurement names: | ||
- procstat_[prefix_]signals_pending value=0 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these 3 actually under rlimit_*
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should they be removed here then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, procstat_[prefix_]signals_pending
here is the currently used value. The procstat_[prefix_]rlimit_signals_pending_hard
(and _soft) down in the rlimit_*
section are the limits.
Or maybe I'm misunderstanding what you mean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's actually all under one measurement. That's just how it's documented. I just kept the existing format.
|
||
Signals related measurement names: | ||
- procstat_[prefix_]signals_pending value=0 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
Ok, I've updated enough to where it can go through formal review (out of design). Just made the tests pass (circleci failure seems unrelated), and updated docs. |
- procstat_[prefix_]rlimit_signals_pending_hard value=1758 | ||
- procstat_[prefix_]rlimit_signals_pending_soft value=1758 | ||
|
||
*NOTE: Due to a limitation in an underlying library Telegraf uses, any resource limit > 2147483647 will be misreported as 2147483647.* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, and I just remembered to add this little note. I think this is a major issue. But the gopsutil
owners won't fix the issue because it might break consumers. (which I personally think is a lousy excuse as that's what vendoring is meant to solve)
This adds resource limits to the procstat input plugin.
closes #2617
Required for all PRs: