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: add intel_pmu plugin #9724

Merged
merged 7 commits into from
Nov 23, 2021
Merged

feat: add intel_pmu plugin #9724

merged 7 commits into from
Nov 23, 2021

Conversation

bkotlowski
Copy link
Contributor

Required for all PRs:

  • Updated associated README.md.
  • Wrote appropriate unit tests.
  • Pull request title or commits are in conventional commit format (e.g. feat: or fix:)

resolves #9687

Added an input plugin to exposes Intel PMU (Performance Monitoring Unit) metrics available through Linux Perf subsystem.

@telegraf-tiger
Copy link
Contributor

telegraf-tiger bot commented Sep 6, 2021

Thanks so much for the pull request!
🤝 ✒️ Just a reminder that the CLA has not yet been signed, and we'll need it before merging. Please sign the CLA when you get a chance, then post a comment here saying !signed-cla

@telegraf-tiger telegraf-tiger bot added feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin new plugin plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins labels Sep 6, 2021
@bkotlowski bkotlowski changed the title feat:add intel_pmu plugin feat: add intel_pmu plugin Sep 6, 2021
@telegraf-tiger
Copy link
Contributor

telegraf-tiger bot commented Sep 6, 2021

Thanks so much for the pull request!
🤝 ✒️ Just a reminder that the CLA has not yet been signed, and we'll need it before merging. Please sign the CLA when you get a chance, then post a comment here saying !signed-cla

3 similar comments
@telegraf-tiger
Copy link
Contributor

telegraf-tiger bot commented Sep 7, 2021

Thanks so much for the pull request!
🤝 ✒️ Just a reminder that the CLA has not yet been signed, and we'll need it before merging. Please sign the CLA when you get a chance, then post a comment here saying !signed-cla

@telegraf-tiger
Copy link
Contributor

telegraf-tiger bot commented Sep 7, 2021

Thanks so much for the pull request!
🤝 ✒️ Just a reminder that the CLA has not yet been signed, and we'll need it before merging. Please sign the CLA when you get a chance, then post a comment here saying !signed-cla

@telegraf-tiger
Copy link
Contributor

telegraf-tiger bot commented Sep 7, 2021

Thanks so much for the pull request!
🤝 ✒️ Just a reminder that the CLA has not yet been signed, and we'll need it before merging. Please sign the CLA when you get a chance, then post a comment here saying !signed-cla

@sspaink
Copy link
Contributor

sspaink commented Sep 7, 2021

@bkotlowski are you having trouble signing the CLA? It is the form at the end of this page: https://www.influxdata.com/legal/cla/

@telegraf-tiger
Copy link
Contributor

telegraf-tiger bot commented Sep 7, 2021

Thanks so much for the pull request!
🤝 ✒️ Just a reminder that the CLA has not yet been signed, and we'll need it before merging. Please sign the CLA when you get a chance, then post a comment here saying !signed-cla

@telegraf-tiger
Copy link
Contributor

telegraf-tiger bot commented Sep 7, 2021

Thanks so much for the pull request!
🤝 ✒️ Just a reminder that the CLA has not yet been signed, and we'll need it before merging. Please sign the CLA when you get a chance, then post a comment here saying !signed-cla

@reimda
Copy link
Contributor

reimda commented Sep 7, 2021

@sspaink bklotowski is covered by the Intel corporate CLA. Is there a way to disable the bot or have it check the corporate list?

reimda
reimda reviewed Sep 7, 2021
View reviewed changes
Copy link
Contributor

@reimda reimda left a comment

Choose a reason for hiding this comment

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

There are a lot of go source code files. Many of them don't seem to be telegraf specific but would be useful for any go language program that uses pmu. Would it be more appropriate to put them in a separate library or include them in intel/iaevents instead of putting them directly in telegraf?

plugins/inputs/intel_pmu/README.md
They form a basis for profiling applications to trace dynamic control flow and identify hotspots.

