fix: Collect multiple smartrest messages from c8y devicecontrol topic #3301

Bravo555 · 2024-12-16T16:15:43Z

Proposed changes

c8y/devicecontrol/notifications topic, as many others, can contain in its smartrest payload multiple records. For now ensure that multiple records are parsed from this topic in particular, as C8y sends multiple operations in a single message if e.g. device is offline, multiple operations are started, and then the device connects, it then receives all those operations in a single MQTT message.

But a longer term solution would be to refactor the code to ensure that we're supporting collecting multiple smartrest messages from all the possible MQTT topics.

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

#3297

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

rina23q

Some renaming required.

crates/extensions/c8y_mapper_ext/src/converter.rs

codecov · 2024-12-16T16:33:13Z

Codecov Report

Attention: Patch coverage is 94.28571% with 6 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
crates/extensions/c8y_mapper_ext/src/converter.rs	80.00%	2 Missing and 3 partials ⚠️
...rates/extensions/c8y_firmware_manager/src/actor.rs	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

📢 Thoughts on this report? Let us know!

…icecontrol topic c8y/devicecontrol/notifications topic, as many others, can contain in its smartrest payload multiple records. For now ensure that multiple records are parsed from this topic in particular, as C8y sends multiple operations in a single message if e.g. device is offline, multiple operations are started, and then the device connects, it then receives all those operations in a single MQTT message. But a longer term solution would be to refactor the code to ensure that we're supporting collecting multiple smartrest messages from all the possible MQTT topics. Signed-off-by: Marcel Guzik <[email protected]>

rina23q

LGTM

rina23q · 2024-12-16T16:38:47Z

crates/extensions/c8y_mapper_ext/src/converter.rs

+                .await;
+            let result = self.handle_c8y_operation_result(&result, Some(operation.op_id));
+            output.extend(result);
+        }

        Ok(output)


My opinion for the future development.

What I don't like from the current way (due to the converter design) is, this return value Ok(output) is a vector of MQTT messages that will be published later at the same time. If 5 operations are included in one message from c8y, 5 of 504 messages will be sent together. But it's definitely out of scope to fix now.

What I don't like from the current way (due to the converter design) is, this return value Ok(output) is a vector of MQTT messages that will be published later at the same time. If 5 operations are included in one message from c8y, 5 of 504 messages will be sent together.

What don't you like in the current design? That the value returned by this method is a vector of MQTT messages or that the five 504 messages are not packed into a single MQTT message? The later can be implemented using the former method signature.

504 changes an operation state to executing in c8y.

Let's say there are 3 operation (Op_A, Op_B, Op_C) are delivered at the same.
Ideally,

Op_A start proceeding -> Publish 504 for Op_A

Op_B start proceeding -> Publish 504 for Op_B

Op_C start proceeding -> Publish 504 for Op_C

However, with the current design, when all of Op_A, Op_B, and Op_C start proceeding, mapper publishes three 504 messages for these 3 operations.

In short, what I don't like is the timing the 504 message is published by mapper. Not critical, but minor point.

Though this doesn't occur for operations being processed by a workflow right (as the workflow execution is controlled by the tedge-agent, and currently only processes one type of operation at a time of the same type).

github-actions · 2024-12-16T16:44:09Z

Robot Results

✅ Passed	❌ Failed	⏭️ Skipped	Total	Pass %	⏱️ Duration
549	0	2	549	100	1h23m5.202436999s

didier-wenzek

Approved

As this function can be used for JSON over MQTT as well, updating the name. Signed-off-by: Marcel Guzik <[email protected]>

Bravo555 added this to the 1.4.0 milestone Dec 16, 2024

Bravo555 requested review from didier-wenzek, albinsuresh, jarhodes314 and rina23q as code owners December 16, 2024 16:15

Bravo555 had a problem deploying to Test Pull Request December 16, 2024 16:15 — with GitHub Actions Error

Bravo555 force-pushed the fix/3297/c8y-devicecontrol-split-on-newlines branch from 1ef4041 to 5ad5345 Compare December 16, 2024 16:18

Bravo555 temporarily deployed to Test Pull Request December 16, 2024 16:18 — with GitHub Actions Inactive

Bravo555 requested a review from reubenmiller December 16, 2024 16:22

rina23q reviewed Dec 16, 2024

View reviewed changes

