feat(outputs.mqtt): Add support for MQTT protocol version 5 #11284

cmackenzie1 · 2022-06-13T01:54:35Z

Required for all PRs:

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

Summary

By refactoring into versioned clients and adding github.com/eclipse/paho.golang, the MQTT output plugin can now support MQTT protocols 3.1.1 and 5, with the default remaining 3.1.1

Other notable changes:

The list of servers in the config can now be prefixed with a scheme (ie, mqtt:// or mqtts://), with the default being tcp:// if not present to maintain original behavior.
The write timeout is for protocol 5 client is done using context.WithTimeout.
MQTT config was reordered to group similar properties together and toml tags were added for all exported fields.

srebhan

Looks mostly good. Thanks for your submission @cmackenzie1! I do have some small suggestions. Can you please take a look?

plugins/outputs/mqtt/mqtt.go

cmackenzie1 · 2022-06-13T13:46:26Z

@srebhan upon digging into the paho.golang client code, I see they mention v3 protocol but explicitly set it to v5. https://github.com/eclipse/paho.golang/blob/master/paho/client.go#L211-L212

Thoughts on using both the new and old libraries and adding a field Procotol string to the MQTT struct?

srebhan · 2022-06-14T15:28:16Z

@cmackenzie1 do you know if the paho guys are planning to support both in one library? The setting might be an oversight or something...

If this is not an issue they want to fix, we should indeed look into supporting both libraries with a Protocol user-setting as a last resort. ;-)

cmackenzie1 · 2022-06-14T16:20:44Z

@srebhan - just created eclipse-paho/paho.golang#89 to see what the status is. There is no clear answer in the docs from what I can tell.

MattBrittan · 2022-06-17T22:22:34Z

Note that the v5 package does not yet support persistence (this negates the point of QOS1/2).

As per the response on the issue raised on paho.golang there is no current plan to support MQTT 3.11.

srebhan · 2022-06-22T13:17:13Z

@cmackenzie1 so you need to go with both versions then?

cmackenzie1 · 2022-06-22T19:50:41Z

@srebhan I think so. I'll try and work on the PR this week during my downtime.

srebhan · 2022-07-12T07:41:45Z

@cmackenzie1 any update on this PR?

cmackenzie1 · 2022-07-12T17:36:27Z

Apologies @srebhan - I've been on a mix of vacation and being sick. Did you want me to close the PR for now until I get some time to work on it?

cmackenzie1 · 2022-07-14T01:31:55Z

@srebhan I managed to get some time this afternoon to work on the change to support multiple clients. Ready for a review at your leisure!

srebhan

Thanks for taking time to update this PR @cmackenzie1! I didn't mean to urge your with this, I just wanted to make sure this PR is not forgotten. :-)
The code looks nice and clean. Just some minor comments. One thing that would be nice is if you can add an integration test for version 5 similar to what is there for v3 as I think this is not covered currently....

go.mod

plugins/outputs/mqtt/README.md

plugins/outputs/mqtt/sample.conf

plugins/outputs/mqtt/mqtt.go

plugins/outputs/mqtt/mqtt_v3.go

plugins/outputs/mqtt/mqtt_v5.go

cmackenzie1 · 2022-07-15T23:27:59Z

@srebhan no worries, I addressed those comments and even switched the test docker image over to use the official image from Eclipse.

srebhan

That looks very good @cmackenzie1. Thanks for your quick update. I only have one more comment regarding your tests. I suggest to put the server configuration to a file and ship it instead of temporary file creation. That will save a lot of trouble and code. :-)

plugins/outputs/mqtt/mqtt_test.go

srebhan · 2022-07-20T09:56:00Z

Oh and please rebase to latest master as something strange happend to the CI tests. I'm pretty sure that you need to add github.com/eclipse/paho.golang to docs/LICENSE_OF_DEPENDENCIES.md. You can check locally by running make check-deps...

cmackenzie1 · 2022-07-21T00:08:52Z

Rebased, updated to include a testdata file with mosquitto.conf, and added the dependency docs/LICENSE_OF_DEPENDENCIES.md.!

srebhan · 2022-07-21T15:11:38Z

@cmackenzie1 it also seems like you need to run make tidy to make the checks happy.

cmackenzie1 · 2022-07-21T15:22:06Z

@srebhan ran make tidy, didn't like the order I guess... https://github.com/influxdata/telegraf/compare/42e89769f6e75a9f22368acb12b441081f5f8dc8..cfdbc7caa682c1ce9c8c60f391398d573f7d20a5

srebhan · 2022-07-21T16:36:54Z

Yeah go mod is picky... :-) Can you please also take a look at the other comments?

cmackenzie1 · 2022-07-21T16:38:31Z

@srebhan I think I addressed them already, unless they are still in "pending" on your end.

srebhan · 2022-07-22T12:58:40Z

Oh boy, forgot to submit my review.... ;-)

srebhan

Hey @cmackenzie1, sorry forgot to submit the review... Just two more comments for the test-cases and we are good to go from my side...

plugins/outputs/mqtt/mqtt_test.go

