feat(inputs.modbus): Optimise grouped requests #11106

TimurDela · 2022-05-17T06:09:57Z

Simple modification to introduce an optimise_requests flag in the configuration setup that, when set to true

minimises the number of requests when a large number of fields is omitted
removes the trailing omitted fields from every request so the request is not longer than needed

As a result, the total number of request is generally lower, also improving performance

Required for all PRs:

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

adds a new field optimise_requests in requestDefinition
when true, groupFieldsToRequests switches to groupFieldsToRequestsOptimised that
- always starts requests with non-omitted fields
- shortens the requests if their last fields are flagged as omitted

This results in fewer and shorter requests when the user specifies many (several hundred) fields, many of which are omitted, hence increasing performance.

srebhan

Thanks for tackling this issue @TimurDela! Well spotted and you are completely right that this needs fixing.

I have some comments in the code, but for the background: We introduced the omit flag to give the user more control over what is actually sent to the device. There might be strange behaviors, we have for example seen devices requiring to read coil starting from address zero and there might be others with similar constraints.
Therefore, I would not just skip leading omits as they might be required but keep the starting address. We should omit the field assignment though...
Furthermore, I would filter non-empty requests (the once filling at least one field) at the end of groupFieldsToRequests as this simplifies the logic. What do you think?

srebhan · 2022-05-19T14:21:28Z

plugins/inputs/modbus/modbus_test.go

+	require.Equal(t, uint16(2), modbus.requests[1].holding[0].address)
+	require.Equal(t, uint16(4), modbus.requests[1].holding[0].length)


I think we should not mess with the addresses here (see my overall comment) but rather check that we only scrape out one field from this request

Suggested change

require.Equal(t, uint16(2), modbus.requests[1].holding[0].address)

require.Equal(t, uint16(4), modbus.requests[1].holding[0].length)

require.Len(t, modbus.requests[1].holding[0].fields, 1)

srebhan · 2022-05-19T14:23:05Z

plugins/inputs/modbus/modbus_test.go

+	require.NoError(t, modbus.Init())
+	require.NotEmpty(t, modbus.requests)
+	require.NotNil(t, modbus.requests[1])
+	require.Len(t, modbus.requests[1].holding, 0)


It's a matter of taste, but I like require.Empty as it is more speaking

Suggested change

require.Len(t, modbus.requests[1].holding, 0)

require.Empty(t, modbus.requests[1].holding)

srebhan · 2022-05-19T14:25:40Z

plugins/inputs/modbus/request.go

-				current.fields = append(current.fields, f)
+				if isNonEmptyRequest(current) {
+					current.fields = append(current.fields, f)
+				} else {
+					current = newRequest(f, tags)
+				}


How about taking another approach here? For the "starting with omit" case we could simply modify newRequest to omit the field like

