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

fix(inputs.systemd_units): Revert to only gather loaded units by default #15108

Merged
merged 6 commits into from
Apr 12, 2024

Conversation

srebhan
Copy link
Member

@srebhan srebhan commented Apr 4, 2024

Summary

With PR #14814 we also produce metrics for unloaded units. However, for unspecific filters (like * being the default) this touches a lot of files and forces systemd to lookup those unit-files which is expensive.

Therefore, this PR reverts the behavior to pre-v1.30.0 versions and only collects units already loaded by systemd. To allow collecting disabled/unloaded units you can use the new collect_disabled_units.

Additionally to this change, the PR also optimizes the interaction with systemd for cases where non-detailed metrics are requested.

This leads to the following improvements when gathering all units (pattern = "*") in non-detailed mode

goos: linux
goarch: amd64
pkg: github.com/influxdata/telegraf/plugins/inputs/systemd_units
cpu: AMD Ryzen 7 3700X 8-Core Processor 
version performance improvement
v1.30.0 reference 2,021,195,198 ns/op 1.0x
all units including non-loaded 700,185,759 ns/op 2.9x
only loaded units (default) 5,754,030 ns/op 351.3x

Checklist

  • No AI generated code was used in this PR

Related issues

resolves #15104
resolves #15093

@telegraf-tiger telegraf-tiger bot added fix pr to fix corresponding bug plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins labels Apr 4, 2024
@srebhan srebhan added area/system ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. labels Apr 4, 2024
Copy link
Contributor

@powersj powersj left a comment

Choose a reason for hiding this comment

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

Thanks for jumping on this. As a PR this looks fine, but I'm still concerned that the usage is still higher than it used to be. Do we need to have this new method be opt-in?

@srebhan srebhan changed the title fix(inputs.systemd_units): Add option to only gather loaded units fix(inputs.systemd_units): Revert to only gather loaded units by default Apr 11, 2024
@srebhan
Copy link
Member Author

srebhan commented Apr 11, 2024

@1tft FYI: This PR disables collection of non-loaded/disabled units by default, so in case you need them please use the new collect_disabled_units option! Would be cool if you can test this in your environment...

Copy link
Contributor

@powersj powersj left a comment

Choose a reason for hiding this comment

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

Thank you - one question about a println statement?

plugins/inputs/systemd_units/systemd_units_linux.go Outdated Show resolved Hide resolved
@1tft
Copy link

1tft commented Apr 12, 2024

@srebhan Thank you so much for keeping that feature alive. With this build and collect_disabled_units = true we can collect metrics for unloaded units.
Only one remark for Readme: Maybe you should also mention that collect_disabled_units = true activates metrics for systemd_units with state=disabled AND with state=static.

@telegraf-tiger
Copy link
Contributor

Copy link
Contributor

@powersj powersj left a comment

Choose a reason for hiding this comment

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

Thank you!

@powersj powersj removed the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Apr 12, 2024
@powersj powersj removed their assignment Apr 12, 2024
@DStrand1 DStrand1 merged commit 084d49d into influxdata:master Apr 12, 2024
26 checks passed
@github-actions github-actions bot added this to the v1.30.2 milestone Apr 12, 2024
powersj pushed a commit that referenced this pull request Apr 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/system fix pr to fix corresponding bug plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins
Projects
None yet
4 participants