feat(inputs.mqtt_consumer): Add incoming mqtt message size calculation #11426
Conversation
telegraf-tiger
bot
added
the
feat
Fetch upstream changes to resolve conflicts in order to open a pull request into my forked repo and test my plugin.
Hipska
added
area/mqtt
plugin/input
Hipska
changed the title
|
todo: can't get the mqtt_consumer.go file to recognize the new collect_bytes_received field from the configuration file in order to allow the data collection to be optional. |
… message comes in
|
Update: upon testing the plugin, I've found that it's calculating every single message as only 61 bytes (found this by dividing the size by the number of messages at various points in time, and would always get 61). I'm thinking that it has to do with the way I add the message size, perhaps it's calculating the size of the object type rather than the object itself? Any suggestions? |
|
Are you still working on it? Or is this ready to be reviewed? (Asking since it is still marked as draft) |
Co-authored-by: Thomas Casteleyn <thomas.casteleyn@me.com>
Hipska
added
the
ready for final review
Co-authored-by: Sven Rebhan <36194019+srebhan@users.noreply.github.com>
|
!retry-failed |
|
@sspaink another one not running CircleCI... :-( |
|
@mrro253 To get the tests to pass you will need to run |
sspaink
changed the title
| topicSize := len(msg.Topic()) + 2 | ||
| switch int(msg.Qos()) { | ||
| case 1: | ||
| qOsSize = 2 | ||
| qosFlagsSize = 4 | ||
| case 2: | ||
| qOsSize = 2 | ||
| qosFlagsSize = 12 | ||
| default: | ||
| qOsSize = 0 | ||
| } | ||
| payloadSize := len(msg.Payload()) | ||
| remainingContent := topicSize + qOsSize + payloadSize | ||
| if remainingContent < 1<<7 { | ||
| remainingLength = 1 | ||
| } else if remainingContent < 1<<14 { | ||
| remainingLength = 2 | ||
| } else if remainingContent < 1<<21 { | ||
| remainingLength = 3 | ||
| } else { | ||
| remainingLength = 4 | ||
| } | ||
| publishMessageSize := remainingLength + remainingContent + 1 | ||
| byteCount := publishMessageSize + qosFlagsSize |
There was a problem hiding this comment.
Thanks for these changes. They will be useful!
Should this only be reporting the payload size instead of the whole message size? The message has been parsed by now.
The rest of the code here seems to be making an educated guess on what the actual message size was at a lower level. I'm concerned that this isn't correct and there's no way to easily check it. I also wonder if the mqtt module will change and make this inaccurate in the future.
Is there a way to get the message size directly from eclipse/paho.mqtt.golang? If not, could we look into adding it upstream?
There was a problem hiding this comment.
Since the message has already been parsed, what I did to get the total incoming message size was work backwards from the given topic, payload, and flags. I don't want just the payload size, I want to get the total incoming packet size in order to know how much data usage a client is using. I was recommended to go to the eclipse/paho.mqtt.golang community to see if there was a way to get the message size directly from there, and the technique I implemented here was based off of their suggestion here. I suppose it could be possible to add this metric directly to eclipse/paho.mqtt.golang but what would be the advantages of doing it there rather than here?
There was a problem hiding this comment.
A message in that thread illustrates my concern:
... and if it's MQTT v5 then you need to add some more on to account for properties, but you won't absolutely get all the information that you need to correctly calculate the size in a user library."
The calculations here may be accurate for your use case, but evidently they're not accurate for all use cases (at least not v5) and it may not be possible to know when they're accurate.
I do think the payload size and number of messages are great internal metrics because they're directly measured, not derived.
There was a problem hiding this comment.
I see what you mean. So would you recommend I change it to just collect the payload size and number of messages metrics rather than trying to get the total size of the message?
There was a problem hiding this comment.
I don't see anything preventing this from being merged if we remove the code to calculate the total size. Maybe we should go ahead with that, then if/when we find a way to directly measure the total size we can add it in another PR.
|
Download PR build artifacts for linux_amd64.tar.gz, darwin_amd64.tar.gz, and windows_amd64.zip. 🥳 This pull request decreases the Telegraf binary size by -1.86 % for linux amd64 (new size: 147.6 MB, nightly size 150.4 MB) 📦 Click here to get additional PR build artifactsArtifact URLs |
Required for all PRs:
resolves #11334
Gets the size of each component of the incoming mqtt message as described here then adds it to the selfstat object so that it can be accessed by the internal plugin. Also gets the amount of messages received.