### Configuration:
```toml
Copy link
Contributor

Choose a reason for hiding this comment

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

Make sure to follow the config style guide (https://github.com/influxdata/telegraf/blob/master/docs/developers/SAMPLE_CONFIG.md#style) It looks like you have some config values here that aren't applicable to all users or are defaults that should be commented.

Sync with intel_pmu.go SampleConfig() if you make changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

In terms of defaults, I believe that all of them are commented out. If you mean "events" or "cores"/"sockets" fields, there is a quote in guidance: "If a parameter does not have a default or must frequently be changed then have it uncommented". So there are parameters that need to be frequently changed like a mentioned list of events or the list of cores/sockets. They do not have a specific default value, only we check if the parameters are absent in config, we consider it as a will to obtain all possible events/cores/sockets, but this is not a very common case.

Could you point out which exactly config values here aren't applicable to all users or don't meet the guidelines? Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

@KubaTrojan If they change frequently keeping them uncommented seems fine. I'm not too familiar with Intel PMU, are the events in the config (e.g. INST_RETIRED.ANY) universal? Or do they depend on the event_definitions? If they don't apply to everyone then commenting out the events parameter might be a good idea.

Copy link
Contributor

Choose a reason for hiding this comment

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

They change frequently and that's the most common use case for the plugin is to fill this with event names that need to be counted - so we'd keep that uncommented. But for sure we could explicitly state that fact in the comment above events field.

Added "Event names shall match names from event_definitions files" info.

@sspaink
Copy link
Contributor

sspaink commented Sep 7, 2021

!signed-cla

@bkotlowski bkotlowski force-pushed the intel_pmu branch 5 times, most recently from f3f001a to a392126 Compare September 8, 2021 06:00
@bkotlowski bkotlowski marked this pull request as draft September 8, 2021 07:23
@bkotlowski bkotlowski marked this pull request as ready for review September 20, 2021 11:40
@KubaTrojan
Copy link
Contributor

Hi @reimda. Thanks for your comments! I'm an original author of this plugin, @bkotlowski has created this PR during my holidays, so now I will try to answer your remarks.

@KubaTrojan
Copy link
Contributor

KubaTrojan commented Oct 1, 2021 •
edited
Loading

It seems that the CI failed because "powerdns" plugin's test has failed. Is it a known issue and could we rerun the CI?

@telegraf-tiger
Copy link
Contributor

telegraf-tiger bot commented Oct 5, 2021

Looks like new artifacts were built from this PR. Get them here!

Artifact URLs

@KubaTrojan
Copy link
Contributor

There haven't been more comments for some time. @reimda, do you want to provide more feedback? If not, could we process this PR to be merged?

@reimda reimda 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 Nov 2, 2021
sspaink
sspaink suggested changes Nov 3, 2021
View reviewed changes
Copy link
Contributor

@sspaink sspaink 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 for the contribution @bkotlowski, overall the code looks great and your test coverage is awesome. Although there is a considerable amount of code being added for this plugin, and some of the files look like they have logic that should probably be in the package github.com/intel/iaevents (mainly the logic in: activators.go, config.go, reader.go, resolver.go). I would like to hear your thoughts on what could be moved to help reduce the size of the pull request.

plugins/inputs/intel_pmu/README.md
They form a basis for profiling applications to trace dynamic control flow and identify hotspots.

### Configuration:
```toml
Copy link
Contributor

Choose a reason for hiding this comment

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

@KubaTrojan If they change frequently keeping them uncommented seems fine. I'm not too familiar with Intel PMU, are the events in the config (e.g. INST_RETIRED.ANY) universal? Or do they depend on the event_definitions? If they don't apply to everyone then commenting out the events parameter might be a good idea.

@KubaTrojan
Copy link
Contributor

KubaTrojan commented Nov 5, 2021 •
edited
Loading

Lint Code Base fails on part of the LICENSE_OF_DEPENDENCIES.md file which is not in the scope of proposed changes.

image

Fixed here: #10065

@telegraf-tiger
Copy link
Contributor

telegraf-tiger bot commented Nov 5, 2021

☺️ This pull request doesn't significantly change the Telegraf binary size (less than 1% increase)

📦 Looks like new artifacts were built from this PR.

Expand this list to get them here! 🐯

Artifact URLs

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

@sspaink sspaink merged commit 9480e49 into influxdata:master Nov 23, 2021
reimda pushed a commit that referenced this pull request Dec 1, 2021
@bkotlowski @KubaTrojan @reimda
feat: add intel_pmu plugin (#9724)
6a4e19a 
Co-authored-by: ktrojan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@reimda reimda reimda left review comments

@KubaTrojan KubaTrojan KubaTrojan left review comments

@sspaink sspaink sspaink approved these changes

Assignees
No one assigned
Labels
feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin new 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

New Intel PMU input plugin
4 participants
@bkotlowski @sspaink @reimda @KubaTrojan