Export systemd's own metrics #709
Conversation
|
I'll also fix #562 while I'm at it. |
|
While fixing #562, I discovered Systemd exports the required metrics in two flavors: Wall clock time, and Monotonic time. I gues Monotonic is preferred, because then we can export it as a counter, right? |
collector/systemd_linux.go
Outdated
| }, | ||
| "NRestarts": metric{ | ||
| desc: prometheus.NewDesc( | ||
| prometheus.BuildFQName(namespace, subsystem, "nrestarts"), |
There was a problem hiding this comment.
Counters should always end in _total, so this would be restarts_total
| func (c *systemdCollector) collectUnitProperiesMetrics(ch chan<- prometheus.Metric) error { | ||
| conn, err := c.newDbus() | ||
| if err != nil { | ||
| return fmt.Errorf("couldn't get dbus connection: %s", err) |
There was a problem hiding this comment.
This should probably start with an uppercase letter.
collector/systemd_linux.go
Outdated
| desc: prometheus.NewDesc( | ||
| prometheus.BuildFQName(namespace, subsystem, "ip_ingress_packets_total"), | ||
| "Ingress packets total", | ||
| []string{"name"}, |
There was a problem hiding this comment.
name is too generic, if this is the name of the unit, I would suggest unit or unit_name.
|
This seems like an interesting feature, but possibly overlaps too much with cAdvisor? We've been avoiding having the node_exporter duplicate cAdvisor's features. |
|
I'm not very familiar with cAdvisor. Does it just export the cgroup metrics? Systemd metrics are oriented to a specific unit file, also not limiited to the cgroups, per se. Also, it's orthogonal to containers. Systemd unit files don't have much to do with containers at all I think so I find it hard to see why it would conflict with cAdvisor. Also, metrics that have nothing to do with cgroups whatsoever are exported as well. Like the timestamp when the latest state change happened (as requested in #562) , and how many times a service restarted. The systemd collector is not enabled by default, so I would not think it's a problem it overlaps with the cadvisor metrics too much? I think there are many people who are not running containers in production, but do use the systemd init system, and would like to have metrics about their unit files exported. |
collector/systemd_linux.go
Outdated
| ), | ||
| valueType: prometheus.CounterValue, | ||
| }, | ||
| "AssertTimestampMonotonic": &metric{ |
There was a problem hiding this comment.
This and the following metrics still need to be renamed.
|
cAdvisor does export metrics from cgroups, as this is how most of the container systems control and account for process groups. Technically, this means that systemd in this case is a type of container system. I don't think we want to reproduce the cgroup data here, but having systemd-specific metrics like start/stop/restart counts would be good. |
|
IMHO, the memory and ingress/egress metrics are also interesting in cases where systemd is not used as a container system, as you just want to monitor resource usage of different processes. Given those resource metrics are exposed by systemd together with other service-specific metrics and don't introduce any new dependencies, I dont really see an issue in exporting them, without having to install a second container-specific collector ;-) |
|
The difficulty is that cgroups ~= containers, and cAdvisor provides these metrics. I don't really like it either, but our policy is to not duplicate separate functionality. Really, from a Prometheus perspective we would want a stand-alone systemd exporter, as it's not exactly a "node" specific thing, but is kinda tied 1:1 per node due to the design. This is a fine line, but only due to policy, not technical. |
|
The point that SuperQ is trying to make is. Because cadvisor exports just the cgroup hierarchy, and every systemd service file has its own cgroup systemd services will actually just show up in them. Also the systemd slices will as well. So you could use the cadvisor collector perfectly well to collect systemd service metrics. Actually the systemd metrics are just a shim of the values that the cgroup exports. So perhaps it's fair to say cadvisor is the better tool for such metrics. Though, systemd does support some extra features, like keeping track of network traffic for a service which cadvisor does not currently do. Also it is really nice that we can already get these metrics with just node_exporter, as we have access to these metrics through dbus already anyway - we just discard them currently. It's one runtime dependency less for people, and they already get some accounting of resources out of the box. So perhaps that's good enough reason to do include systemd metrics. But I'll leave that up to you to decide. I can always adjust the PR to just have the changes needed for #562 and the number of restarts stuff. And then leave out the resource metrics |
|
Edit: the above message I typed parallel to SuperQ's response. We seem to have come to similar conclusions Shall I adjust this PR to just keep track of the restarts and state changes? And then send the rest of the patch to the |
|
@arianvp I think @SuperQ suggested adding restarts and state changes to I did not yet use cAdvisor, by a quick glimpse it seems to be much more than just a prometheus exporter, so I'd be tempted to pull this in just to get resource utilization metrics for systemd units. So maybe moving the systemd restart and state changes stuff into |
|
There has been some talk about building a new cgroups/container metrics exporter that is more light weight and specific than cAdvisor. Going through systemd/dbus just to get cgroup metrics seems like the wrong idea. And a lot of people seem to have problems with cAdvisor. |
|
So adding just the restart and state changes metrics here seems to be the right way to go for this PR. @SuperQ Would a |
|
I think a separate exporter for cgroups would probably be better at this time. I was thinking we may want a a cgroup library, similar to how we have procfs. We have a lot of scope creep issues with the node_exporter as is. |
|
Yes I will see if I can make some time this weekend to clean up the PR :) |
|
Ping, if we can get this cleaned up soon, we can include it in the next release. |
|
I'll be at Fosdem this weekend. So I have 2 days of time to spend on FOSS :) So I'll move this PR to be ready for release then! |
|
Yes, a bunch of us are also at FOSDEM. Look forward to nice graphs of the conference network. |
|
Do you guys have a preferred communication channel for prometheus? Perhaps we can meet up |
|
We have an IRC/Matrix channel: https://prometheus.io/community/ |
|
Last time I touched this code, dbus interface was giving |
|
At least according to src/core/dbus-unit.c, (didn't try it on my own bus) numeric values should have a signature of |
|
Weird...I'll investigate more..
…On Sun, 4 Feb 2018, 17:12 Florian Klink, ***@***.***> wrote:
At least according to src/core/dbus-unit.c
<https://github.com/systemd/systemd/blob/master/src/core/dbus-unit.c#L1135>,
(didn't try it on my own bus) numeric values should have a signature of t,
which translates to SD_BUS_TYPE_UINT64
<https://github.com/systemd/systemd/blob/master/src/systemd/sd-bus-protocol.h#L50>,
and it looks pretty much like this hasn't changed recently.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#709 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAmWo2g1npxTi9ELyjAo0iYnPRwjOQNNks5tRdb7gaJpZM4QBjqv>
.
|
|
It must have been an overdose of Club Mate. I can not reproduce anymore. haha. I'll clean up the PR Today |
First of all, state changes of individual systemd units are now exposed. Things like: When was the unit started, when was it stopped, how many times did it restart before crash? How long until a timer activates. Second of all, behind an optional flag --collector.systemd.accounting additional metrics are defined, which expose systemd's view of CGroup statistics of a unit. These metrics are also exposed in cAdvisor, and to avoid duplicate metrics, this metric is explicitly opt in.
|
I have updated the PR! I have for now hidden the metrics that conflict with However, I'm running into a merge conflict. Apparently a feature landed in master which seems to implement partially what I implemented here, but in a slightly different way. @tomwilkie seems to have implemented exporting some timer metrics, but this PR will expose those as well (under systemd's naming convetions) We should decide whether we would want to revert @tomwilkie 's commits in favor of mine. Also, @tomwilkie seems to have done some refactoring to only keep open one dbus connection. This is also useful for me, and shouldn't be too hard to adapt. But first we'll have to decide if we should rever that commit before I can apply the patchset and start refactoring to adapt the single connection change |
|
What's left is:
Hope to get some feedback |
|
Sorry @arianvp! I didn't see this PR. Let me know if you need a hand.
So were a bunch of us - sorry I didn't get to meet you.
This means this PR export usecs instead of seconds - is this desirable? What does the rest of node_exporter do? |
|
@tomwilkie could you have a look at the above questions I posted? I think we had a race condition because we seem to both have commented at the same time. You might have missed it. |
|
bump @tomwilkie |
|
Regarding the time units: It should be use seconds, using only base metrics is generally best practice in Prometheus. |
|
Going to close this for now. Feel welcome to re-open this once you updated it. |
I bit the bullet and started work on this: https://github.com/arianvp/cgroup-exporter |
Systemd does all kinds of accounting when IOAccounting, CPUAccounting
and IPAccounting are enabled. This commit exposes some of these.
Also, systemd keeps track of how many times services restart, when
their state changes happens, and a lot more. These metrics are very
useful for detecting problems with running applications.
Still need to write tests, and add some missing metrics so not finished yet.
But I welcome any feedback!