func newRequest(f field, tags map[string]string) request { r := request{ address: f.address, length: f.length, fields: []field{}, tags: map[string]string{}, } if !f.omit { r.fields = append(r.fields, f) } // Copy the tags for k, v := range tags { r.tags[k] = v } return r }

and later check the requests. See below.

srebhan · 2022-05-19T14:27:02Z

plugins/inputs/modbus/request.go

+	if isNonEmptyRequest(current) {
+		requests = append(requests, current)
+	}


Instead of all of this I would just check if the request will fill a field and delete it otherwise

Suggested change

if isNonEmptyRequest(current) {

requests = append(requests, current)

}

requests = append(requests, current)

// Filter out completely empty requests

nonEmptyRequests := make([]request, 0, len(requests))

for _, request := range requests {

if len(request.fields) != 0 {

nonEmptyRequests = append(nonEmptyRequests, request)

}

}

and then return nonEmptyRequests below.

TimurDela · 2022-05-21T04:22:10Z

Hi @srebhan,
I was not aware that some devices required the address 0 to be queried.
However, my problem is that I have a device with around 1300 signals. My team is only interested in ~50 of them scattered over the entire address range. Sitting down to try to find the most efficient way to group my queries, taking into account the maximum size of one query (which depends on the data types) is a lot of work that would need to be repeated every time I am asked to add or remove a signal. I would prefer to simply provide the entire list of addresses to the Telegraf config file and set the few signals I am interested in to omit=false.

If I use the code I pushed, my configuration leads to 7 requests. If I use your suggested code and throw out only the empty requests, I end up with 21 requests, which becomes incompatible with how often I need to log the data.

Could we think of a way to have both behaviours and let the users decide which one is more appropriate for their needs?

srebhan · 2022-05-23T08:33:27Z

@TimurDela how do you get 21 requests? In my suggestions I'm dropping completely empty requests, so I cannot think of any configuration where 50 registers in 1300 entries cause 21 requests. Can you please provide an example?

The only change I request to your code is to not drop leading omits in requests. I can see that this makes sense, but it would need an additional config option for the user to explicitly enable it. I'm open to discuss this.

My suggestion is to split this PR. Please move fixing the bug with not omitting the first field to an own PR and keep the "empty-filtering" in this PR. Does that make sense to you?

TimurDela · 2022-05-27T05:37:02Z

For the holding registerType, maxBatchSize is 125. Most of my entries are float32 and take 2 addresses each, so that is maximum 62 fields per request. 1300 / 62 = 21. But In fact when I follow your suggestion I get 12 requests which is better than 21 but still suboptimal.

I will split this pull request in two as you ask. Please have a look at #11202 .
How should I proceed with this pull request? Wait for you to merge #11202 and rebase this one or add new changes directly taking the chance of having merge conflicts?

srebhan · 2022-05-27T09:12:47Z

My suggestion of the further procedure would be to first get #11202 ready. In the meantime, I want to ask you to fix the "omit first field" bug, maybe in this PR or you split it out to another.

If you furthermore, need the above optimization, you should do it in this PR. Please guard the "new" behavior with a flag (e.g. optimize_heading_omits or similar) or the other way, add a workaround option, e.g. use_strict_omit.

How does that sound to you?

TimurDela · 2022-06-07T05:38:43Z

Hello @srebhan , is there something else you want me to modify in this pull request?

srebhan · 2022-06-09T07:35:44Z

@TimurDela after thinking a bit longer on your request I realized that there are solutions closer to the optimum and came up with #11273. Can you give it a try so we can discuss the further proceeding? Your test is integrated and the result matches your expectations. However, you can optimize further by setting optimization = "rearrange" in the request which tries to find optimal request boundaries wrt the total request size.

Looking forward to your comments.

TimurDela · 2022-06-09T07:55:17Z

Thank you @srebhan
I will have a look at it as soon as I can but it may take a few days.

TimurDela · 2022-06-18T06:57:52Z

I have left a few fmt.Printf here and there for comparing with #11273 but of course they are meant to be removed

srebhan · 2022-06-22T13:32:06Z

Can you please use #11273 as a basis for your implementation by adding another strategy there? I intentionally kept my PR open for additional strategies to make it extensible.

TimurDela · 2022-06-22T14:07:30Z

Just to be clear, do you want me to rebase this pull request on top of srebhan:modbus or do you want me to add my changes into pull request #11273 ?

srebhan · 2022-06-27T11:50:03Z

I want to add your optimization scheme on-top of #11273. Give it another name (e.g. optimization = "max_insert) and use the groups concept to implement it. Does that make sense to you?

remove extra empty line Co-authored-by: Joshua Powers <powersj@fastmail.com>

Co-authored-by: Sven Rebhan <36194019+srebhan@users.noreply.github.com>

TimurDela · 2022-11-15T12:36:32Z

Thank you @srebhan. This PR is now rebased and the outputs have been moved to Init() and are only active with the -debug flag.

Tests fail for the Windows test suite but the error messages mention plugins I have not modified (google_cloud_storage and statsd) so I am not sure what to do about it.

srebhan

The code looks quite good now @TimurDela! I have some smaller issues like naming or documentation style. However, the size of the test-diff worries me. Can you please try to get this into a more readable state, e.g. by reverting the modbus_test.go to the current master and re-add your tests in a suitable place?

plugins/inputs/modbus/README.md

plugins/inputs/modbus/configuration_request.go

plugins/inputs/modbus/modbus.go

plugins/inputs/modbus/modbus_test.go

plugins/inputs/modbus/request.go

plugins/inputs/modbus/sample_request.conf

plugins/inputs/modbus/request.go

plugins/inputs/modbus/configuration_request.go

plugins/inputs/modbus/README.md

TimurDela · 2022-11-16T08:02:03Z

Thank you @srebhan I have now implemented your suggestions and cleaned up the test file

telegraf-tiger · 2022-11-23T07:56:54Z

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.

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

📦 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

TimurDela · 2022-11-23T07:57:19Z

Hello @srebhan I just solved new conflicts with master but I would really appreciate if we could merge this PR before the next change on modbus so I don't have to do that again.

srebhan

This looks really beautiful @TimurDela! Thanks for all your effort and the valuable contribution!

TimurDela · 2022-11-30T11:52:34Z

Thank you @srebhan for the very useful feedback. I am really happy this is going to be on the main branch.

powersj

Thank you for the PR and your patience!

TimurDela changed the title ~~fix(inputs/modbus) #11105 no empty grouped requests~~ fix(inputs/modbus): #11105 no empty grouped requests May 17, 2022

srebhan requested changes May 19, 2022

View reviewed changes

srebhan added fix pr to fix corresponding bug area/modbus plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins labels May 19, 2022

srebhan self-assigned this May 19, 2022

TimurDela changed the title ~~fix(inputs/modbus): #11105 no empty grouped requests~~ feature(inputs/modbus): optimise grouped requests May 27, 2022

TimurDela changed the title ~~feature(inputs/modbus): optimise grouped requests~~ feat(inputs/modbus): optimise grouped requests May 27, 2022

TimurDela mentioned this pull request May 27, 2022

fix(inputs/modbus): #11105 fix requests starting with an omitted field #11202

Merged

3 tasks

TimurDela marked this pull request as draft May 30, 2022 08:14

TimurDela force-pushed the modbus_request_grouping_#11105 branch from 638f0d7 to cf8a047 Compare June 2, 2022 03:18

TimurDela marked this pull request as ready for review June 2, 2022 03:24

TimurDela requested a review from srebhan June 2, 2022 03:24

TimurDela marked this pull request as draft June 2, 2022 03:34

TimurDela marked this pull request as ready for review June 2, 2022 04:21

srebhan mentioned this pull request Jun 8, 2022

feat(inputs.modbus): Optimize requests #11273

Merged

3 tasks

TimurDela marked this pull request as draft June 18, 2022 06:20

TimurDela force-pushed the modbus_request_grouping_#11105 branch from 575eb44 to 46feed7 Compare June 18, 2022 07:00

TimurDela closed this Sep 7, 2022

TimurDela and others added 4 commits November 15, 2022 12:01

Update plugins/inputs/modbus/request.go

f415047

remove extra empty line Co-authored-by: Joshua Powers <powersj@fastmail.com>

Update plugins/inputs/modbus/configuration_request.go

2c4839c

Co-authored-by: Sven Rebhan <36194019+srebhan@users.noreply.github.com>

review by srebhan

530704b

rebase and move query info to debug mode

89a2cdf

TimurDela force-pushed the modbus_request_grouping_#11105 branch from e881f7e to 89a2cdf Compare November 15, 2022 12:02

Timur added 2 commits November 15, 2022 13:07

linting readme

a0853b8

more linting for readme

81b78d8

TimurDela requested review from srebhan and removed request for powersj November 15, 2022 13:01

srebhan reviewed Nov 15, 2022

View reviewed changes

Timur added 3 commits November 16, 2022 08:09

better naming and readme. rebase issue on test file fixed

c3aaa86

fix index error

8d642ea

fix readme

00d9f69

TimurDela requested a review from srebhan November 16, 2022 08:03

Timur added 2 commits November 23, 2022 08:13

Merge branch 'master' into modbus_request_grouping_#11105

d462df5

attempt to fix lint

f594014

TimurDela requested review from powersj and removed request for srebhan November 28, 2022 13:27

srebhan approved these changes Nov 30, 2022

View reviewed changes

srebhan assigned powersj and unassigned srebhan Nov 30, 2022

srebhan 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 Nov 30, 2022

srebhan changed the title ~~feat(inputs/modbus): optimise grouped requests~~ feat(inputs.modbus): Optimise grouped requests Nov 30, 2022

powersj approved these changes Nov 30, 2022

View reviewed changes

powersj merged commit edb2358 into influxdata:master Nov 30, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(inputs.modbus): Optimise grouped requests #11106

feat(inputs.modbus): Optimise grouped requests #11106

TimurDela commented May 17, 2022 •

edited

Loading

srebhan left a comment

srebhan May 19, 2022

srebhan May 19, 2022

srebhan May 19, 2022

srebhan May 19, 2022

TimurDela commented May 21, 2022

srebhan commented May 23, 2022

TimurDela commented May 27, 2022 •

edited

Loading

srebhan commented May 27, 2022

TimurDela commented Jun 7, 2022

srebhan commented Jun 9, 2022

TimurDela commented Jun 9, 2022

TimurDela commented Jun 18, 2022

srebhan commented Jun 22, 2022

TimurDela commented Jun 22, 2022

srebhan commented Jun 27, 2022

TimurDela commented Nov 15, 2022

srebhan left a comment

TimurDela commented Nov 16, 2022

telegraf-tiger bot commented Nov 23, 2022

Artifact URLs

TimurDela commented Nov 23, 2022

srebhan left a comment

TimurDela commented Nov 30, 2022

powersj left a comment

		require.Equal(t, uint16(2), modbus.requests[1].holding[0].address)
		require.Equal(t, uint16(4), modbus.requests[1].holding[0].length)

	require.Equal(t, uint16(2), modbus.requests[1].holding[0].address)
	require.Equal(t, uint16(4), modbus.requests[1].holding[0].length)
	require.Len(t, modbus.requests[1].holding[0].fields, 1)

	require.Len(t, modbus.requests[1].holding, 0)
	require.Empty(t, modbus.requests[1].holding)

-	if isNonEmptyRequest(current) {
-		requests = append(requests, current)
-	}
+	requests = append(requests, current)
+	// Filter out completely empty requests
+	nonEmptyRequests := make([]request, 0, len(requests))
+	for _, request := range requests {
+		if len(request.fields) != 0 {
+			nonEmptyRequests = append(nonEmptyRequests, request)
+		}
+	}

feat(inputs.modbus): Optimise grouped requests #11106

feat(inputs.modbus): Optimise grouped requests #11106

Conversation

TimurDela commented May 17, 2022 • edited Loading

Required for all PRs:

srebhan left a comment

Choose a reason for hiding this comment

srebhan May 19, 2022

Choose a reason for hiding this comment

srebhan May 19, 2022

Choose a reason for hiding this comment

srebhan May 19, 2022

Choose a reason for hiding this comment

srebhan May 19, 2022

Choose a reason for hiding this comment

TimurDela commented May 21, 2022

srebhan commented May 23, 2022

TimurDela commented May 27, 2022 • edited Loading

srebhan commented May 27, 2022

TimurDela commented Jun 7, 2022

srebhan commented Jun 9, 2022

TimurDela commented Jun 9, 2022

TimurDela commented Jun 18, 2022

srebhan commented Jun 22, 2022

TimurDela commented Jun 22, 2022

srebhan commented Jun 27, 2022

TimurDela commented Nov 15, 2022

srebhan left a comment

Choose a reason for hiding this comment

TimurDela commented Nov 16, 2022

telegraf-tiger bot commented Nov 23, 2022

Artifact URLs

TimurDela commented Nov 23, 2022

srebhan left a comment

Choose a reason for hiding this comment

TimurDela commented Nov 30, 2022

powersj left a comment

Choose a reason for hiding this comment

TimurDela commented May 17, 2022 •

edited

Loading

TimurDela commented May 27, 2022 •

edited

Loading