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 dynamic tagging to gnmi plugin #7484

Merged
merged 3 commits into from
Feb 7, 2022
Merged

feat: Add dynamic tagging to gnmi plugin #7484

merged 3 commits into from
Feb 7, 2022

Conversation

bewing
Copy link
Contributor

@bewing bewing commented May 8, 2020 •
edited
Loading

Required for all PRs:

  • Signed CLA.
  • Associated README.md updated.
  • Has appropriate unit tests.

Closes #7462

Add the ability to specify gNMI subscriptions to be tag-only, storing them for application to other subscriptions with the exact same tagged "name". The primary application of the author is to apply interface description tags to all other interface metrics, to ease search in Influx.

I am extremely open to suggestion on improvement for either the memory storage of the tags, or alternatives for additional aliasing of the dynamic tag naming.

@bewing
Copy link
Contributor Author

bewing commented May 8, 2020

If anyone wants to do integration testing against non-Arista hardware, it would be appreciated.

@bewing
Copy link
Contributor Author

bewing commented May 11, 2020 •
edited
Loading

Switched from using / as the tag separator to _, as grafana was having a heck of a time when tags contained slashes per grafana/grafana#6012

Switched back to /, since grafana claims to have resolved the issue with slashes in tags.

@bewing bewing changed the title Add dynamic tagging to cisco_telemetry_gnmi Add dynamic tagging to gnmi plugin Aug 27, 2020
@bewing
Copy link
Contributor Author

bewing commented Aug 27, 2020

Updated for new plugin name

@bewing
Copy link
Contributor Author

bewing commented Oct 14, 2020

Am I missing any data/information needed to merge this feature? What are the next steps from my side?

ssoroka
ssoroka reviewed Oct 28, 2020
View reviewed changes
plugins/inputs/gnmi/README.md Outdated Show resolved Hide resolved
plugins/inputs/gnmi/gnmi.go Outdated Show resolved Hide resolved
plugins/inputs/gnmi/gnmi.go Show resolved Hide resolved
@bewing
Copy link
Contributor Author

bewing commented Nov 11, 2020

Switched from using / as the tag separator to _, as grafana was having a heck of a time when tags contained slashes per grafana/grafana#6012

Given that Granfana has resolved their issue with slashes, does it make sense to switch back to using slashes as separators in this plugin?

@sjwang90 sjwang90 added area/gnmi feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin labels Nov 23, 2020
@bewing bewing mentioned this pull request Dec 1, 2020
Closed
@kb70 kb70 mentioned this pull request Dec 21, 2020
Closed
@slampt
Copy link

slampt commented Mar 2, 2021

Hola, just curious why this wasn't merged in?

@bewing bewing force-pushed the gnmi-lookup-tags branch 5 times, most recently from 266ca12 to 54708f6 Compare March 17, 2021 13:09
@cprecup
Copy link

cprecup commented Mar 19, 2021

Hello @bewing,

Tested this with IOS-XR and the OpenConfig model openconfig-interfaces but I can't confirm it works on my end.

Here is the configuration:

[...]
 [[inputs.gnmi.subscription]]
   origin = "openconfig-interfaces"
   path = "/interfaces/interface/state/counters"
   subscription_mode = "sample"
   sample_interval = "60s"

 [[inputs.gnmi.subscription]]
   name = "descr"
   origin = "openconfig-interfaces"
   path = "/interfaces/interface/state/description"
   subscription_mode = "on_change"
   tag_only = true
[...]

The output is as follows:

telegraf git:(7484) ✗ ./telegraf --config telegraf-7484-gnmi-embedded-tags.conf 
2021-03-18T17:58:17Z I! Starting Telegraf 
[...]
2021-03-18T17:58:19Z E! [inputs.gnmi] Subscribe error (3), "openconfig-interfaces:interfaces/interface/state/description:ERR:No sysdb paths found for yang path openconfig-interfaces:interfaces/interface/state/description\x00"
counters,host=telegraf,name=GigabitEthernet0/0/0/0,path=openconfig-interfaces:/interfaces/interface/state/counters,source=x.f.g.h in_octets=0i,out_octets=0i,in_multicast_pkts=0i,in_broadcast_pkts=0i,out_multicast_pkts=0i,out_broadcast_pkts=0i,in_unknown_protos=0i,in_errors=0i,out_errors=0i,last_clear="2021-03-18T15:10:02Z",in_unicast_pkts=0i,in_discards=0i,out_unicast_pkts=0i,out_discards=0i 1616089682978000000
counters,host=telegraf,name=GigabitEthernet0/0/0/1,path=openconfig-interfaces:/interfaces/interface/state/counters,source=x.f.g.h in_octets=0i,out_octets=0i,in_multicast_pkts=0i,in_broadcast_pkts=0i,out_multicast_pkts=0i,out_broadcast_pkts=0i,in_unknown_protos=0i,in_errors=0i,out_errors=0i,last_clear="2021-03-18T15:10:02Z",in_unicast_pkts=0i,in_discards=0i,out_unicast_pkts=0i,out_discards=0i 1616089682978000000
[...]

@telegraf-tiger
Copy link
Contributor

telegraf-tiger bot commented Jun 2, 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

@bewing
Copy link
Contributor Author

bewing commented Jun 2, 2021

Rebased for merge conflict

