RichiH / modbus_exporter

Exporter which retrieves stats from a modbus system and exports them via HTTP for Prometheus consumption.
Apache License 2.0
90 stars 44 forks source link

Should support range reads for good performance #20

Open rfc1036 opened 3 years ago

rfc1036 commented 3 years ago

modbus_exporter currently reads one register at a time, and this takes a lot of time because a serial modbus bus is very slow (usually not more than 9600 bps) and hence each command has a very high latency. I did some tests with mbpoll and reading a range of tens of registers takes the same time as reading 2-3 registers one by one. This is needed to be able to frequently query more than an handful of instruments over a serial bus.

Since modbus_exporter reads in sequence all the registers configured in the module when it receives the HTTP request is should be easy to add to each module a directive containing a list of register ranges, something like:

  - name: ABB_M2M
    protocol: 'tcp/ip'
    ranges:
   # the format is: the number of the first register and the number of registers to read in the same request
    - '34098 12'
    - '34126 22'
    metrics:
...

And then have it read each range instead of single registers and report back as metrics the ones configured in the module.

Some experimentation may be needed to determine the optimal number of ranges lenght vs. number of ranges, hence the need to be able to list more than one. Also, the same instrument may require to use different modbus commands for different metrics.

(I would love to implement this but I do not know Go...)

RichiH commented 3 years ago

I feel as if this might impact stability, and thus I suspect that there's guidance on save limits somewhere. The question is: Where might that be?

@dshatohin as you seem to be cleaning up, have modbus devices, and other scada software at hand: Want to take a stab at this?

dshatokhin commented 3 years ago

@RichiH, the limits I've met is around 100 registers. But it's just my experience.

It would be interesting to try šŸ˜

dshatokhin commented 3 years ago

Yes, modbus is part of my life right now, no matter how sad it sounds. Modbus-devices, scada installation and container environment to test modbus_exporter in the field šŸ¤­

RichiH commented 3 years ago

There's worse; I enjoyed the short time I had access. And it kinda keeps the world ticking. I may be biased, but I think marrying ModBus & Prometheus is a good thing and will make industrial settings more reliable, so... :)

RichiH commented 3 years ago

Maybe introduce a limit through configuration for how many registers can be queried at once? We could set the default to 10 and allow people to go to whatever. With some guidance around 100 being anecdotally save and us requesting data submissions on performance & safety from people.

dshatokhin commented 3 years ago

This limit can be set as modbus_exporter flag with value 10 as default

RichiH commented 3 years ago

I think it needs to be part of the config per device. Imagine you have one super-old device and a lot of new ones. You don't want to slow everything down for just that one thing.

dshatokhin commented 3 years ago

That's a good point! But we're overloading one yaml file with config information IMO. Maybe it's time to read config-folder instead?

rfc1036 commented 3 years ago

It must really be possible to manually set the number of registers read for each modbus command like in my example: this is both the most simple thing to implement and the safest, because some devices may have requirements that a specific range is read and not one register too many. There is no point in trying to second-guess the operator here and if no ranges are listed then registers can continue to be read inefficiently one by one as they are now. (first register, lenght) pairs also map naturally to the wire protocol and to how vendors describe that the hardware should be accessed.

Perl-like pseudocode:

foreach range in (ranges_from_module_config) {
  (register, length) = parse_range(range);
  values = modbus_read(register, length);
  for (i = 0; i < length; i++) {
    read_values{register+i} = values[i];
  }
}

foreach metric in (metrics_from_module_config) {
  if (exists read_values{metric.register}) {
    report_metric(metric, read_values{metric.register});
  } else {
    values = modbus_read(register, 1);
    report_metric(metric, values[0]);
  }
}

(Currently I am spending some significant as well making my world better thanks to Prometheus and modbus... I lack the Go skills to contribute to this part, but I can provide theory and test code.)

dshatokhin commented 3 years ago

Hmmm, why not we're just specifying range by just one additional parameter - count:

metrics:
- name: "some_gauge"
  help: "some help for some gauge"
  address: 300023
  count: 12
  dataType: int16
  metricType: gauge

And then we'll start to exposing metrics such as some_gauge_0, some_gauge_1, some_gauge_2 etc. This way we can keep using all other parameters from metrics section (dataType, endianness, factor ...).

Maybe there is some king of design flaw which keeps being unnoticed by me šŸ¤”

rfc1036 commented 3 years ago

At least three major flaws:

dshatokhin commented 3 years ago

@rfc1036 maybe I'm understanding your idea in a wrong way. How you're going to name registers from specified ranges?

dshatokhin commented 3 years ago

So idea is to read ranges and then expose already scraped values as metrics, correct?

rfc1036 commented 3 years ago

Yes: reading by ranges is an optimization needed to not saturate the serial bus, but the metrics would still be configured as they are now. See my example in the issue post itself.

dshatokhin commented 3 years ago

Now we talking! Thank you for explaining your idea šŸ™ I'm not so confident in my golang knowledge, but seems like possible task for me (šŸ¤„)

Kampe commented 3 years ago

did this ever get merged?

rfc1036 commented 3 years ago

Actually it has not yet been written.

lod commented 3 years ago

I want this feature but I feel that all the discussion here is looking in the wrong direction.

This should be a code optimisation without requiring significant configuration changes.

I want to be able to specify three different addresses with their own different labels
I want the code to recognise that those addresses are adjacent, taking into account the data type
I want the code to fetch them as a single optimised block
I want the code to break that block out and present me with the three metrics as it currently does

I believe this addresses many of the issues raised above:

This will require adding a single new configuration value, maxRegisteReads or the like with a conservative value as discussed. The value could even initially be one, which will largely mimic the current behaviour.

This will require a significant restructuring of the current code base.

I believe this will be a significant improvement in speed. The low hanging fruit I see is bit access, currently each bit in the same address triggers another read. (Which could potentially fix side effects on alarm reset-on-read systems.)