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.

Sign up for GitHub

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

Jump to bottom

Feat/systemd summary #765

Merged
merged 1 commit into from
Jan 4, 2018
Merged

Feat/systemd summary #765

merged 1 commit into from
Jan 4, 2018

Conversation

sevagh
Copy link
Contributor

@sevagh sevagh commented Dec 20, 2017

Here's the topic in the Google Groups to discuss this: https://groups.google.com/forum/?fromgroups#!topic/prometheus-developers/uhpXqdS0r8A

By using ./node_exporter --collector.systemd --collector.systemd.summary, you can get simplified/summarized systemd stats:

# HELP node_systemd_unit_state_summary Summary of systemd unit states
# TYPE node_systemd_unit_state_summary gauge
node_systemd_unit_state_summary{state="activating"} 0
node_systemd_unit_state_summary{state="active"} 275
node_systemd_unit_state_summary{state="deactivating"} 0
node_systemd_unit_state_summary{state="failed"} 0
node_systemd_unit_state_summary{state="inactive"} 70
node_systemd_unit_state_summary{state="total"} 345

SuperQ
SuperQ requested changes Dec 20, 2017
View reviewed changes
collector/systemd_linux.go Outdated
func summarizeUnits(units []dbus.UnitStatus) map[string]float64 {
summarized := make(map[string]float64)

summarized["total"] = float64(len(units))
Copy link
Member

Choose a reason for hiding this comment

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

Total should not be included, as totals are a function of sum() in PromQL.

collector/systemd_linux.go Outdated
@@ -57,23 +60,36 @@ func NewSystemdCollector() (Collector, error) {
"Whether the system is operational (see 'systemctl is-system-running')",
nil, nil,
)
summaryDesc := prometheus.NewDesc(
prometheus.BuildFQName(namespace, subsystem, "unit_state_summary"),
Copy link
Member

Choose a reason for hiding this comment

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

I would suggest simply calling this units. So the metric name would be node_systemd_units

@SuperQ
Copy link
Member

SuperQ commented Dec 20, 2017

I like the idea, but I'm conflicted about the control flag. I need to think about this.

@SuperQ
Copy link
Member

SuperQ commented Dec 20, 2017

I had a talk with @brian-brazil. What do you think of doing the flags like this.

Instead of having a bool for summary, we always collect the summary when the collector is enabled. And use the whitelist to disable collection of the per-unit metrics.

./node_exporter --collector.systemd --collector.systemd.unit-whitelist=''

This would be a little unintuitive, but allow for a bit more flexibility where you might want the unit state summary and also a very small whitelist of units.

@sevagh
Copy link
Contributor Author

sevagh commented Dec 20, 2017

Sounds OK to me. I'll get those changes in. It should simplify the code also.

@sevagh
Copy link
Contributor Author

sevagh commented Dec 20, 2017 •
edited
Loading

A little bit of the code logic had to change to only call getAllUnits once and use that output for both summary and regular metrics.

@sevagh
Copy link
Contributor Author

sevagh commented Dec 20, 2017

Squashed my commits.

SuperQ
SuperQ reviewed Dec 20, 2017
View reviewed changes
collector/systemd_linux.go Outdated
}

summary := summarizeUnits(allUnits)
if err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

This looks like leftover, summarizeUnits() doesn't return an err.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, there's another one for filterUnits as well a few lines after. Will clean both.

@SuperQ
Copy link
Member

SuperQ commented Dec 20, 2017

Looks great! One minor comment.

@sevagh
Copy link
Contributor Author

sevagh commented Dec 20, 2017

Did I break something?

@SuperQ
Copy link
Member

SuperQ commented Dec 20, 2017

No, it looks like our freebsd build slave ran out of disk again. You can ignore that.

@sevagh
Copy link
Contributor Author

sevagh commented Dec 21, 2017

Alright then, thanks for the feedback. Looks OK now?

SuperQ
SuperQ approved these changes Dec 21, 2017
View reviewed changes
Copy link
Member

@SuperQ SuperQ left a comment

Choose a reason for hiding this comment

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

LGTM

@SuperQ
Copy link
Member

SuperQ commented Dec 21, 2017

We fixed the broken test server, now all checks pass.

@SuperQ SuperQ requested a review from juliusv December 21, 2017 14:48
discordianfish
discordianfish approved these changes Jan 4, 2018
View reviewed changes
Copy link
Member

@discordianfish discordianfish left a comment

Choose a reason for hiding this comment

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

LGTM

@discordianfish discordianfish merged commit 4329b0a into prometheus:master Jan 4, 2018
oblitorum pushed a commit to shatteredsilicon/node_exporter that referenced this pull request Apr 9, 2024
@sevagh @oblitorum
Add summary metrics for systemd exporter (prometheus#765)
d25bf6e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SuperQ SuperQ SuperQ approved these changes

@discordianfish discordianfish discordianfish approved these changes

@juliusv juliusv Awaiting requested review from juliusv

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sevagh @SuperQ @discordianfish