feat(inputs.opcua): Implement browsing tags on initial connect #15064
Conversation
supergrover
force-pushed
the
feature/opcua-browse
branch
from
b82fa3e
to
b36af19
Compare
Introduce a `browse` flag in opcua and opcua_listener configurations which causes the plugin to browse the tag using the OPC servers' browse feature.
supergrover
force-pushed
the
feature/opcua-browse
branch
from
b36af19
to
fc65907
Compare
srebhan
changed the title
telegraf-tiger
bot
added
area/opcua
feat
There was a problem hiding this comment.
@supergrover first of all thank you for your pull-request! I do have some comments in the code and additionally the following more high-level comments:
- Please run
make lintto see the linter issues in your code. Please fix them! - How does this code relate to gopcua's BrowseRequest? Can we make use of the upstream functionality instead of coding this by hand?
| @@ -57,6 +58,7 @@ type NodeSettings struct { | |||
| TagsSlice [][]string `toml:"tags" deprecated:"1.25.0;use 'default_tags' instead"` | |||
| DefaultTags map[string]string `toml:"default_tags"` | |||
| MonitoringParams MonitoringParameters `toml:"monitoring_params"` | |||
| Browse bool `toml:"browse"` | |||
There was a problem hiding this comment.
How about making this a pointer like
| Browse bool `toml:"browse"` | |
| Browse *bool `toml:"browse"` |
as this allows you to decide whether the value was actually set by the user or not.
| if node.Browse == false { | ||
| node.Browse = group.Browse | ||
| } |
There was a problem hiding this comment.
With the above change you can do
| if node.Browse == false { | |
| node.Browse = group.Browse | |
| } | |
| if node.Browse == nil { | |
| node.Browse = &group.Browse | |
| } |
| var nid *ua.NodeID = nil | ||
|
|
||
| if node.Tag.Browse && node.Tag.IdentifierType == "s" { | ||
| browsedNodeID, err := o.browseNode(ctx, node.Tag.Namespace, node.Tag.Identifier) | ||
| if err != nil { | ||
| o.Log.Errorf("Error browsing node %s: %s", node.Tag.FieldName, err) | ||
| // continue, other nodes may be browseable | ||
| // also, add the node as a string identifier. The server will not be able to return data | ||
| // but we must add a node to the o.NodeIDs array in order for it to match the o.NodeMetricMappings | ||
| parsedNodeID, err := ua.ParseNodeID(node.Tag.NodeID()) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| nid = parsedNodeID | ||
| } else { | ||
| nid = browsedNodeID | ||
| } | ||
| } else { | ||
| parsedNodeID, err := ua.ParseNodeID(node.Tag.NodeID()) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| nid = parsedNodeID | ||
| } |
There was a problem hiding this comment.
How about
| var nid *ua.NodeID = nil | |
| if node.Tag.Browse && node.Tag.IdentifierType == "s" { | |
| browsedNodeID, err := o.browseNode(ctx, node.Tag.Namespace, node.Tag.Identifier) | |
| if err != nil { | |
| o.Log.Errorf("Error browsing node %s: %s", node.Tag.FieldName, err) | |
| // continue, other nodes may be browseable | |
| // also, add the node as a string identifier. The server will not be able to return data | |
| // but we must add a node to the o.NodeIDs array in order for it to match the o.NodeMetricMappings | |
| parsedNodeID, err := ua.ParseNodeID(node.Tag.NodeID()) | |
| if err != nil { | |
| return err | |
| } | |
| nid = parsedNodeID | |
| } else { | |
| nid = browsedNodeID | |
| } | |
| } else { | |
| parsedNodeID, err := ua.ParseNodeID(node.Tag.NodeID()) | |
| if err != nil { | |
| return err | |
| } | |
| nid = parsedNodeID | |
| } | |
| var nid *ua.NodeID = nil | |
| if node.Tag.Browse && node.Tag.IdentifierType == "s" { | |
| browsedNodeID, err := o.browseNode(ctx, node.Tag.Namespace, node.Tag.Identifier) | |
| if err == nil { | |
| o.NodeIDs = append(o.NodeIDs, browsedNodeID) | |
| continue | |
| } | |
| // Continue as other nodes might still be browseable. | |
| // Add the node as a string identifier, the server will not be able to return data | |
| // but we must add a node to the o.NodeIDs array in order for it to match the | |
| // o.NodeMetricMappings | |
| o.Log.Errorf("Browsing node %q failed: %v", node.Tag.FieldName, err) | |
| } | |
| parsedNodeID, err := ua.ParseNodeID(node.Tag.NodeID()) | |
| if err != nil { | |
| return err | |
| } | |
| o.NodeIDs = append(o.NodeIDs, parsedNodeID) |
| func parseNamespaceId(ns string) (uint16, error) { | ||
| nsId, err := strconv.ParseUint(ns, 10, 16) | ||
| if err != nil { | ||
| return 0, fmt.Errorf("invalid namespace number: %s", err) | ||
| } | ||
|
|
||
| return uint16(nsId), nil | ||
| } |
There was a problem hiding this comment.
Please merge this into the respective calling places as this is not worth a wrapper.
| } | ||
|
|
||
| func (o *OpcUAInputClient) browseNode(ctx context.Context, ns string, browsePath string) (*ua.NodeID, error) { | ||
| o.Log.Infof("Browsing node %s", browsePath) |
There was a problem hiding this comment.
This should at most be a debug log.
| @@ -800,6 +800,7 @@ func TestSubscribeClientConfigValidMonitoringParams(t *testing.T) { | |||
| }) | |||
|
|
|||
| subClient, err := subscribeConfig.CreateSubscribeClient(testutil.Logger{}) | |||
| subClient.Connect() | |||
There was a problem hiding this comment.
Why do you need to modify this test?
| @@ -124,6 +110,22 @@ func (o *SubscribeClient) Connect() error { | |||
| return err | |||
| } | |||
|
|
|||
| // Make sure we setup the node-ids correctly after reconnect | |||
| // as the server might be restarted and IDs changed | |||
| o.Log.Debugf("Creating monitored items on first connection") | |||
There was a problem hiding this comment.
This log line does not help anyone. ;-)
|
Thanks for the suggestions! I'll work on them and get back to this.
Sure, I did not know about the linter until Github started screaming at me :)
The current code basically parses the string formatted browse path and passes it to the opcua.Node.TranslateBrowsePathsToNodeIDs method on the gopcua client, so basically this already is an upstream function we're calling. Although the parsing of the browse paths could be a suggestion for upstream, if that's what you mean.
That's part of my work on seeing what needs to be done to satisfy the tests which appears to have slipped through. Basically, the previous code parsed all the node configurations and created the monitored item requests on creation of the subscribe client in https://github.com/influxdata/telegraf/blob/master/plugins/inputs/opcua_listener/subscribe_client.go#L111. |
No worries! It would be good to fix them before pushing so we save some CircleCI credits. ;-)
Thanks for your explanation! Yeah I wondered why you manually need to mangle the path and suspected an upstream function for doing this. :-) I'm OK with doing it in the plugin but if you want to maybe suggesting this upstream is also a good thing. We can still switch over to it later...
I see, then please check all errors before continuing! I think the |
|
Hello! I am closing this issue due to inactivity. I hope you were able to resolve your problem, if not please try posting this question in our Community Slack or Community Forums or provide additional details in this issue and reqeust that it be re-opened. Thank you! |
Summary
This is a proposed implementation in order to implement tag browsing for OPC-UA tags on initial connect as described in #15001.
This pull request is for review purposes, some tests are still failing on this because a connection to an OPC server is now required in order initialize all NodeIDs. I'm still working on that part.
Checklist
Related issues
resolves #15001