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

[ADDED] MQTT: test/bench using external client #4821

Merged
merged 1 commit into from
Feb 6, 2024
Merged

Conversation

levb
Copy link
Contributor

@levb levb commented Nov 27, 2023

This PR adds a number of MQTT-specific benchmarks, executed with an external test client (see ConnectEverything/mqtt-test#1). This came out of my work on MQTT message delivery issues in 2.10, and is now packaged as a go test --bench, and added to CI.

The test client uses the Eclipse paho library, and can be executed standalone, pointed at an MQTT server. It measures the timing of its operations, and outputs it as JSON of reportable metrics.

The benchmarks here run the client, and report back the metrics it outputs. Memory allocations are measured server-only.

Github action benchmark is used to compare the benchmark results to the base ref of the PR, or the prior result in a branch. If it fails a 120% threshold, the failure is reported as a comment on the commit(!), and the build fails.

To measure the effect of parallel-fetching retained messages (SUBRET), added disable_retained_message_cache MQTT option to the server.

The following benchmarks are run, covering standalone/cluster, QOS, message sizes to 100K, as well as concurrent clients.

  • PUB measures just publishing
  • PUBRET measures publishing with the retained flag on
  • PUBSUB measures message delivery times from publisher to subscriber
  • SUBRET measures delivery of 100* retained messages to new subscriptions

Example of a failure (I rolled back the retained message "fanout" commit):
image
if you click on the comment link, you get:
image

TODO
  • CON measure CONNECT to CONNACK, with stored sessions
  • SUB measuring SUBSCRIBE to SUBACK

@levb levb changed the title TEST ONLY: Added test/bench for MQTT using external clients [ADDED] MQTT: test/bench using external client Jan 9, 2024
@levb levb marked this pull request as ready for review January 15, 2024 14:51
@levb levb requested a review from a team as a code owner January 15, 2024 14:51
@levb levb requested review from wallyqs and mprimi January 15, 2024 14:51
Copy link
Member

@derekcollison derekcollison left a comment

Choose a reason for hiding this comment

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

So this adds in an option to disable the retained msg cache. When would one want to do this? Why are we exposing it I guess is my question.

PR looks fine, just looking for some clarifications.

@levb
Copy link
Contributor Author

levb commented Jan 15, 2024

@derekcollison the option is used only for testing, and is exposed because we use config files in the test utility functions to initialize servers/clusters.

I could move the flag into the (server.)srvMQTT struct, set it after the server(s) have been started, would that be preferable? If there's a better mechanism of passing "test options" into a server, please advise.

@derekcollison
Copy link
Member

Let's see what @kozlovic has to say, he has done a few of these testing flags / switches for things.

Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

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

Assuming the proposed change does not cause races (tests not running in parallel), it could be a better way since we would not introduce an non exposed option.

server/mqtt.go Outdated Show resolved Hide resolved
server/mqtt_ex_test.go Outdated Show resolved Hide resolved
server/opts.go Outdated Show resolved Hide resolved
server/opts.go Outdated Show resolved Hide resolved
Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@wallyqs wallyqs left a comment

Choose a reason for hiding this comment

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

LGTM

@derekcollison derekcollison merged commit ac86e01 into main Feb 6, 2024
4 checks passed
@derekcollison derekcollison deleted the lev-mqtt-benchex branch February 6, 2024 17:03
wallyqs added a commit that referenced this pull request Feb 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants