Conversation
…onfig to 'metric_name' and the one in node config to field_name
…oups also have a default namespace and identifier type so node config isn't so repetetive. Don't export variables that aren't needed outside. Rename structs that hold toml settings
plugins/inputs/opcua/README.md
Outdated
| ## identifier - tag as shown in opcua browser | ||
| ## data_type - boolean, byte, short, int, uint, uint16, int16, | ||
| ## uint32, int32, float, double, string, datetime, number | ||
| ## field_name - field name to use in the output |
There was a problem hiding this comment.
this documentation is a bit confusing; I thought these were top-level settings until I got to the example below. Not sure what we can do to improve it.
There was a problem hiding this comment.
Nodes was a top level setting before this PR. I added groups in this PR and each group also has a set of nodes. I left in the top level nodes for backward compatibility. I can see that this is confusing so I'll have to work on the docs.
|
I reverted the changes that affect backward compatibility. I think you both are right that this plugin has been released long enough that we should keep compatibility. I also added a feature OPC UA users have requested, which is to be able to add tags to specific node IDs or to groups. Sorry for sneaking this feature into this PR after the initial review but it seemed closely related and not big enough to start a separate PR. |
plugins/inputs/opcua/opcua_client.go
Outdated
| for i, item := range o.NodeList { | ||
| nameEncountered := map[metricParts]struct{}{} | ||
| for i := range o.nodes { | ||
| node := &o.nodes[i] |
There was a problem hiding this comment.
Couldn't you get this directly in the for loop above?
| func newMP(n *Node) metricParts { | ||
| var keys []string | ||
| for key := range n.metricTags { | ||
| keys = append(keys, key) | ||
| } | ||
| sort.Strings(keys) | ||
| var sb strings.Builder | ||
| for i, key := range keys { | ||
| if i != 0 { | ||
| sb.WriteString(", ") | ||
| } | ||
| sb.WriteString(key) | ||
| sb.WriteString("=") | ||
| sb.WriteString(n.metricTags[key]) | ||
| } | ||
| x := metricParts{ | ||
| metricName: n.metricName, | ||
| fieldName: n.tag.FieldName, | ||
| tags: sb.String(), | ||
| } | ||
| return x | ||
| } |
There was a problem hiding this comment.
This function might benefit speedwise from the optimization done here: #8391.
There was a problem hiding this comment.
Since this validation is only needed once during plugin initialization I didn't think optimization was worthwhile
srebhan
left a comment
There was a problem hiding this comment.
Two very minor comments. Looks good to me.
Remove unneeded node settings (data_type and description).
Add the concept of a node group. Groups have default settings for all nodes in the group. They also are able to override the plugin's metric name so multiple metric names can be used in a single plugin instance.
Remove the name tag which wasn't useful because it duplicated the OPC UA value's field name.
Expose stats to telegraf internal plugin.