By refactoring into versioned clients and adding `github.com/eclipse/paho.golang`, the MQTT output plugin can now support MQTT protocols `3.1.1` and `5`, with the default remaining `3.1.1` Other notable changes: - The list of servers in the config can now be prefixed with a scheme (ie, `mqtt://` or `mqtts://`), with the default being `tcp://` if not present to maintain original behavior. - The write timeout is for protocol `5` client is done using `context.WithTimeout`. - `MQTT` config was reordered to group similar properties together and `toml` tags were added for all exported fields. - `MQTT` integration tests were switched to the official [`eclipse-mosquitto`](https://hub.docker.com/_/eclipse-mosquitto) docker image

cmackenzie1 · 2022-07-26T01:05:18Z

@srebhan no worries! I addressed those two comments and rebased onto the latest master.

srebhan

Perfect. Thank you @cmackenzie1 for all your effort!

MyaLongmire

Thank you so much for adding this!

telegraf-tiger bot added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Jun 13, 2022

cmackenzie1 changed the title ~~feat(output.mqtt): github.com/eclipse/paho.mqtt.golang with githubcom/eclipse/paho.golang~~ feat(output.mqtt): github.com/eclipse/paho.mqtt.golang with github.com/eclipse/paho.golang Jun 13, 2022

srebhan changed the title ~~feat(output.mqtt): github.com/eclipse/paho.mqtt.golang with github.com/eclipse/paho.golang~~ feat(output.mqtt): Replace github.com/eclipse/paho.mqtt.golang with github.com/eclipse/paho.golang Jun 13, 2022

srebhan reviewed Jun 13, 2022

View reviewed changes

plugins/outputs/mqtt/mqtt.go Outdated Show resolved Hide resolved

plugins/outputs/mqtt/mqtt.go Outdated Show resolved Hide resolved

plugins/outputs/mqtt/mqtt.go Outdated Show resolved Hide resolved

plugins/outputs/mqtt/mqtt.go Outdated Show resolved Hide resolved

srebhan self-assigned this Jun 13, 2022

srebhan added area/mqtt plugin/output 1. Request for new output plugins 2. Issues/PRs that are related to out plugins labels Jun 13, 2022

Hipska mentioned this pull request Jun 13, 2022

MQTT Consumer: Support Reading User-Properties #11258

Open

cmackenzie1 changed the title ~~feat(output.mqtt): Replace github.com/eclipse/paho.mqtt.golang with github.com/eclipse/paho.golang~~ feat(output.mqtt): Add support for MQTT protocol version 5 Jul 14, 2022

srebhan reviewed Jul 15, 2022

View reviewed changes

srebhan reviewed Jul 20, 2022

View reviewed changes

plugins/outputs/mqtt/mqtt_test.go Outdated Show resolved Hide resolved

srebhan reviewed Jul 22, 2022

View reviewed changes

plugins/outputs/mqtt/mqtt_test.go Outdated Show resolved Hide resolved

plugins/outputs/mqtt/mqtt_test.go Outdated Show resolved Hide resolved

srebhan approved these changes Jul 27, 2022

View reviewed changes

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 Jul 27, 2022

srebhan changed the title ~~feat(output.mqtt): Add support for MQTT protocol version 5~~ feat(outputs.mqtt): Add support for MQTT protocol version 5 Jul 27, 2022

MyaLongmire approved these changes Jul 27, 2022

View reviewed changes

MyaLongmire merged commit af43d01 into influxdata:master Jul 27, 2022

cmackenzie1 deleted the cole/mqtt-protocol-version branch July 27, 2022 15:42

sjwang90 mentioned this pull request Sep 7, 2022

Telegraf 1.24 influxdata/docs-v2#4422

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(outputs.mqtt): Add support for MQTT protocol version 5 #11284

feat(outputs.mqtt): Add support for MQTT protocol version 5 #11284

cmackenzie1 commented Jun 13, 2022 •

edited

Loading

srebhan left a comment

cmackenzie1 commented Jun 13, 2022

srebhan commented Jun 14, 2022

cmackenzie1 commented Jun 14, 2022

MattBrittan commented Jun 17, 2022

srebhan commented Jun 22, 2022

cmackenzie1 commented Jun 22, 2022

srebhan commented Jul 12, 2022

cmackenzie1 commented Jul 12, 2022

cmackenzie1 commented Jul 14, 2022

srebhan left a comment

cmackenzie1 commented Jul 15, 2022

srebhan left a comment

srebhan commented Jul 20, 2022

cmackenzie1 commented Jul 21, 2022

srebhan commented Jul 21, 2022

cmackenzie1 commented Jul 21, 2022

srebhan commented Jul 21, 2022

cmackenzie1 commented Jul 21, 2022

srebhan commented Jul 22, 2022

srebhan left a comment

cmackenzie1 commented Jul 26, 2022

srebhan left a comment

MyaLongmire left a comment

feat(outputs.mqtt): Add support for MQTT protocol version 5 #11284

feat(outputs.mqtt): Add support for MQTT protocol version 5 #11284

Conversation

cmackenzie1 commented Jun 13, 2022 • edited Loading

Required for all PRs:

Summary

srebhan left a comment

Choose a reason for hiding this comment

cmackenzie1 commented Jun 13, 2022

srebhan commented Jun 14, 2022

cmackenzie1 commented Jun 14, 2022

MattBrittan commented Jun 17, 2022

srebhan commented Jun 22, 2022

cmackenzie1 commented Jun 22, 2022

srebhan commented Jul 12, 2022

cmackenzie1 commented Jul 12, 2022

cmackenzie1 commented Jul 14, 2022

srebhan left a comment

Choose a reason for hiding this comment

cmackenzie1 commented Jul 15, 2022

srebhan left a comment

Choose a reason for hiding this comment

srebhan commented Jul 20, 2022

cmackenzie1 commented Jul 21, 2022

srebhan commented Jul 21, 2022

cmackenzie1 commented Jul 21, 2022

srebhan commented Jul 21, 2022

cmackenzie1 commented Jul 21, 2022

srebhan commented Jul 22, 2022

srebhan left a comment

Choose a reason for hiding this comment

cmackenzie1 commented Jul 26, 2022

srebhan left a comment

Choose a reason for hiding this comment

MyaLongmire left a comment

Choose a reason for hiding this comment

cmackenzie1 commented Jun 13, 2022 •

edited

Loading