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

fix(inputs.gnmi): Handle both new-style tag_subscription and old-style tag_only #12512

Merged
merged 27 commits into from
Feb 8, 2023

Conversation

srebhan
Copy link
Contributor

@srebhan srebhan commented Jan 16, 2023

resolves #11778

In PR #11019 a new style of handling dynamic tags was introduced, able to handle more complex tagging rules (e.g. by matching an interface key). However, the conversion/back-porting of the previous behavior will only work for a small subset of devices that expose an interface key and thus broke previous users (as described in #11778).

This PR reworks the tag-handling to be able to support both behaviors adding an match option. The plugin intelligently chooses the value if not given to be backward-compatible to both current use-cases.
However, this logic required a larger refactoring of the plugin. While code is more understandable now IMO, the volume of the change is considerable...

@telegraf-tiger telegraf-tiger bot added area/gnmi fix pr to fix corresponding bug plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins labels Jan 16, 2023
@cjonesdialpad
Copy link

Sent feedback via Slack thread. Summary: old tag_only syntax still not working. New syntax does get the interface description as its own item but does not seem to get applied to the actual interface data lines.

@clsmyth
Copy link

clsmyth commented Jan 19, 2023

i downloaded the binary with the 3 additional commits from 18 jan 12:15...still broken for me...i will update the ticket.

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

oof this took a while, but I only have a question about all the commented out new code?

plugins/inputs/gnmi/gnmi_test.go Outdated Show resolved Hide resolved
@telegraf-tiger
Copy link
Contributor

telegraf-tiger bot commented Feb 7, 2023

@srebhan srebhan requested a review from powersj February 8, 2023 11:22
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.

Leaving it to you to merge, thanks for the fix!

@powersj powersj assigned srebhan and unassigned powersj Feb 8, 2023
@srebhan srebhan merged commit 58a01e1 into influxdata:master Feb 8, 2023
@srebhan srebhan deleted the gnmi_issue_11778 branch February 8, 2023 18:30
powersj pushed a commit that referenced this pull request Feb 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/gnmi fix pr to fix corresponding bug 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.

inputs.gnmi with dynamic tagging won't work
4 participants