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.p4runtime): Implementation of P4Runtime input plugin #12473

Merged
merged 4 commits into from
Feb 7, 2023

Conversation

jokuniew
Copy link
Contributor

@jokuniew jokuniew commented Jan 9, 2023

Co-Authored-By: Jakub Sikorski jakub.sikorski@intel.com

Required for all PRs

resolves #12470

P4Runtime is a gRPC server whose API is used to interact with a control plane specification for controlling the data plane elements of a device defined or described by a P4 program.

P4Runtime Specification: https://p4.org/p4-spec/docs/p4runtime-spec-working-draft-html-version.html

P4 programmable pipelines provide the flexibility of adding custom counters that can help observe the pipeline's operation. We provide the means to evaluate the effectiveness of those pipelines via exposed counters. That is the motivation why we would like to have a Telegraf input plugin that exposes P4Runtime Counters metrics.

@telegraf-tiger
Copy link
Contributor

telegraf-tiger bot commented Jan 9, 2023

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

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.

Hi,

Thank you for the PR! I have done an initial first pass through this. While I have seen your issue, my high-level question is, what is P4Runtime used for? What devices, systems, etc. use this? Trying to understand what the target user is and what we would need to support this.

Thanks!

plugins/inputs/p4runtime/README.md Outdated Show resolved Hide resolved
plugins/inputs/p4runtime/README.md Outdated Show resolved Hide resolved
plugins/inputs/p4runtime/README.md Outdated Show resolved Hide resolved
plugins/inputs/p4runtime/p4runtime.go Outdated Show resolved Hide resolved
plugins/inputs/p4runtime/p4runtime.go Outdated Show resolved Hide resolved
plugins/inputs/p4runtime/p4runtime.go Show resolved Hide resolved
plugins/inputs/p4runtime/p4runtime.go Show resolved Hide resolved
plugins/inputs/p4runtime/README.md Outdated Show resolved Hide resolved
plugins/inputs/p4runtime/README.md Show resolved Hide resolved
plugins/inputs/p4runtime/README.md Outdated Show resolved Hide resolved
@jokuniew
Copy link
Contributor Author

Hi,

Thank you for the PR! I have done an initial first pass through this. While I have seen your issue, my high-level question is, what is P4Runtime used for? What devices, systems, etc. use this? Trying to understand what the target user is and what we would need to support this.

Thanks!

Added brief info about P4 and P4Runtime at the beginning of README.md

@jokuniew jokuniew changed the title Implementation of P4Runtime input plugin feat(inputs.p4runtime): Implementation of P4Runtime input plugin Jan 24, 2023
@jokuniew
Copy link
Contributor Author

I don't know what's going on with run-readme-linter job, but for this readme result is:
Pass plugins/inputs/p4runtime/README.md

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 the updates! Think I am good except for the coutner_index as a tag. Maybe if I saw more example data that you are parsing, but as of now having an 'index' as a tag is a red flag.

Thanks again!

@powersj powersj self-assigned this Jan 26, 2023
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!

@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 Feb 1, 2023
@powersj powersj assigned srebhan and unassigned powersj Feb 1, 2023
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.

Very nice code @jokuniew! I do have some smaller comments though. Can you please take a look!?

plugins/inputs/p4runtime/p4runtime.go Outdated Show resolved Hide resolved
plugins/inputs/p4runtime/p4runtime.go Outdated Show resolved Hide resolved
plugins/inputs/p4runtime/p4runtime.go Outdated Show resolved Hide resolved
plugins/inputs/p4runtime/p4runtime_test.go Outdated Show resolved Hide resolved
plugins/inputs/p4runtime/p4runtime_test.go Outdated Show resolved Hide resolved
plugins/inputs/p4runtime/p4runtime_test.go Outdated Show resolved Hide resolved
plugins/inputs/p4runtime/p4runtime_test.go Outdated Show resolved Hide resolved
jokuniew and others added 4 commits February 3, 2023 12:47
Co-Authored-By: Jakub Sikorski <jakub.sikorski@intel.com>
* rename address -> endpoint
* rename counter_names -> counter_names_include
* remove device_id string to int parsing
* few README fixes, lists & more explanation

Co-Authored-By: Jakub Sikorski <jakub.sikorski@intel.com>
* remove line with CounterNamesInclude in init()
* change testutils methods from Equal to Empty etc.
* move Add to wg inside loop
* unwrap method in tests

Co-Authored-By: Jakub Sikorski <jakub.sikorski@intel.com>
@telegraf-tiger
Copy link
Contributor

telegraf-tiger bot commented Feb 3, 2023

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

⚠️ This pull request increases the Telegraf binary size by 2.86 % for linux amd64 (new size: 166.4 MB, nightly size 161.7 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

@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.

Looks good to me. Thanks for the fast and nice update @jokuniew!

@srebhan srebhan 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 Feb 7, 2023
@srebhan srebhan merged commit 0f2db7a into influxdata:master Feb 7, 2023
@srebhan srebhan added this to the v1.26.0 milestone Jun 21, 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 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
Development

Successfully merging this pull request may close these issues.

New P4Runtime input plugin
5 participants