feat: Modbus request optimization

[srebhan]

• [x] Updated associated README.md.
• [x] Wrote appropriate unit tests.
• [x] Pull request title or commits are in conventional commit format

replaces #11106

This PR adds a optimization parameter that allows for optimizing requests sent to the device. Currently there are four modes,

• none doing nothing besides confirming to the max request size and ignoring empty requests
• shrink removing leading and trailing fields that are omitted
• rearrange moving the request boundaries in order to reduce the total request size further while keeping the number of requests
• aggressive auto-filling gaps between requested fields reducing the number of requests at the cost of a larger total request size

the first mode is what is done in the current code. The second option is what #11106 is implementing. The last two are more advanced optimizations trying to reduce the total request size or the number of requests.

[TimurDela]

Dear @srebhan , I am sorry it took me so long to answer you on this. Nice work with the aggressive optimisation that allows reducing the number of requests even further. Please note that there must be some bug in the rearrange method that in some cases ends up in an infinite loop. Try running your unit test TestRequestOptimizationShrink by setting optimize to rearrange instead of shrink, go will panic.

However, I have been doing some reading concerning modbus and it looks like minimising the number of requests is not always the optimal solution. Each request comes with a cost but each field as well, this means that sometimes it can be better to split a large request into two if this means not touching many addresses we are not interested in. See this publication.

How much time it takes for each extra request and each extra field depends on the hardware and the network so there is no generic answer to what is optimal. One needs to try, measure, try, measure.

One approach that I found online is the following: set a limit on the number of "omitted" fields you are willing to accommodate in order to reduce the number of requests and optimise accordingly.

I have pushed on #11106 a possible implementation of this. Instead of choosing an optimisation mode, the user would set the value of max_extra_register between 0 (as many requests as non-consecutive fields) and 125 (as few requests as possible, whatever their size). As you can see, this requires minor changes to the actual code (only a few lines). I have added your "aggressive" unit test and it passes but with two different values of the max_extra_register parameter. This means no risk of infinite loops and a much faster startup (the aggressive optimisation takes a few seconds to make the list of requests).

The problem I see, however, is that I do not see how the user would choose the value of max_extra_register that optimises their hardware plus network setup. If one runs time telegraf --test, over a single interaction, the duration tends to fluctuate a bit and it is hard to see if it got better or worse. Is there a way to do telegraf --test 10 to test the input 10 times in a row?

[srebhan]

@TimurDela thanks for taking the time to test this!

Regarding the panic and the infinite loop: You cannot set rearrange in TestRequestOptimizationShrink as it will take forever (thus the panic probably) as the worst-case scenario is to hard for the current implementation. This is because we brute-force recursively optimize between group borders which scales with O(N!) where N is the number of groups. Therefore I added the big-fat warning in the readme... We probably can do better, but I didn't have time to look more deeply...

Regarding the tradeoff between requests vs. number of registers... Currently, rearrange minimizes the number of registers without changing the number of requests by moving the request borders around! For aggressive I do agree with your observation to add a optimization_max_fill parameter. Feel free to implement it on the basis of this PR as I intentionally implemented the underlying things in a way that people can add more strategies later.

[telegraf-tiger[bot]]

Download PR build artifacts for linux_amd64.tar.gz, darwin_amd64.tar.gz, and windows_amd64.zip. Downloads for additional architectures and packages are available below.

:relaxed: This pull request doesn't significantly change the Telegraf binary size (less than 1%)

:package: Click here to get additional PR build artifacts

Artifact URLs

DEB	RPM	TAR GZ	ZIP
amd64.deb	aarch64.rpm	darwin_amd64.tar.gz	windows_amd64.zip
arm64.deb	armel.rpm	darwin_arm64.tar.gz	windows_i386.zip
armel.deb	armv6hl.rpm	freebsd_amd64.tar.gz
armhf.deb	i386.rpm	freebsd_armv7.tar.gz
i386.deb	ppc64le.rpm	freebsd_i386.tar.gz
mips.deb	riscv64.rpm	linux_amd64.tar.gz
mipsel.deb	s390x.rpm	linux_arm64.tar.gz
ppc64el.deb	x86_64.rpm	linux_armel.tar.gz
riscv64.deb		linux_armhf.tar.gz
s390x.deb		linux_i386.tar.gz
		linux_mips.tar.gz
		linux_mipsel.tar.gz
		linux_ppc64le.tar.gz
		linux_riscv64.tar.gz
		linux_s390x.tar.gz
		static_linux_amd64.tar.gz

[TimurDela]

Hello @srebhan, sorry it took me so long but with holidays and then other tasks to work on, I only found the time to work on this now. The code is ready for your review but I do not have the rights to push to your repo srebhan/telegraf:modbus. Can you please explain to me how I add code to this pull-request?

I have tried to rebase my branch on master, merge your changes and then add mine but the first step has closed the pull request #11106 and my subsequent push have not reopened it, even though it says No new commits influxdata: modbus_request_grouping_#11105 was force-pushed and no longer has any new commits. Pushing new commits will allow the pull request to be re-opened.

Do you have the possibility to re-open PR #11106 ?

As you can see, on my fork, I have both your changes and mine: fork

[TimurDela]

Hello @srebhan, sorry to insist. Is there any way to reopen PR #11106 or should I start a new one?

[MyaLongmire]

@TimurDela I have reopened your pr :)

[TimurDela]

Thank you @MyaLongmire but it looks like I have lost the possibility to modify the PR. It is now a draft but I cannot change its status to open nor ask for reviews. Is there something you can do in the settings of the PR?

[powersj]

@srebhan - If I followed the comments correctly, we are favoring #11106 over this PR?

[srebhan]

@powersj and @TimurDela if you both agree I would like to first merge this PR and then add #11106 which extends this code in a nice way. This way, #11106 becomes smaller and can serve as an example of what needs to be done to add more optimization schemes... What do you think?

[telegraf-tiger[bot]]

Download PR build artifacts for linux_amd64.tar.gz, darwin_amd64.tar.gz, and windows_amd64.zip. Downloads for additional architectures and packages are available below.

:relaxed: This pull request doesn't significantly change the Telegraf binary size (less than 1%)

:package: Click here to get additional PR build artifacts

Artifact URLs

DEB	RPM	TAR GZ	ZIP
amd64.deb	aarch64.rpm	darwin_amd64.tar.gz	windows_amd64.zip
arm64.deb	armel.rpm	darwin_arm64.tar.gz	windows_arm64.zip
armel.deb	armv6hl.rpm	freebsd_amd64.tar.gz	windows_i386.zip
armhf.deb	i386.rpm	freebsd_armv7.tar.gz
i386.deb	ppc64le.rpm	freebsd_i386.tar.gz
mips.deb	riscv64.rpm	linux_amd64.tar.gz
mipsel.deb	s390x.rpm	linux_arm64.tar.gz
ppc64el.deb	x86_64.rpm	linux_armel.tar.gz
riscv64.deb		linux_armhf.tar.gz
s390x.deb		linux_i386.tar.gz
		linux_mips.tar.gz
		linux_mipsel.tar.gz
		linux_ppc64le.tar.gz
		linux_riscv64.tar.gz
		linux_s390x.tar.gz
		static_linux_amd64.tar.gz