-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Add node groups to opcua input plugin #8389
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏼
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't you get this directly in the for loop above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed, thanks
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function might benefit speedwise from the optimization done here: #8391.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this validation is only needed once during plugin initialization I didn't think optimization was worthwhile
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.