Add PathElemsMatchQuery helper for checking path membership to a telemetry path #467

wenovus · 2020-10-22T22:01:11Z

This helper checks whether a particular concrete path matches a query
path per gNMI path specifications

NOTE: the new pathstrings.go is COPIED from the ygot package, as
otherwise we have a circular dependency. In the future, the copied
helpers should move to the util package.

…metry path This helper checks whether a particular concrete path matches a query path per [gNMI path specifications](https://github.com/openconfig/reference/blob/master/rpc/gnmi/gnmi-path-conventions.md) NOTE: the new pathstrings.go is **COPIED** from the ygot package, as otherwise we have a circular dependency. In the future, the copied helpers should move to the util package.

coveralls · 2020-10-22T22:05:21Z

Coverage increased (+0.02%) to 90.634% when pulling 78691db on wildcard-path-helper into 00a6fee on master.

robshakir · 2020-10-23T15:32:06Z

util/pathstrings_test.go

+	gnmipb "github.com/openconfig/gnmi/proto/gnmi"
+)
+
+// XXX: This is copied code from ygot package. ygot's code should probably


Is there a reason to take on this debt in the codebase rather than fix it now? It seems like there'd be relatively minimal refactoring here -- i.e., we move the actual implementation to util, and keep the public APIs in ygot just calling these util functions.

We maybe also should consider whether util itself is going to grow into a similar situation, and create a smaller package just for path handling functions.

From a quick scan of the internal dependencies it looks like it's possible to split out all gnmi-path related functions into a new package. This is all of gnmi.go and the GetNode and relatives in reflect.go.

If we move all path-related functions into a new package, we'll essentially be moving util to a new package.

So, I'm not sure if we can quite create a package called pathutil, but gnmiutil is still possible.

I looked at the internal dependencies further.

The util package has roughly the following high-level components in topological/dependency order:

Debug utilities

Basic error type

ygot-specific goyang schema helpers

string path helpers operating on the goyang schema, which uses 3

gNMI path helpers, which uses 4
note: 4 can be separated into helpers that don't use the goyang schema (4a) vs. do (4b), and 5 only depends on those that don't (4b); however, 4a&4b are so intertwined that 4b also depends on 4a's internal methods, so it doesn't make sense functionally to split 4 into two.

GoStruct helpers

GoStruct helpers that operating on goyang's schema, which uses 4

GoStruct traversal functions based on goyang's schema, which uses 4, 6-7

GoStruct traversal functions based on gNMI paths and goyang's schema, that uses 3-7.

While 5&9 can be separated into another package called gnmiutil, which would contain pathstrings.go as well, it would be splitting based on that they both use gpb.Path -- the meaning of gnmiutil would still be unclear to the reader, which I think is the true problem: that when a reader reads util, they don't know what it does.

I think it makes sense, rather, to split util into the following packages in topological order:

util: 1-3

pathutil: 4-5 + ygot/pathstrings.go

gostructutil 6-9
What do you think @robshakir

I think this set sounds a good set to have. Apologies for the delay in responding here. I'm happy with this split, which AIUI should mean that we don't need to duplicate code here.

Since generic package names are not recommended, perhaps a better set of names would be
internal: 1 https://golang.org/doc/go1.4#internalpackages
ygoterrors: 2
yangschema: 3 (we have the option of simplifying some of the function names, e.g. util.SchemaPath -> yangschema.Path)
yangpath: 4-5 + ygot/pathstrings.go
gostruct: 6-9

We might also consider renaming genutil to ygotgen.

Part of util refactoring effort: #556

Ah, sorry -- I thought that this was what you had decided upon. I think that this sounds a reasonable split -- I'm all OK with dividing out to very specific packages, I think in general it means that we end up with a cleaner import set from other modules.

robshakir · 2020-10-23T15:34:21Z

util/gnmi.go

+// PathElem is a possible gNMI telemetry name match for the reference PathElem.
+// In particular, it matches wildcards for names and list keys, but *not* "...".
+// See https://github.com/openconfig/reference/blob/master/rpc/gnmi/gnmi-path-conventions.md
+func pathElemMatchesQuery(elem, refElem *gpb.PathElem) bool {


I'd suggest to call the first argument here queryElem and the second pathElem so that it's clear which of the two we're trying to match against each other.

robshakir · 2020-10-23T15:35:19Z

util/gnmi.go

@@ -134,6 +134,41 @@ func FindPathElemPrefix(paths []*gpb.Path) *gpb.Path {
 	}
 }

