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.

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

Blacklist systemd scope units #534

Merged
merged 1 commit into from
Mar 23, 2017
Merged

Blacklist systemd scope units #534

merged 1 commit into from
Mar 23, 2017

Conversation

SuperQ
Copy link
Member

@SuperQ SuperQ commented Mar 23, 2017

Blacklist scope units from systemd collector by default.

These units are created with unique IDs programatically0. This leads to
huge cardinality problems.

@SuperQ SuperQ requested a review from discordianfish March 23, 2017 13:02
Blacklist `scope` units from systemd collector by default.

These units are created with unique IDs programatically[0].  This leads to
huge cardinality problems.

[0]: https://www.freedesktop.org/software/systemd/man/systemd.scope.html
@SuperQ SuperQ force-pushed the bjk/systemd_scope branch from 38c9829 to 5f43211 Compare March 23, 2017 13:02
@@ -27,7 +27,7 @@ import (

var (
unitWhitelist = flag.String("collector.systemd.unit-whitelist", ".+", "Regexp of systemd units to whitelist. Units must both match whitelist and not match blacklist to be included.")
unitBlacklist = flag.String("collector.systemd.unit-blacklist", "", "Regexp of systemd units to blacklist. Units must both match whitelist and not match blacklist to be included.")
unitBlacklist = flag.String("collector.systemd.unit-blacklist", ".+\\.scope", "Regexp of systemd units to blacklist. Units must both match whitelist and not match blacklist to be included.")
Copy link
Member

Choose a reason for hiding this comment

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

I guess I'd be explicit and anchor the regexp.

Copy link
Member Author

Choose a reason for hiding this comment

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

The code applies an anchor to the regexp already, do we need to do it here as well?

Copy link
Member

Choose a reason for hiding this comment

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

We generally have anchored regexes everywhere in Prometheus, and the existing one above also assumes that. I'd be for leaving it as is...

@discordianfish
Copy link
Member

Not too familiar with systemd; Any chances people actually want these unit metrics and this change would break it for them?

But beside that, LGTM.

@juliusv
Copy link
Member

juliusv commented Mar 23, 2017

@discordianfish Well, it blew up GitLab's Prometheus server big time, so I think even if someone wants them, it's not good practice. But I think we are generally still ok with breaking things like this in Node Exporter, as long as it's not 1.x yet.

@discordianfish
Copy link
Member

@juliusv Right, breaking is okay - we just need to be aware and mention it in the changelog. Agree that we need to merge it either way.

@juliusv
Copy link
Member

juliusv commented Mar 23, 2017

Definitely. This should be listed as a CHANGE in the next release. Merging then.

@juliusv juliusv merged commit 1f2099b into master Mar 23, 2017
@juliusv juliusv deleted the bjk/systemd_scope branch March 23, 2017 15:57
@grobie
Copy link
Member

grobie commented Mar 23, 2017 via email

tamcore pushed a commit to gitgrave/node_exporter that referenced this pull request Oct 22, 2024
Signed-off-by: taherk <tkkathana@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants