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

[inputs/modbus] Add possibility to specify measurement per register #7231

Merged
merged 4 commits into from
Mar 30, 2020

Conversation

srebhan
Copy link
Member

@srebhan srebhan commented Mar 25, 2020

Required for all PRs:

  • Signed CLA.
  • Associated README.md updated.
  • Has appropriate unit tests.

With this PR it is possibile to specify an (optional) measurement property per register definition to group those registers as fields of the specified measurement. In case the measurement property is missing modbus is used for the measurement name just as before.

The reasoning behind this is that some devices have restrictions for the number of simultaneous connections. When reading multiple metrics (e.g. voltage, current, work, power) from one device, assigning each them to different measurements would take multiple instances of the modbus plugin, each with a name_override. Thus these multiple instances will concurrently connect to device potentially exceeding the number of maximum allowed connections.
With the PR applied, one modbus plugin can read all required registers from the device (with one connection) and then sort the fields into the specified measurements.

@danielnelson
Copy link
Contributor

Normally I would suggest against multiple measurements per device. I'd be interested to hear more about why it would be helpful to have multiple measurements, perhaps you could attach your configuration with these changes?

@srebhan
Copy link
Member Author

srebhan commented Mar 26, 2020

@danielnelson this is the config for one of our energy meters without the PR applied:
telegraf_without_measurement.conf.txt
As you can see I create multiple instances (12 here but there are some more information we potentially can gather) of the modbus plugin to query the information and push them out to the corresponding metric using name_override.

While this works and does what we want, we create 12 TCP connections to the meter. However, some meters allow only a very limited number of simultaneous connections (down to 4) and you easily get into trouble. Furthermore, the plugin optimizes register access for the queries by grouping consecutive registers. This cannot be done when split over multiple instances.

This is the configuration with the PR applies:
telegraf_with_measurement.conf.txt
It is leading to exactly the same result as above, however, only one TCP connection is opened and register access can be optimized more within the plugin.

I think we will have the same problem as above with all multi-metric sensors, especially in cases where there are connection limits...

@danielnelson
Copy link
Contributor

Alright, but let's try using SeriesGrouper to build up the metric groups? We could remove the duplicate name check completely, I think it's not worth it.

Just a suggestion, but have you considered joining the measurement with the field key or using another method to fully describe the value? I think in some respects (but not all) this will be more flexible to work with, in particular if you need to work with values from V and I at the same time.

In other words, switching from:

{measurement = "V", name = "L1"},
{measurement = "V", name = "L2"},
{measurement = "V", name = "L3"},

{measurement = "I", name = "L1"},
{measurement = "I", name = "L2"},
{measurement = "I", name = "L3"},

{measurement = "P", name = "L1"},
{measurement = "P", name = "L2"},
{measurement = "P", name = "L3"},
{measurement = "P", name = "value"},

To something like this:

{name = "V_L1"},
{name = "V_L2"},
{name = "V_L3"},

{name = "I_L1"},
{name = "I_L2"},
{name = "I_L3"},

{name = "P_L1"},
{name = "P_L2"},
{name = "P_L3"},
{name = "P"},

@srebhan srebhan force-pushed the modbus_measurement branch from 3e55bbc to ddccb3d Compare March 30, 2020 12:02
@srebhan
Copy link
Member Author

srebhan commented Mar 30, 2020

Alright, but let's try using SeriesGrouper to build up the metric groups?

This now uses the SeriesGrouper making the code look much cleaner and the diff smaller. Thanks for the suggestion!

We could remove the duplicate name check completely, I think it's not worth it.

I left it in because it's neither a lot of code nor a big diff and it saved me a few times already when copy-pasting... :-)
I rebased the code on latest master to avoid a conflict and squashed the changes to simplify the review.

Just a suggestion, but have you considered joining the measurement with the field key or using another method to fully describe the value?

We used this scheme in the past, however having different sensors with different sets of measurements completely destroys discoverability for people working with the data in my experience. Anyway with the PR applied, people will have the chance to do both... ;-)

@danielnelson danielnelson added this to the 1.15.0 milestone Mar 30, 2020
@danielnelson danielnelson added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Mar 30, 2020
@danielnelson danielnelson merged commit 3650d74 into influxdata:master Mar 30, 2020
athoune pushed a commit to bearstech/telegraf that referenced this pull request Apr 17, 2020
@srebhan srebhan deleted the modbus_measurement branch April 21, 2020 18:47
veorlo pushed a commit to GlobalNOC/telegraf that referenced this pull request May 4, 2020
idohalevi pushed a commit to idohalevi/telegraf that referenced this pull request Sep 29, 2020
arstercz pushed a commit to arstercz/telegraf that referenced this pull request Mar 5, 2023
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.

3 participants