crates/extensions/c8y_mapper_ext/src/converter.rs Outdated Show resolved Hide resolved

crates/extensions/c8y_mapper_ext/src/converter.rs Outdated Show resolved Hide resolved

Bravo555 temporarily deployed to Test Auto December 16, 2024 16:30 — with GitHub Actions Inactive

Bravo555 force-pushed the fix/3297/c8y-devicecontrol-split-on-newlines branch from 5ad5345 to 1592ea6 Compare December 16, 2024 16:35

Bravo555 temporarily deployed to Test Pull Request December 16, 2024 16:35 — with GitHub Actions Inactive

rina23q approved these changes Dec 16, 2024

View reviewed changes

Bravo555 temporarily deployed to Test Auto December 16, 2024 16:42 — with GitHub Actions Inactive

didier-wenzek approved these changes Dec 16, 2024

View reviewed changes

Bravo555 force-pushed the fix/3297/c8y-devicecontrol-split-on-newlines branch from 1592ea6 to 0df75f0 Compare December 16, 2024 17:11

Bravo555 temporarily deployed to Test Pull Request December 16, 2024 17:11 — with GitHub Actions Inactive

Bravo555 temporarily deployed to Test Auto December 16, 2024 17:19 — with GitHub Actions Inactive

Rename collect_smartrest_messages to collect_c8y_messages

6748f0e

As this function can be used for JSON over MQTT as well, updating the name. Signed-off-by: Marcel Guzik <[email protected]>

Bravo555 force-pushed the fix/3297/c8y-devicecontrol-split-on-newlines branch from 0df75f0 to 6748f0e Compare December 16, 2024 17:50

Bravo555 temporarily deployed to Test Pull Request December 16, 2024 17:50 — with GitHub Actions Inactive

Bravo555 had a problem deploying to Test Auto December 16, 2024 17:57 — with GitHub Actions Failure

Bravo555 temporarily deployed to Test Auto December 16, 2024 19:56 — with GitHub Actions Inactive

reubenmiller changed the title ~~fix(#3297): Collect multiple smartrest messages from c8y devicecontro…~~ fix(#3297): Collect multiple smartrest messages from c8y devicecontrol topic Dec 16, 2024

reubenmiller changed the title ~~fix(#3297): Collect multiple smartrest messages from c8y devicecontrol topic~~ fix: Collect multiple smartrest messages from c8y devicecontrol topic Dec 16, 2024

reubenmiller added the theme:c8y Theme: Cumulocity related topics label Dec 16, 2024

reubenmiller added this pull request to the merge queue Dec 16, 2024

Merged via the queue into thin-edge:main with commit db7b497 Dec 16, 2024
33 checks passed

Bravo555 mentioned this pull request Dec 16, 2024

fix: split c8y JSON over MQTT messages using newline delimiter #3302

Merged

11 tasks

reubenmiller added the skip-release-notes label Dec 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Collect multiple smartrest messages from c8y devicecontrol topic #3301

fix: Collect multiple smartrest messages from c8y devicecontrol topic #3301

Bravo555 commented Dec 16, 2024

rina23q left a comment

codecov bot commented Dec 16, 2024 •

edited

Loading

rina23q left a comment

rina23q Dec 16, 2024

didier-wenzek Dec 16, 2024

rina23q Dec 16, 2024 •

edited

Loading

reubenmiller Dec 16, 2024

github-actions bot commented Dec 16, 2024 •

edited

Loading

didier-wenzek left a comment

fix: Collect multiple smartrest messages from c8y devicecontrol topic #3301

fix: Collect multiple smartrest messages from c8y devicecontrol topic #3301

Conversation

Bravo555 commented Dec 16, 2024

Proposed changes

Types of changes

Paste Link to the issue

Checklist

Further comments

rina23q left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 16, 2024 • edited Loading

Codecov Report

rina23q left a comment

Choose a reason for hiding this comment

rina23q Dec 16, 2024

Choose a reason for hiding this comment

didier-wenzek Dec 16, 2024

Choose a reason for hiding this comment

rina23q Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

reubenmiller Dec 16, 2024

Choose a reason for hiding this comment

github-actions bot commented Dec 16, 2024 • edited Loading

Robot Results

didier-wenzek left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 16, 2024 •

edited

Loading

rina23q Dec 16, 2024 •

edited

Loading

github-actions bot commented Dec 16, 2024 •

edited

Loading