-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
[metricbeat] Add system/entropy metricset #12450
[metricbeat] Add system/entropy metricset #12450
Conversation
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.
Awesome! can you also add a changelog entry?
MetricSetFields: common.MapStr{ | ||
"entropy": common.MapStr{ | ||
"available_bits": entropy, | ||
"pct": float64(entropy) / float64(4096), |
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.
I think this max of 4096 is hardcoded in linux, can you confirm?
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 is!
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.
Is it the value available in /proc/sys/kernel/random/poolsize
? Should we get it from there in case it changes somewhere at some moment?
beb2aad
to
1611337
Compare
@exekias added the changelog, and (hopefully) fixed the fields. |
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.
LGTM
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.
Sorry for the late review 😬, it LGTM in general, the only thing I'd change now is the release level in the docs and maybe the build tags, the rest are open questions.
}, | ||
"system": { | ||
"entropy": { | ||
"entropy": { |
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.
Do we want this double entropy
level?
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.
ups, I missed this when we talked about object vs group, +1 to one level only
MetricSetFields: common.MapStr{ | ||
"entropy": common.MapStr{ | ||
"available_bits": entropy, | ||
"pct": float64(entropy) / float64(4096), |
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.
Is it the value available in /proc/sys/kernel/random/poolsize
? Should we get it from there in case it changes somewhere at some moment?
// KIND, either express or implied. See the License for the | ||
// specific language governing permissions and limitations | ||
// under the License. | ||
|
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.
I guess we should exclude this metricset from non-linux builds.
Yah, @jsoriano I was sorta ambivalent about |
Updated a bunch of things, and ended up using the
|
Okay, latest commit should fix the field formatting issues, and the build issues. |
Oops, forgot to change the tests after I fixed the mapping. |
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.
LGTM, wait for green
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.
👍 LGTM now!
Okay, that should be the last of the bugfixes and updates. |
This PR adds a system/entropy metricset, which monitors and reports the state of
/proc/sys/kernel/random/entropy_avail
Many other monitoring solutions have this, including node_exporter and netdata.This is a (comparatively) small metricset, as there really isn't a lot going on. Most of the other files in
random
aren't particularly interesting as far as monitoring, although y'all are free to disagree if you think we should add others. *bsd also generates entropy differently, meaning there's no equivalent toentropy_avail
, and most of the metrics exposed bykern.random
I didn't think were particularly interesting, although, again, if we decide they're valuable I can add them.Because the full path (
/proc/sys/kernel/random/entropy_avail
) is a tad long-winded, I wasn't sure if we wanted to add a custom config option for the metricset to specify a path toentropy_avail
, so for now I'm piggy-backing off of the systemhostfs
option.