+// pathElemMatchesQuery returns true if the given concrete (no wildcard)


Can you clarify concrete here? I think that you're trying to say if the given query element matches the reference element -- but the no wildcard disclaimer is throwing me off, because it seems like wildcards are handled?

robshakir · 2020-10-23T15:38:28Z

util/gnmi.go

+	}
+
+	for k, ref := range refElem.Key {
+		if v, ok := elem.Key[k]; !ok || (ref != "*" && v != ref) {


If ref == "*" then you don't need to check that v != ref right? Because even if v == ref then if ref == * we'll return true.

robshakir · 2020-10-23T15:40:53Z

util/gnmi.go

+		return false
+	}
+
+	for k, ref := range refElem.Key {


There's one confusing thing here -- you say that the reference path must not include wildcards, but then we check whether ref == "*", which can presumably never happen.

We should probably note that if the key is not specified in the reference path, then we never check the value in the queried path, so therefore we assume that the element matches. This seems fine, but we should say that this function isn't schema aware -- it just takes in good faith that both paths are valid schema paths.

robshakir · 2020-10-23T15:41:36Z

util/gnmi.go

+// reference PathElem path slice.
+// In particular, it matches wildcards for names and list keys, but *not* "...".
+// See https://github.com/openconfig/reference/blob/master/rpc/gnmi/gnmi-path-conventions.md
+func PathElemsMatchQuery(elems, refElems []*gpb.PathElem) bool {


Similar comment here as above about making the concrete wording clearer, and the argument names.

robshakir · 2020-10-23T15:44:25Z

util/pathstrings.go

+
+// stringToStructuredPath takes a string representing a path, and converts it to
+// a gnmi.Path, using the PathElem element message that is defined in gNMI 0.4.0.
+// XXX: This is copied code from ygot package. ygot's code should probably


Same comment here about making the change to avoid code duplication.

robshakir · 2020-10-23T15:44:42Z

util/pathstrings.go

+// the name of the element, a map keyed by key name with values of the predicates
+// specified. It removes escape characters from keys and values where they are
+// specified.
+// XXX: This is copied code from ygot package. ygot's code should probably


Same as above - please don't duplicate code.

robshakir · 2021-12-29T18:13:18Z

Wen, this was on the review stack but looks to have outstanding comments. Is there anything you need from me here?

wenovus · 2021-12-29T18:27:29Z

It's actually not this PR, but rather this comment: #467 (comment)
I'm wondering if you think it's worth splitting the util package up into these packages in order to make the functionalities more modular. If you think this is a good idea we might consider doing this prior to v1 release.

wenovus requested review from robshakir and greg-dennis October 22, 2020 22:01

googlebot added the cla: yes This PR author has signed the CLA label Oct 22, 2020

robshakir requested changes Oct 23, 2020

View reviewed changes

wenovus added the needs-discussion Needs Discussion on Next Steps label Nov 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add PathElemsMatchQuery helper for checking path membership to a telemetry path #467

Add PathElemsMatchQuery helper for checking path membership to a telemetry path #467

wenovus commented Oct 22, 2020

coveralls commented Oct 22, 2020

robshakir Oct 23, 2020

robshakir Oct 23, 2020

wenovus Oct 23, 2020

wenovus Oct 23, 2020 •

edited

Loading

robshakir Nov 11, 2020

wenovus Sep 14, 2021

wenovus Nov 10, 2021

robshakir Dec 29, 2021

robshakir Oct 23, 2020

robshakir Oct 23, 2020

robshakir Oct 23, 2020

robshakir Oct 23, 2020

robshakir Oct 23, 2020

robshakir Oct 23, 2020

robshakir Oct 23, 2020

robshakir commented Dec 29, 2021

wenovus commented Dec 29, 2021

Add PathElemsMatchQuery helper for checking path membership to a telemetry path #467

Are you sure you want to change the base?

Add PathElemsMatchQuery helper for checking path membership to a telemetry path #467

Conversation

wenovus commented Oct 22, 2020

coveralls commented Oct 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wenovus Oct 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robshakir commented Dec 29, 2021

wenovus commented Dec 29, 2021

wenovus Oct 23, 2020 •

edited

Loading