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(opcua.event.subscription): New Plugin to Subscribe to OPCUA Events #16300

Closed
wants to merge 28 commits into from

Conversation

frmoschner
Copy link

@frmoschner frmoschner commented Dec 12, 2024

Summary

This Plugin is useful to subscribe to OPC UA node ids to receive upcoming events on the subscribed nodes.

Checklist

  • No AI generated code was used in this PR

Related issues

resolves #16275
replaces #16277

@frmoschner frmoschner changed the title feat(opcua.event.subscription) New Plugin to Subscribe to OPCUA Events feat(opcua.event.subscription): New Plugin to Subscribe to OPCUA Events Dec 12, 2024
@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 Dec 12, 2024
@telegraf-tiger
Copy link
Contributor

@srebhan
Copy link
Member

srebhan commented Dec 13, 2024

Copying your comment from the previous PR regarding my question why this functionality cannot be part of the opcua_listener plugin:

Hi @srebhan,
Thank you very much for your reply!
I have adjusted a few things and created a new pull request #16300.
So far OPCUA Polling & Subscribing is separated from each other, and it would make sense at first sight to merge DataChange Subscriptions (opcua_listener) & Event Subscriptions.
However, in my opinion, the new plugin should not be integrated into the Listener plugin, as a plugin always has a task, and the listener_plugin takes care of the processing of DataChangeNotifcations while the new plugin handles EventNotifications. The structure of the subscription is very different and the listener_plugin uses its own subscribeClientConfig to create client and subscription, which cannot be used for my plugin. So if you were to merge the plugins, you would have to differentiate in many places in the code whether the subscription is a DataChange or an event subscription. I think it is much more user-friendly and easier to maintain if the plugins are separated.
I have seen that there is already an opcua client (plugins/common/opcua/client.go), which I am using from now on. I have removed my own client manager. This means there are no more redundancies in the plugins.

@srebhan
Copy link
Member

srebhan commented Dec 13, 2024

@frmoschner I do not agree with your argumentation. Looking at the opcua_listener plugin, and more specifically at subscribe_client.go a lot of code looks familiar when checking your plugin. Sure the subscription-response processing need to be different and you would need to add a startStreamEvents function including a handler but that's OK.

You are starting this new plugin and someone will request adding tags to the events, something opcua_listener has. Then people will request workarounds because some strange devices return strange result codes, something opcua_listener has. I can go on with this why it is NOT easier to maintain a separate plugin.
Furthermore, you can reuse a lot of configuration options and subscribe to both events and values at the same time with the same connection.

All this being said, I still do not see a strong argument for a separate plugin!

We can talk about changes to the listener plugins that might be necessary but unless you do not provide a strong argument I like to ask you to merge this functionality into the opcua_listener plugin.

@srebhan srebhan self-assigned this Dec 13, 2024
@frmoschner
Copy link
Author

@srebhan , thanks for your arguments. I do agree. I will proceed with this migration after my vacations and will close this merge request from now on.

@frmoschner frmoschner closed this Dec 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OPCUA Event Subscription
2 participants