@telegraf-tiger
Copy link
Contributor

telegraf-tiger bot commented Jun 2, 2021

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

Artifact URLs

srebhan
srebhan reviewed Dec 22, 2021
View reviewed changes
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.

@bewing thanks for submitting this PR. I have some comments similar to the previous comments by Steven. Beside some smaller comments in the code I propose a way to save at least one level in the map. While I'm not 100% satisfied with three nested maps, it's a beginning. Maybe we can boil it further down?

plugins/inputs/gnmi/README.md Outdated Show resolved Hide resolved
plugins/inputs/gnmi/gnmi.go Outdated Show resolved Hide resolved
plugins/inputs/gnmi/gnmi.go Outdated Show resolved Hide resolved
plugins/inputs/gnmi/gnmi.go Outdated Show resolved Hide resolved
@srebhan srebhan self-assigned this Dec 22, 2021
@telegraf-tiger
Copy link
Contributor

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

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

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

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

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

srebhan
srebhan approved these changes Jan 24, 2022
View reviewed changes
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.

@bewing that's exactly what I had in mind. :-) Thanks for taking the time to implement it!

Looks good to me.

@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 Jan 24, 2022
@sspaink sspaink changed the title Add dynamic tagging to gnmi plugin feat: Add dynamic tagging to gnmi plugin Jan 24, 2022
@telegraf-tiger
Copy link
Contributor

telegraf-tiger bot commented Feb 1, 2022

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 Feb 1, 2022

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

@powersj
Copy link
Contributor

powersj commented Feb 1, 2022

@bewing have you signed the CLA? @sspaink has fixed up the readme/config docs, so this is ready to merge, minus that.

@telegraf-tiger
Copy link
Contributor

telegraf-tiger bot commented Feb 1, 2022

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

📦 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 darwin_arm64.tar.gz windows_i386.zip
armel.deb armv6hl.rpm freebsd_amd64.tar.gz
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
static_linux_amd64.tar.gz

@bewing
Copy link
Contributor Author

bewing commented Feb 1, 2022

@bewing have you signed the CLA? @sspaink has fixed up the readme/config docs, so this is ready to merge, minus that.

I have not. I have an inquiry with my legal department pending regarding that. I will follow up with them this week.

@bewing
Copy link
Contributor Author

bewing commented Feb 7, 2022 •
edited
Loading

Individual CLA signed. Working on corporate CLA now.

@powersj
Copy link
Contributor

powersj commented Feb 7, 2022

!signed-cla

@powersj powersj merged commit 7e769d7 into influxdata:master Feb 7, 2022
@powersj
Copy link
Contributor

powersj commented Feb 7, 2022

Thank you @bewing!

pteich added a commit to pteich/telegraf that referenced this pull request Feb 13, 2022
@pteich
Merge branch 'master' into feature/opensearch-support
0afaf35 
* master: (117 commits)
  fix: bump github.com/nats-io/nats-server/v2 from 2.6.5 to 2.7.2 (influxdata#10638)
  chore: add -race flag to go tests (influxdata#10629)
  feat: update etc/telegraf.conf and etc/telegraf_windows.conf (influxdata#10631)
  fix: license doc outdated causing CI failure (influxdata#10630)
  fix: bump k8s.io/client-go from 0.22.2 to 0.23.3 (influxdata#10589)
  feat: Implemented support for reading raw values, added tests and doc (influxdata#6501)
  fix: Improve parser tests by using go-cmp/cmp (influxdata#10497)
  feat(mongodb): add FsTotalSize and FsUsedSize informations (influxdata#10625)
  docs: update quay docs for auth (influxdata#10612)
  chore: allow downgrade of go version in windows script (influxdata#10614)
  chore: update CI go to 1.17.6 (influxdata#10611)
  feat: update etc/telegraf.conf and etc/telegraf_windows.conf (influxdata#10600)
  fix(inputs.opcua): add more data to error log (influxdata#10465)
  fix: bump github.com/aws/aws-sdk-go-v2/service/kinesis from 1.6.0 to 1.13.0 (influxdata#10601)
  refactor: use early return pattern (influxdata#10591)
  fix: bump github.com/benbjohnson/clock from 1.1.0 to 1.3.0 (influxdata#10588)
  feat: add dynamic tagging to gnmi plugin (influxdata#7484)
  fix: bump github.com/Azure/azure-kusto-go from 0.5.0 to 0.5.2 (influxdata#10598)
  feat: update etc/telegraf.conf and etc/telegraf_windows.conf (influxdata#10584)
  fix(parsers.json_v2): allow optional paths and handle wrong paths correctly (influxdata#10468)
  ...

# Conflicts:
#	plugins/outputs/elasticsearch/elasticsearch.go
#	plugins/outputs/elasticsearch/elasticsearch_test.go
@sjwang90 sjwang90 mentioned this pull request Mar 7, 2022
Closed
@powersj powersj mentioned this pull request Jan 29, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ssoroka ssoroka ssoroka left review comments

@srebhan srebhan srebhan approved these changes

@powersj powersj powersj approved these changes

Assignees

@srebhan srebhan

Labels
area/gnmi feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin 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.

gNMI: Add support for static and dynamic tag mappings
8 participants
@bewing @slampt @cprecup @powersj @ssoroka @srebhan @sjwang90 @sspaink