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.intel_powerstat): Extract business logic to external library #14363

Merged
merged 6 commits into from
Dec 7, 2023

Conversation

ggalieroc
Copy link
Contributor

Summary

The Intel PowerStat plugin has recently expanded significantly, primarily due to the sophisticated domain logic required to read various metrics from a variety of processor models.
In order to simplify and enhance the readability of the plugin, we (Intel) have extracted most of the mentioned logic into an external (and publicly accessible) library at https://github.com/intel/powertelemetry.
We would like to add this library as a dependency and utilize it in this plugin for metric reading purposes.

This PR also introduces new features:

  • New cpu metrics: cpu_c0_substate_c01_percent, cpu_c0_substate_c02_percent and cpu_c0_substate_c0_wait_percent added (based on kernel's perf interface, currently available only for Intel Sapphire Rapids X and Intel Emerald Rapids X).
  • For kernel version >= 5.18 uncore_frequency_mhz_cur metric is exposed via intel-uncore-frequency kernel module and will be read from there. For older kernels, it is still accessed via msr kernel module.
  • included_cpus and excluded_cpus configuration fields are introduced - to gather cpu_metrics only from configured cpus.
  • msr_read_timeout configuration field added - to set the optional timeout duration for MSR reading.

Checklist

  • No AI generated code was used in this PR

Related issues

Also this PR:

@telegraf-tiger telegraf-tiger bot added 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 labels Nov 28, 2023
@ggalieroc ggalieroc marked this pull request as draft November 28, 2023 18:44
@ggalieroc ggalieroc marked this pull request as ready for review November 28, 2023 18:48
Copy link
Contributor

@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 your contribution and the nice cleanup of this plugin @ggalieroc! I do have some comments...

plugins/inputs/intel_powerstat/intel_powerstat_notlinux.go Outdated Show resolved Hide resolved
plugins/inputs/intel_powerstat/fetcher.go Show resolved Hide resolved
plugins/inputs/intel_powerstat/README.md Outdated Show resolved Hide resolved
plugins/inputs/intel_powerstat/metrics.go Outdated Show resolved Hide resolved
plugins/inputs/intel_powerstat/options.go Outdated Show resolved Hide resolved
plugins/inputs/intel_powerstat/intel_powerstat.go Outdated Show resolved Hide resolved
plugins/inputs/intel_powerstat/intel_powerstat.go Outdated Show resolved Hide resolved
plugins/inputs/intel_powerstat/intel_powerstat.go Outdated Show resolved Hide resolved
@srebhan srebhan self-assigned this Nov 29, 2023
@ggalieroc
Copy link
Contributor Author

Thanks for your contribution and the nice cleanup of this plugin @ggalieroc! I do have some comments...

Thank you for your review comments!. I have replied to them. Please allow me some time to add fixes.

@Hipska Hipska self-assigned this Nov 30, 2023
plugins/inputs/intel_powerstat/sample.conf Outdated Show resolved Hide resolved
plugins/inputs/intel_powerstat/README.md Show resolved Hide resolved
plugins/inputs/intel_powerstat/README.md Show resolved Hide resolved
plugins/inputs/intel_powerstat/README.md Outdated Show resolved Hide resolved
plugins/inputs/intel_powerstat/README.md Outdated Show resolved Hide resolved
plugins/inputs/intel_powerstat/README.md Outdated Show resolved Hide resolved
plugins/inputs/intel_powerstat/intel_powerstat.go Outdated Show resolved Hide resolved
plugins/inputs/intel_powerstat/intel_powerstat.go Outdated Show resolved Hide resolved
plugins/inputs/intel_powerstat/intel_powerstat.go Outdated Show resolved Hide resolved
plugins/inputs/intel_powerstat/intel_powerstat.go Outdated Show resolved Hide resolved
plugins/inputs/intel_powerstat/intel_powerstat.go Outdated Show resolved Hide resolved
plugins/inputs/intel_powerstat/intel_powerstat.go Outdated Show resolved Hide resolved
plugins/inputs/intel_powerstat/intel_powerstat.go Outdated Show resolved Hide resolved
@ggalieroc
Copy link
Contributor Author

Thanks for your feedback @Hipska and @srebhan. I have addressed your comments and would appreciate another round of reviews.

Copy link
Contributor

@Hipska Hipska left a comment

Choose a reason for hiding this comment

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

Great improvements, only minor comments..

plugins/inputs/intel_powerstat/sample.conf Outdated Show resolved Hide resolved
@ggalieroc ggalieroc requested a review from Hipska December 4, 2023 11:17
@telegraf-tiger
Copy link
Contributor

telegraf-tiger bot commented Dec 5, 2023

@ggalieroc
Copy link
Contributor Author

Hi @srebhan, I addressed your comments, :). If you could please have a look would be great.

Copy link
Contributor

@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 your work @ggalieroc and thanks for the valuable input @p-zak!

@srebhan srebhan 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 Dec 6, 2023
@srebhan srebhan assigned powersj and unassigned srebhan Dec 6, 2023
@Hipska Hipska requested a review from p-zak December 7, 2023 11:56
@p-zak p-zak removed their request for review December 7, 2023 12:14
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 putting this up.

I think we are all in agreement that in future PRs we would like to see the tests pull data from test files like we do in other plugins. I would also love to see smaller PRs ;) There is no way that any of us were able to thoroughly and confidently review 11k+ lines.

@powersj powersj merged commit 5d59832 into influxdata:master Dec 7, 2023
23 checks passed
@github-actions github-actions bot added this to the v1.29.0 milestone Dec 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
5 participants