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

feat(inputs.kernel): Add Pressure Stall Information #14507

Merged
merged 26 commits into from
Jan 5, 2024

Conversation

iBug
Copy link
Contributor

@iBug iBug commented Dec 26, 2023

Summary

Implemented an input plugin for Pressure Stall Information (Linux v4.20+).

Part of the code was derived from gridscale/linux-psi-telegraf-plugin, which is available under the MIT license.

I will be after this PR as long as I am around.

Checklist

  • No AI generated code was used in this PR

Related issues

Resolves #6760

@iBug iBug changed the title Add Pressure Stall Information (plugins/inputs/psi) feat(plugins/inputs/psi): Add input plugin for Pressure Stall Information Dec 26, 2023
@srebhan srebhan added feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin area/system plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins labels Jan 2, 2024
@srebhan srebhan self-assigned this Jan 2, 2024
@srebhan
Copy link
Member

srebhan commented Jan 2, 2024

Thanks for your contribution @iBug! Before giving this a full review I would like to ask why you do not implement this feature as part of the inputs.kernel plugin? You could easily add this as a new collect ID and make this explorable to all users of kernel which is probably the target audience anyway!?!?

Furthermore, I suggest to merge the pressure and pressureTotal metrics to something like

pressure,resource=cpu some_avg10=1.53,some_avg60=1.87,some_avg300=1.73,some_total=1088168194,full_avg10=0,full_avg60=0,full_avg300=0,full_total=0 1700000000000000000
pressure,resource=memory some_avg10=0,some_avg60=0,some_avg300=0,some_total=3463792,full_avg10=0,full_avg60=0,full_avg300=0,full_total=1429641 1700000000000000000
pressure,resource=io some_avg10=0,some_avg60=0,some_avg300=0,some_total=68568296,full_avg10=0,full_avg60=0,full_avg300=0,full_total=54982338 1700000000000000000

What do you think @iBug?

@iBug
Copy link
Contributor Author

iBug commented Jan 2, 2024

implement this feature as part of the inputs.kernel plugin

Good suggestion. My reasons for not doing this now:

  • I didn't realize this was possible when I started this PR.
  • Maybe this could share some reasoning behind having separate plugins for inputs.system and inputs.swap, the latter of which completely relies on code from the former anyway.
  • github.com/prometheus/procfs (which Telegraf is already using) provides a convenient interface for retrieving these data so I don't have to write yet another parser (albeit easy). I don't see a straightforward way to integrate the current implementation into inputs.kernel.

May I ask what should I do now? Start migrating this entire code to inputs.kernel, wait for more reviews / suggestions / discussions, or prepare a separate PR?

merge the pressure and pressureTotal metrics

  • Merging pressure and pressureTotal makes total sense.
  • Merging some and full tags: Hesitant for two reasons:
    • The kernel provides them on separate lines with the same structure, so logically keeping them apart seems better. This also works better for InfluxDB GROUP BY queries.
    • Specifically for resource=cpu, there is no type=full. Should I just omit the full_* values for resource=cpu?

Just revisited the code:

Technical difficulty in merging pressure and pressureTotal: The total field is a counter type while avg* fields are gauge types. I can add them all onto the same pressure series (in InfluxDB parlance) but I still need to AddCounter and AddGauge separately.

@srebhan
Copy link
Member

srebhan commented Jan 3, 2024

@iBug let me discuss this with the team...

@srebhan
Copy link
Member

srebhan commented Jan 4, 2024

@iBug we discussed this PR yesterday and can live with the current metric format to keep the Gauge vs. Counter properties. However, we do also agree on merging this plugin into inputs.kernel as this is a natural place for this information. I don't think this is too hard as you basically need to move the contents of psi_linux.go there and guard the execution of the function with optCollect containing the feature flag (e.g. "psi")... What do you think?

I can assist you in the merge if you want and permit pushes to your PR branch in your repository...

@iBug iBug marked this pull request as draft January 4, 2024 10:18
@iBug iBug changed the title feat(plugins/inputs/psi): Add input plugin for Pressure Stall Information feat(plugins/inputs/kernel): Add Pressure Stall Information with optCollect = "psi" Jan 4, 2024
@iBug iBug marked this pull request as ready for review January 4, 2024 10:44
@iBug
Copy link
Contributor Author

