feat: add optional break conditions to `tedge mqtt sub` by rina23q · Pull Request #3445 · thin-edge/thin-edge.io

rina23q · 2025-03-04T16:02:15Z

Proposed changes

This PR introduces two new options to tedge mqtt sub to give a break to the command.

  -W, --duration <DURATION>
          Set a timeout duration (e.g., 60s, 1h)

  -C, --count <COUNT>
          Set the number of packets before stopping

--duration: tedge mqtt sub breaks after given time. e.g.,

tedge mqtt sub "#" --duration 10s

--count: tedge mqtt sub breaks after the given number of packets received. e.g.,

$ tedge mqtt sub "#" --count 3
INFO: Connected
[te/device/main/service/c8y-firmware-plugin] {"@parent":"device/main//","@type":"service","type":"service"}
[te/device/main/service/c8y-firmware-plugin/status/health] {"pid":1400,"status":"down"}
[te/device/main/service/mosquitto-c8y-bridge] {"@id":"rina0010:device:main:service:mosquitto-c8y-bridge","@parent":"device/main//","@type":"service","name":"mosquitto-c8y-bridge","type":"service"}
INFO: Break

Types of changes

Bugfix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Improvement (general improvements like code refactoring that doesn't explicitly fix a bug or add any new functionality)
Documentation Update (if none of the other choices apply)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Paste Link to the issue

#3412

Checklist

I have read the CONTRIBUTING doc
I have signed the CLA (in all commits with git commit -s)
I ran cargo fmt as mentioned in CODING_GUIDELINES
I used cargo clippy as mentioned in CODING_GUIDELINES
I have added tests that prove my fix is effective or that my feature works
I have added necessary documentation (if appropriate)

Further comments

codecov · 2025-03-04T16:15:15Z

Codecov Report

Attention: Patch coverage is 0% with 23 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
crates/core/tedge/src/cli/mqtt/subscribe.rs	0.00%	19 Missing ⚠️
crates/core/tedge/src/cli/mqtt/cli.rs	0.00%	4 Missing ⚠️

Additional details and impacted files

📢 Thoughts on this report? Let us know!

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

reubenmiller · 2025-03-04T18:21:36Z

@rina23q Just naming things. It might be more useful to align with naming used by mosquitto_sub (to make it easier to switch between the tools)

--window => --duration and with a short option match mosquittos_sub -W. I'd originally thought about --timeout...but that is to unclear e.g. timeout of what, connecting to the broker?
--packets => --count and with a short option match mosquittos_sub -C (uppercase)

Plus, it the previous names (window and packets) are super obvious what they mean.

albinsuresh

LGTM. Agree to Reuben's naming suggestions. Also would be nice to add some basic system tests.

albinsuresh · 2025-03-05T06:19:53Z

crates/core/tedge/src/cli/mqtt/subscribe.rs


+    // the default keepalive in rumqttc is 60 sec
+    if !cmd.window.is_zero() && cmd.window.as_secs() < 60 {
+        options.set_keep_alive(cmd.window);


I'm okay with that. But be aware that the precision is 60 sec if the timeout is greater than 60s.

An option, is to set the keep alive to 5 seconds to have a more precise timeout.

If user gives 1m10s, then indeed with 60s keepalive, the command will break after 2m.
On the other hand, if user provides 1h, every 5s ping sounds a bit too much.

Does it make simplify if we set the same keepalive value as provided unless it's bigger than the maximum value?

the maximum Keep Alive interval is 18 hours, 12 minutes, and 15 seconds. (https://www.hivemq.com/blog/mqtt-essentials-part-10-alive-client-take-over/)

if user provides 1h, every 5s ping sounds a bit too much.

I would not bother too much with that. This command being used locally and for test purposes.

Changed in 2088490

didier-wenzek · 2025-03-05T08:47:20Z

crates/core/tedge/src/cli/mqtt/cli.rs

+        /// Set a timeout duration (e.g., 60s, 1h). Use 0 for no timeout
+        #[clap(long, default_value = "0")]
+        window: SecondsOrHumanTime,
+        /// Set the number of packets before stopping. Use 0 for unlimited
+        #[clap(long, default_value = "0")]
+        packets: u32,


A cleaner approach is to use optional inputs instead of magic values.

Suggested change

/// Set a timeout duration (e.g., 60s, 1h). Use 0 for no timeout

#[clap(long, default_value = "0")]

window: SecondsOrHumanTime,

/// Set the number of packets before stopping. Use 0 for unlimited

#[clap(long, default_value = "0")]

packets: u32,

/// Set a timeout duration (e.g., 60s, 1h).

#[clap(long)]

window: Option<SecondsOrHumanTime>,

/// Set the number of packets before stopping.

#[clap(long)]

packets: Option<u32>,

Done in 2088490

didier-wenzek · 2025-03-05T08:53:54Z

crates/core/tedge/src/cli/mqtt/subscribe.rs


+    // the default keepalive in rumqttc is 60 sec
+    if !cmd.window.is_zero() && cmd.window.as_secs() < 60 {
+        options.set_keep_alive(cmd.window);


I'm okay with that. But be aware that the precision is 60 sec if the timeout is greater than 60s.

An option, is to set the keep alive to 5 seconds to have a more precise timeout.

rina23q · 2025-03-05T18:06:19Z

Added system tests in deb4b54. The good highlight is, now we can use tedge mqtt sub --duration 1 instead of timeout 1 in system test :)

github-actions · 2025-03-05T18:25:10Z

Robot Results

✅ Passed	❌ Failed	⏭️ Skipped	Total	Pass %	⏱️ Duration
585	0	3	585	100	1h35m26.891285s

didier-wenzek

Approved. Really convenient!

* timeout duration (--duration, -W) * number of received packets (--count, -C) Signed-off-by: Rina Fujino <rina.fujino.23@gmail.com>

Signed-off-by: Rina Fujino <rina.fujino.23@gmail.com>

Removed the MQTT keepalive mechanism for timeout introduced by thin-edge#3445. Instead, using a thread in charge of timeout. Signed-off-by: Rina Fujino <rina.fujino.23@gmail.com>

rina23q requested review from albinsuresh, didier-wenzek and jarhodes314 as code owners March 4, 2025 16:02

rina23q had a problem deploying to Test Pull Request March 4, 2025 16:02 — with GitHub Actions Failure

albinsuresh reviewed Mar 5, 2025

View reviewed changes

didier-wenzek reviewed Mar 5, 2025

View reviewed changes

rina23q had a problem deploying to Test Pull Request March 5, 2025 16:56 — with GitHub Actions Failure

rina23q requested a review from a team as a code owner March 5, 2025 18:02

rina23q temporarily deployed to Test Pull Request March 5, 2025 18:02 — with GitHub Actions Inactive

rina23q added theme:cli Theme: cli related topics theme:mqtt Theme: mqtt and mosquitto related topics labels Mar 5, 2025

rina23q temporarily deployed to Test Auto March 5, 2025 18:09 — with GitHub Actions Inactive

didier-wenzek approved these changes Mar 5, 2025

View reviewed changes

albinsuresh approved these changes Mar 6, 2025

View reviewed changes

rina23q added 2 commits March 6, 2025 10:30

feat: add CLI options to break tedge mqtt sub (timeout, packet)

c7b62c5

* timeout duration (--duration, -W) * number of received packets (--count, -C) Signed-off-by: Rina Fujino <rina.fujino.23@gmail.com>

test: add tedge mqtt sub system tests

ef7029a

Signed-off-by: Rina Fujino <rina.fujino.23@gmail.com>

rina23q force-pushed the feature/3412/add-timeout-to-tedge-mqtt-sub branch from deb4b54 to ef7029a Compare March 6, 2025 10:32

rina23q temporarily deployed to Test Pull Request March 6, 2025 10:33 — with GitHub Actions Inactive

rina23q temporarily deployed to Test Auto March 6, 2025 10:39 — with GitHub Actions Inactive

rina23q enabled auto-merge March 6, 2025 10:47

rina23q added this pull request to the merge queue Mar 6, 2025

Merged via the queue into thin-edge:main with commit 1e7027f Mar 6, 2025
33 checks passed

reubenmiller mentioned this pull request Mar 6, 2025

tedge mqtt sub panics when using a duration less than 1 second #3447

Closed

rina23q mentioned this pull request Mar 6, 2025

fix: don't panic when --duration has less than 1s value #3448

Merged

11 tasks

Conversation

rina23q commented Mar 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed changes

Types of changes

Paste Link to the issue

Checklist

Further comments

Uh oh!

codecov bot commented Mar 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

reubenmiller commented Mar 4, 2025

Uh oh!

albinsuresh left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

albinsuresh Mar 5, 2025

Choose a reason for hiding this comment

Uh oh!

didier-wenzek Mar 5, 2025

Choose a reason for hiding this comment

Uh oh!

rina23q Mar 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

didier-wenzek Mar 5, 2025

Choose a reason for hiding this comment

Uh oh!

rina23q Mar 5, 2025

Choose a reason for hiding this comment

Uh oh!

didier-wenzek Mar 5, 2025

Choose a reason for hiding this comment

Uh oh!

rina23q Mar 5, 2025

Choose a reason for hiding this comment

Uh oh!

didier-wenzek Mar 5, 2025

Choose a reason for hiding this comment

Uh oh!

rina23q commented Mar 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Mar 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Robot Results

Uh oh!

didier-wenzek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rina23q commented Mar 4, 2025 •

edited

Loading

codecov bot commented Mar 4, 2025 •

edited

Loading

albinsuresh left a comment •

edited

Loading

rina23q Mar 5, 2025 •

edited

Loading

rina23q commented Mar 5, 2025 •

edited

Loading

github-actions bot commented Mar 5, 2025 •

edited

Loading