fix(gnmi) Refactor tag-only subs for complex keys (#11011)

[bewing]

Required for all PRs:

• Updated associated README.md.
• Wrote appropriate unit tests.
• Pull request title or commits are in conventional commit format

resolves #11011

Re-work how to handle tag-only subscriptions to support more complex cases/lookups and improve concurrency

[bewing]

So at this point, I think I have the basic backwards compatibility done.

Will noodle on the right way to handle tag storing/application tomorrow.

[bewing]

Continuing to think on this. What I have currently works, but I'm unsure about the store

Current store behavior:

• User specifies a subscription leaf, and one or more PathElems that MAY contain keys when sent by the gNMI server
• While processing an Update message, if the full PathElem of an update matches one of the original requests, and there are PathElems matching the config that do contain keys, we have received a tag update, and need to store it.
• We want to store this update so that when we receive another update that contains those path elems, IN ORDER, we apply the tag update as a tag to that update.
• The tag store is a tree. The root node has a map[string][]*Node pointing at other nodes. As insert walks the tree:
  • if we have no keyed PathElems remaining, write our value and return
  • Otherwise, match the first PathElem name to the map to retreive a list of nodes
  • Use a simple map comparison function to compare our map[string]string elem.Key to each node in the retrieved list (please tell me a better way to handle this)
  • When found, remove that PathElem from our key list, jump to the next node and continue

To retrieve, we start at the root, and try to walk all our keys down as far as possible, picking up extra tags along the way.

I'm not great with Golang, or computer science in general, so please feel free to recommend better approaches, up to including there already being a generic solution out there we can just import.

Still need to add tests and docs. How should deprecation of tag_only be handled if we proceed with this change?

[bewing]

Tests written. Testing has found another existing bug in handling elements with multiple keys:
lab testing with the config

[[inputs.gnmi.subscription]]
  name = "bgp"
  origin = "openconfig"
  path = "/network-instances/network-instance/protocols/protocol/bgp/neighbors/neighbor/state/session-state"
  subscription_mode = "sample"
  sample_interval="10s"

returns metrics like
bgp,/network-instances/network-instance/protocols/protocol/name=BGP,host=localhost,identifier=BGP,name=default,neighbor_address=192.0.2.1,source=172.20.0.2
when testing against EOS (which uses identifier and name keys in the protocol path element.

I think the above is out of scope for this pull request, however, and wrote the new tests to reflect the current behavior

[powersj]

Before I dig into this a bit more, one of the guiding principles of Telegraf is ensuring we aren't breaking existing users. When I see the config format start to change it makes me wonder :) These two questions come to mind:

1. Will existing configurations continue to work?
2. Would this change the format of metrics that are generated?

[bewing]

Before I dig into this a bit more, one of the guiding principles of Telegraf is ensuring we aren't breaking existing users. When I see the config format start to change it makes me wonder :) These two questions come to mind:

1. Will existing configurations continue to work?

Existing configurations are silently converted to the new configuration, with the caveat that old and new configuration methods cannot be used together: https://github.com/influxdata/telegraf/pull/11019/files#diff-8f015c9f22f0f8f4f8ee17b671e6d8f3ffa1871b86fc765a96e00d9d45ba0f39R507-R510

This code works for the majority use case (applying interface descriptions to other metrics). However, given the user config in #11007, I am going to have to think a bit more. An earlier attempt had a separate function to extract legacy tags -- I will may have to add it to this PR as well, but given the underlying bug we're looking to fix, I'm not sure those users were actually getting useable/correct data

I am unsure if there is a mechanism for informing the end-user of this / an existing deprecation policy for telegraf -- would appreciate any pointers here.

2. Would this change the format of metrics that are generated?

I did not modify existing tests when adding this code, and they all still pass, so I do not believe so.

[powersj]

@bewing thanks for this - can you resolve the conflict and we can land this?

[bewing]

Rebased and pushed. Looks like I might have some linting to do?

[powersj]

plugins/inputs/gnmi/README.md:1: missing required section 'Metrics'

I'm happy to ignore for now, but if you want to put up a PR or update this one with the metrics section filed out it would be nice at some point. That section just describes the layout of the metric collected.

[telegraf-tiger]

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 doesn't significantly change the Telegraf binary size (less than 1%)

📦 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_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]

plugins/inputs/gnmi/README.md:1: missing required section 'Metrics'

I'm happy to ignore for now, but if you want to put up a PR or update this one with the metrics section filed out it would be nice at some point. That section just describes the layout of the metric collected.

Will put in a separate PR, as it should be a separate commit anyway.