Add support for attaching aggregation labels to sysctl objects.
ClosedPublic
Actions

Authored by ed on Dec 13 2016, 2:55 PM.

Details

Reviewers

peterj
cem
allanjude
imp
jonathan
adrian

Group Reviewers

manpages

Commits

rS310056: Let all FEATURE()s use the same Prometheus metric.
rS310055: Add a "device_index" label to all sysctls under dev.$driver.$index.
rS310054: Attach a "thermal_zone" label to the ACPI thermal zone sysctls.
rS310053: Add labels to sysctls related to clocks.
rS310052: Add label annotations to CAM sysctls.
rS310051: Add support for attaching aggregation labels to sysctl objects.

Summary

I'm currently working on writing a metrics exporter for the Prometheus
monitoring system to provide access to sysctl metrics. Prometheus and
sysctl have some structural differences:

sysctl is a tree of string component names.
Prometheus uses a flat namespace for its metrics, but allows you to attach labels with values to them, so that you can do aggregation.

An initial version of my exporter simply translated

hw.acpi.thermal.tz1.temperature

sysctl_hw_acpi_thermal_tz1_temperature_celcius

while we should ideally have

sysctl_hw_acpi_thermal_temperature_celcius{thermal_zone="tz1"}

allowing you to graph all thermal zones on a system in one go.

The change presented in this commit adds support for accomplishing this,
by providing the ability to attach labels to nodes. In the example I
gave above, the label "thermal_zone" would be attached to "tz1". As this
is a feature that will only be used very rarely, I decided to not change
the KPI too aggressively.

While there, I've also added labels to places where I thought it already
makes sense. Some of these are actually needed to make the metrics
exporter work at all, as they get rid of some metrics having a dash in
their name (which Prometheus doesn't allow).

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 6226
Build 6469: arc lint + arc unit

Event Timeline

ed updated this revision to Diff 22872.Dec 13 2016, 2:55 PM

ed retitled this revision from to Add support for attaching aggregation labels to sysctl objects..

ed updated this object.

ed edited the test plan for this revision. (Show Details)

ed added reviewers: allanjude, jonathan, peterj, imp, adrian.

Herald added a reviewer: manpages. · View Herald TranscriptDec 13 2016, 2:55 PM

rpokala added a subscriber: rpokala.Dec 13 2016, 6:39 PM

rpokala added inline comments.

share/man/man9/sysctl.9
28	s/2015/2016/ # or maybe 2017 by the time this gets committed ;-)
share/man/man9/sysctl_add_oid.9
30	s/2015/2016/
sys/sys/sysctl.h
277	Doesn't the #define for SYSCTL_OID_WITH_LABEL need to come before its use in the #define for SYSCTL_OID?
304	Same here - SYSCTL_NODE_WITH_LABEL is used in the definition of SYSCTL_NODE before SYSCTL_NODE_WITH_LABEL is defined.
313	And again.

Use the proper year.

Spotted by: rpokala

Herald edited edge metadata. · View Herald TranscriptDec 13 2016, 6:47 PM

ed marked 4 inline comments as done.Dec 13 2016, 6:50 PM

ed added inline comments.

sys/sys/sysctl.h
277	Well, that's intersting about macros: it's jut flat substitution. This means that ordering is irrelevant. The entire expansion is determined at invocation. This means that the existing ordering is all right. It's actually intentional, as I'd rather place the `_WITH_LABEL()` versions below the more commonly used ones. Your thoughts?

I'd prefer the use modifications to be a separate commit from the new API.

It looks like these label strings are just blindly passed up to userspace, so it's really up to userspace to do anything with them. Does your userspace program handle multiple labels on a given sysctl, or is it a single tag deal?

This revision is now accepted and ready to land.Dec 13 2016, 7:04 PM

In D8775#181837, @cem wrote:

I'd prefer the use modifications to be a separate commit from the new API.

Will do!

It looks like these label strings are just blindly passed up to userspace, so it's really up to userspace to do anything with them. Does your userspace program handle multiple labels on a given sysctl, or is it a single tag deal?

It should deal with that perfectly fine. Pretty nice if you want to keep track of a histogram of packet size distribution per NIC:

net_received_packets_size_bytes_bucket{interface="em0",le="64"} 1200
net_received_packets_size_bytes_bucket{interface="em0",le="128"} 1400
net_received_packets_size_bytes_bucket{interface="em0",le="256"} 1400
net_received_packets_size_bytes_bucket{interface="em0",le="512"} 2000
net_received_packets_size_bytes_bucket{interface="em0",le="1024"} 3000
net_received_packets_size_bytes_bucket{interface="em0",le="+Inf"} 4000

Closed by commit rS310051: Add support for attaching aggregation labels to sysctl objects. (authored by ed). · Explain WhyDec 14 2016, 12:47 PM

This revision was automatically updated to reflect the committed changes.

wblock added a subscriber: wblock.Dec 16 2016, 7:32 PM

wblock added inline comments.

head/share/man/man9/sysctl.9
482 ↗	(On Diff #22911)	This sentence doesn't need to repeat that the label is attached to an OID, that was done in the previous sentence. Labels can make exporting sysctl data to monitoring systems that support aggregations through labels, like Prometheus.