iBug commented Jan 4, 2024

I just moved all the code into inputs.kernel and I think this PR is good to go.

permit pushes to your PR branch

I forked the repository to an organization named iBug-forks, and it seems like "allow edits from maintainers" is only for user-owned repos. Meanwhile, GitHub doesn't allow changing the incoming branch once a PR is created, so I'm afraid I'll have to commit & push all the code myself.

Fortunately I'll be able to spare some minutes daily in the upcoming days, so you can just comment and I'll handle the code. Alternatively I can close this PR, re-fork and re-submit another PR so you can push.

Copy link
Member

@srebhan srebhan 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 merging this to the kernel plugin @iBug! Two comments from my side...

plugins/inputs/kernel/kernel.go Outdated Show resolved Hide resolved
plugins/inputs/kernel/psi.go Outdated Show resolved Hide resolved
@srebhan srebhan changed the title feat(plugins/inputs/kernel): Add Pressure Stall Information with optCollect = "psi" feat(inputs.kernel): Add Pressure Stall Information Jan 4, 2024
Copy link
Member

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

Very nice @iBug, it's really a joy to work with you! I would like to convert the test to use testutil.RequireMetricsEqual as this prints a nice diff on fail and allows to specify the expected metrics as such. The other two comments are minor...

We are almost there... :-)

plugins/inputs/kernel/psi_test.go Outdated Show resolved Hide resolved
plugins/inputs/kernel/kernel.go Show resolved Hide resolved
plugins/inputs/kernel/kernel.go Outdated Show resolved Hide resolved
iBug and others added 2 commits January 5, 2024 18:15
Co-authored-by: Sven Rebhan <36194019+srebhan@users.noreply.github.com>
@telegraf-tiger
Copy link
Contributor

telegraf-tiger bot commented Jan 5, 2024

Download PR build artifacts for linux_amd64.tar.gz, darwin_arm64.tar.gz, and windows_amd64.zip.
Downloads for additional architectures and packages are available below.

🥳 This pull request decreases the Telegraf binary size by -3.74 % for linux amd64 (new size: 211.6 MB, nightly size 219.9 MB)

📦 Click here to get additional PR build artifacts

Artifact URLs

DEB RPM TAR GZ ZIP
amd64.deb aarch64.rpm darwin_amd64.tar.gz windows_amd64.zip
arm64.deb armel.rpm darwin_arm64.tar.gz windows_arm64.zip
armel.deb armv6hl.rpm freebsd_amd64.tar.gz windows_i386.zip
armhf.deb i386.rpm freebsd_armv7.tar.gz
i386.deb ppc64le.rpm freebsd_i386.tar.gz
mips.deb riscv64.rpm linux_amd64.tar.gz
mipsel.deb s390x.rpm linux_arm64.tar.gz
ppc64el.deb x86_64.rpm linux_armel.tar.gz
riscv64.deb linux_armhf.tar.gz
s390x.deb linux_i386.tar.gz
linux_mips.tar.gz
linux_mipsel.tar.gz
linux_ppc64le.tar.gz
linux_riscv64.tar.gz
linux_s390x.tar.gz

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 very much for the back and forth on this PR!

@powersj powersj added 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 Jan 5, 2024
Copy link
Member

@srebhan srebhan 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 all you work @iBug! It was fun to work with you! Looking forward to your next PR. :-)

@srebhan srebhan merged commit 0052fc3 into influxdata:master Jan 5, 2024
26 checks passed
@github-actions github-actions bot added this to the v1.30.0 milestone Jan 5, 2024
@iBug iBug deleted the inputs/psi branch January 5, 2024 21:56
@ajfriesen
Copy link
Contributor

Thank you very much for integrating the psi plugin we created.

Very much appreciated 🙏

iBug added a commit to iBug/iBug-source that referenced this pull request Jan 8, 2024
iBug pushed a commit to iBugOne/iBugOne.github.io that referenced this pull request Jan 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/system feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add input plugin for PSI (Pressure Stall Information)
4 participants