Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Honor maximum message size limit of MQTT server #15009

Merged
merged 6 commits into from May 18, 2023

Conversation

underhood
Copy link
Contributor

@underhood underhood commented May 3, 2023

Summary

New MQTT broker on cloud size has maximum message size set to less than maximum MQTT message size.
We honor this limit now.

This gives us following change in behavior:
BEFORE:

  • if cloud requested data that were too big for the MQTT broker to handle we sent it anyway leading to MQTT broker disconnecting us. This led to temporary disruption of service for user until the agent reconnected.

AFTER:

  • if cloud requests data that are too big for MQTT broker we detect this and report it to cloud with special error code. This way cloud can show this to particular user (who initated the request) but all the other charts (and other users) will not experience any disruption.
Test Plan
Additional Information
For users: How does this change affect me?

@underhood underhood marked this pull request as ready for review May 11, 2023 19:52
@stelfrag
Copy link
Collaborator

Setting the limit to 32768 bytes will produce.

2023-05-12 11:15:15: netdata ERROR : ACLK_QRY[6] : [mqtt_wss] E: Message too big for server: 49021
2023-05-12 11:15:15: netdata ERROR : ACLK_QRY[6] : Failed to send cancelation message for http reply

So maybe we need to add the negotiated limit in the error message -- eg 49021 (max size = 32768) or something like that.

The next message Failed to send cancelation message for http reply is probably not correct in this case.

@underhood
Copy link
Contributor Author

So maybe we need to add the negotiated limit in the error message -- eg 49021 (max size = 32768) or something like that. I log the maximum when we establish a connection. I try to avoid repeating the same number on every log line to make logs compact

@underhood
Copy link
Contributor Author

rebased for conflict resolution + fixed bug

@underhood underhood requested a review from stelfrag May 17, 2023 12:59
stelfrag
stelfrag previously approved these changes May 17, 2023
thiagoftsm
thiagoftsm previously approved these changes May 17, 2023
Copy link
Contributor

@thiagoftsm thiagoftsm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR is working as expecting, LGTM!

@underhood underhood dismissed stale reviews from thiagoftsm and stelfrag via 6d23ea9 May 17, 2023 18:47
@underhood
Copy link
Contributor Author

after 2 approving reviews I merged changes in mqtt_websockets submodule from dev to master.
Last commit only changes submodule pointer to point to master branch 0 code changes.
@thiagoftsm @stelfrag Please re-approve at your convenience

@underhood underhood merged commit 5827732 into netdata:master May 18, 2023
124 of 126 checks passed
@underhood underhood deleted the honor_msgsizemax branch May 18, 2023 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants