Skip to content
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

feat(inputs.opcua_listener): OPC UA Event subscriptions #11786

Merged
merged 7 commits into from
Oct 25, 2022

Conversation

LarsStegman
Copy link
Contributor

@LarsStegman LarsStegman commented Sep 12, 2022

Required for all PRs

resolves #8083

Refactored the opc ua plugin to use a more generic structure. This allows easy injection of either the read client or a subscription client.

I extracted the logic for an opc ua client into an /internal/opcua package, as the same client logic could be reused for creating an opcua client output plugin.

It has become quite a big pull request, so please be thorough with your reviews.

There should be no breaking changes, as by default the request service is used. This PR is purely additive.

@telegraf-tiger telegraf-tiger bot added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Sep 12, 2022
@LarsStegman LarsStegman force-pushed the opcua-subs branch 4 times, most recently from 0a2719d to 1c00277 Compare September 13, 2022 08:39
@Hipska Hipska added area/opcua plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins labels Sep 13, 2022
Copy link
Contributor

@Hipska Hipska left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not yet reviewed all files, but already some minor comments..

internal/opcua/util.go Outdated Show resolved Hide resolved
plugins/inputs/opcua/sample.conf Outdated Show resolved Hide resolved
plugins/inputs/opcua/README.md Outdated Show resolved Hide resolved
plugins/inputs/opcua/README.md Outdated Show resolved Hide resolved
plugins/inputs/opcua/read_client_test.go Outdated Show resolved Hide resolved
@Hipska Hipska changed the title feat(opcua): Input plugin subscriptions feat(inputs.opcua): OPC UA Event subscriptions Sep 13, 2022
@LarsStegman LarsStegman requested a review from Hipska September 16, 2022 11:55
@Hipska Hipska requested a review from srebhan September 16, 2022 13:02
@LarsStegman LarsStegman requested review from Hipska and removed request for srebhan September 23, 2022 11:59
@Hipska Hipska requested a review from srebhan September 23, 2022 12:28
@powersj powersj added the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Sep 23, 2022
Copy link
Member

@srebhan srebhan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First of all thanks @LarsStegman for the huge undertaking to clean this up. Much appreciated! Before going into a more detailed review I have some more general questions:

  1. Can you please more internal/opcua to plugins/common/opcua as this is where we should keep plugin related things.
  2. Is it possible to mimick the behavior of the HTTP common part (in plugins/common/http) with a shared config and a defined way to create a client from it? This client would then provide the interface functions you define.
  3. Does it make sense to split the opcu plugin into one actively querying part (named opcu) and a listening part named opcua_listener or opcua_consumer? This way, you don't need to deal with partially overlapping config options and stuff but rather move common things to the common part and keep distictive code in the corresponding plugin.

Looking forward to hearing your opinion!

internal/opcua/client.go Outdated Show resolved Hide resolved
internal/opcua/client.go Outdated Show resolved Hide resolved
internal/opcua/client.go Outdated Show resolved Hide resolved
internal/opcua/client.go Outdated Show resolved Hide resolved
internal/opcua/client.go Outdated Show resolved Hide resolved
Comment on lines 82 to 91
type OpcUAClient struct {
Config *OpcUAClientConfig
Log telegraf.Logger

State ConnectionState
Client *opcua.Client

opts []opcua.Option
codes []ua.StatusCode
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess both State and Client don't need to be exported, do they? Furthermore, you probably want to generate the client from a given config (similar to http's common part) and do not need to store it internaly...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Client and State need to be exported, because the OpcUAClient is now in the common package and SubscribeClient and ReadClient need access to it.

I do not entirely understand what you mean with generating the Client from a config and not needing to store it internally. We need the client in memory, because we need to maintain a connection to the server.

internal/opcua/client.go Outdated Show resolved Hide resolved
internal/opcua/client.go Outdated Show resolved Hide resolved

switch u.Scheme {
case "opc.tcp":
o.State = Connecting
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this state-tracking? Isn't the state encoded in the existence of the client?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was already in the original code.

There was some discussion on it here, so I'm hesitant to remove it.

internal/opcua/client_test.go Outdated Show resolved Hide resolved
@LarsStegman
Copy link
Contributor Author

LarsStegman commented Sep 27, 2022

  1. Can you please more internal/opcua to plugins/common/opcua as this is where we should keep plugin related things.

Yes, I will do this.

  1. Is it possible to mimick the behavior of the HTTP common part (in plugins/common/http) with a shared config and a defined way to create a client from it? This client would then provide the interface functions you define.

I think this should be possible. I will look into it. I wasn't very happy with how the client creation was done anyway, but I hadn't found a better solution.

  1. Does it make sense to split the opcu plugin into one actively querying part (named opcu) and a listening part named opcua_listener or opcua_consumer? This way, you don't need to deal with partially overlapping config options and stuff but rather move common things to the common part and keep distictive code in the corresponding plugin.

This might be a good idea. This will be much easier now that the new configuration loading structure has been adopted everywhere. When I implemented this, that wasn't the case yet, so loading the configuration would have been much more of a hassle. I am a bit worried about having to maintain two example configs, but I think this is not that big of a problem.

@LarsStegman
Copy link
Contributor Author

I will fix the build failing tomorrow

plugins/inputs/opcua/README.md Outdated Show resolved Hide resolved
plugins/inputs/opcua_listener/README.md Outdated Show resolved Hide resolved
plugins/inputs/opcua_listener/README.md Show resolved Hide resolved
@LarsStegman LarsStegman changed the title feat(inputs.opcua): OPC UA Event subscriptions feat(inputs.opcua_listener): OPC UA Event subscriptions Sep 28, 2022
@LarsStegman LarsStegman force-pushed the opcua-subs branch 4 times, most recently from a6bf8d5 to 07249c9 Compare September 28, 2022 11:30
@LarsStegman
Copy link
Contributor Author

@Hipska any idea what is triggering the CI? I can't find it.

plugins/inputs/opcua/README.md Show resolved Hide resolved
plugins/inputs/opcua/opcua.go Outdated Show resolved Hide resolved
@LarsStegman LarsStegman marked this pull request as draft October 20, 2022 07:02
@LarsStegman LarsStegman force-pushed the opcua-subs branch 3 times, most recently from 28eb435 to 81e6856 Compare October 20, 2022 14:47
@LarsStegman LarsStegman marked this pull request as ready for review October 20, 2022 14:56
Copy link
Contributor

@powersj powersj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for driving this!

@telegraf-tiger
Copy link
Contributor

@reimda reimda merged commit 739f800 into influxdata:master Oct 25, 2022
@LarsStegman LarsStegman deleted the opcua-subs branch October 25, 2022 14:08
@LarsStegman
Copy link
Contributor Author

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/opcua feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OPC UA Events based plugin
6 participants