Aws topic mapping #1739

Bravo555 · 2023-02-20T16:30:17Z

Proposed changes

This PR fixes some bugs and inaccuracies concerning AWS support:

It fixes a wrong binary name used in the unit file
Maps tedge/measurements to aws/td/measurements instead of aws/td
Corrects mentions of previous AWS topic mappings used in the documentation

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

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

Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>

github-actions · 2023-02-21T08:25:21Z

Robot Results

✅ Passed	❌ Failed	⏭️ Skipped	Total	Pass %
137	0	5	137	100

Passed Tests

Name	⏱️ Duration	Suite
Define Child device 1 ID	0.005 s	`C8Y Child Alarms Rpi`
Normal case when the child device does not exist on c8y cloud	2.087 s	`C8Y Child Alarms Rpi`
Normal case when the child device already exists	0.713 s	`C8Y Child Alarms Rpi`
Reconciliation when the new alarm message arrives, restart the mapper	1.316 s	`C8Y Child Alarms Rpi`
Reconciliation when the alarm that is cleared	5.656 s	`C8Y Child Alarms Rpi`
Prerequisite Parent	17.173 s	`Child Conf Mgmt Plugin`
Prerequisite Child	0.398 s	`Child Conf Mgmt Plugin`
Child device bootstrapping	13.882 s	`Child Conf Mgmt Plugin`
Snapshot from device	19.292 s	`Child Conf Mgmt Plugin`
Child device config update	14.55 s	`Child Conf Mgmt Plugin`
Configuration types should be detected on file change (without restarting service)	56.181 s	`Inotify Crate`
Child devices support sending simple measurements	55.741 s	`Child Device Telemetry`
Child devices support sending custom measurements	40.532 s	`Child Device Telemetry`
Child devices support sending custom events	36.336 s	`Child Device Telemetry`
Child devices support sending custom events overriding the type	31.627 s	`Child Device Telemetry`
Child devices support sending custom alarms #1699	31.408 s	`Child Device Telemetry`
Child devices support sending inventory data via c8y topic	21.445 s	`Child Device Telemetry`
Main device support sending inventory data via c8y topic	23.543 s	`Child Device Telemetry`
Successful firmware operation	70.818 s	`Firmware Operation`
Install with empty firmware name	46.461 s	`Firmware Operation`
Supports restarting the device	78.955 s	`Restart Device`
Update tedge version from previous using Cumulocity	64.367 s	`Tedge Self Update`
Successful shell command with output	3.867 s	`Shell Operation`
Check Successful shell command with literal double quotes output	3.393 s	`Shell Operation`
Execute multiline shell command	2.94 s	`Shell Operation`
Failed shell command	2.955 s	`Shell Operation`
Software list should be populated during startup	59.002 s	`Software`
Install software via Cumulocity	56.597 s	`Software`
Software list should only show currently installed software and not candidates	40.709 s	`Software`
Stop tedge-agent service	0.224 s	`Log Path Config`
Customize the log path	0.1 s	`Log Path Config`
Initialize tedge-agent	0.13 s	`Log Path Config`
Check created folders	0.101 s	`Log Path Config`
Remove created custom folders	0.127 s	`Log Path Config`
Install latest via script (from current branch)	26.983 s	`Install Tedge`
Install specific version via script (from current branch)	14.131 s	`Install Tedge`
Install latest tedge via script (from main branch)	20.837 s	`Install Tedge`
Support starting and stopping services	35.473 s	`Service-Control`
Supports a reconnect	49.018 s	`Test-Commands`
Supports disconnect then connect	43.898 s	`Test-Commands`
Update unknown setting	27.263 s	`Test-Commands`
Update known setting	24.387 s	`Test-Commands`
Stop c8y-configuration-plugin	0.179 s	`Health C8Y-Configuration-Plugin`
Update the service file	0.209 s	`Health C8Y-Configuration-Plugin`
Reload systemd files	0.617 s	`Health C8Y-Configuration-Plugin`
Start c8y-configuration-plugin	0.165 s	`Health C8Y-Configuration-Plugin`
Start watchdog service	10.193 s	`Health C8Y-Configuration-Plugin`
Check PID of c8y-configuration-plugin	0.173 s	`Health C8Y-Configuration-Plugin`
Kill the PID	0.131 s	`Health C8Y-Configuration-Plugin`
Recheck PID of c8y-configuration-plugin	0.245 s	`Health C8Y-Configuration-Plugin`
Compare PID change	0.003 s	`Health C8Y-Configuration-Plugin`
Stop watchdog service	0.199 s	`Health C8Y-Configuration-Plugin`
Remove entry from service file	0.181 s	`Health C8Y-Configuration-Plugin`
Stop c8y-log-plugin	0.189 s	`Health C8Y-Log-Plugin`
Update the service file	0.13 s	`Health C8Y-Log-Plugin`
Reload systemd files	0.557 s	`Health C8Y-Log-Plugin`
Start c8y-log-plugin	0.167 s	`Health C8Y-Log-Plugin`
Start watchdog service	10.216 s	`Health C8Y-Log-Plugin`
Check PID of c8y-log-plugin	0.264 s	`Health C8Y-Log-Plugin`
Kill the PID	0.308 s	`Health C8Y-Log-Plugin`
Recheck PID of c8y-log-plugin	0.246 s	`Health C8Y-Log-Plugin`
Compare PID change	0.001 s	`Health C8Y-Log-Plugin`
Stop watchdog service	0.304 s	`Health C8Y-Log-Plugin`
Remove entry from service file	0.246 s	`Health C8Y-Log-Plugin`
Stop tedge-mapper	0.316 s	`Health Tedge Mapper C8Y`
Update the service file	0.248 s	`Health Tedge Mapper C8Y`
Reload systemd files	1.1219999999999999 s	`Health Tedge Mapper C8Y`
Start tedge-mapper	0.442 s	`Health Tedge Mapper C8Y`
Start watchdog service	10.328 s	`Health Tedge Mapper C8Y`
Check PID of tedge-mapper	0.08 s	`Health Tedge Mapper C8Y`
Kill the PID	0.109 s	`Health Tedge Mapper C8Y`
Recheck PID of tedge-mapper	2.202 s	`Health Tedge Mapper C8Y`
Compare PID change	0.001 s	`Health Tedge Mapper C8Y`
Stop watchdog service	0.191 s	`Health Tedge Mapper C8Y`
Remove entry from service file	0.158 s	`Health Tedge Mapper C8Y`
Stop tedge-agent	0.25 s	`Health Tedge-Agent`
Update the service file	0.107 s	`Health Tedge-Agent`
Reload systemd files	0.602 s	`Health Tedge-Agent`
Start tedge-agent	0.122 s	`Health Tedge-Agent`
Start watchdog service	10.155 s	`Health Tedge-Agent`
Check PID of tedge-mapper	0.09 s	`Health Tedge-Agent`
Kill the PID	0.046 s	`Health Tedge-Agent`
Recheck PID of tedge-agent	2.1189999999999998 s	`Health Tedge-Agent`
Compare PID change	0.001 s	`Health Tedge-Agent`
Stop watchdog service	0.068 s	`Health Tedge-Agent`
Remove entry from service file	0.123 s	`Health Tedge-Agent`
Stop tedge-mapper-az	0.149 s	`Health Tedge-Mapper-Az`
Update the service file	0.168 s	`Health Tedge-Mapper-Az`
Reload systemd files	0.409 s	`Health Tedge-Mapper-Az`
Start tedge-mapper-az	0.131 s	`Health Tedge-Mapper-Az`
Start watchdog service	10.109 s	`Health Tedge-Mapper-Az`
Check PID of tedge-mapper-az	0.174 s	`Health Tedge-Mapper-Az`
Kill the PID	0.327 s	`Health Tedge-Mapper-Az`
Recheck PID of tedge-agent	2.448 s	`Health Tedge-Mapper-Az`
Compare PID change	0.001 s	`Health Tedge-Mapper-Az`
Stop watchdog service	0.156 s	`Health Tedge-Mapper-Az`
Remove entry from service file	0.144 s	`Health Tedge-Mapper-Az`
Stop tedge-mapper-collectd	0.153 s	`Health Tedge-Mapper-Collectd`
Update the service file	0.162 s	`Health Tedge-Mapper-Collectd`
Reload systemd files	0.575 s	`Health Tedge-Mapper-Collectd`
Start tedge-mapper-collectd	0.214 s	`Health Tedge-Mapper-Collectd`
Start watchdog service	10.137 s	`Health Tedge-Mapper-Collectd`
Check PID of tedge-mapper-collectd	0.068 s	`Health Tedge-Mapper-Collectd`
Kill the PID	0.106 s	`Health Tedge-Mapper-Collectd`
Recheck PID of tedge-mapper-collectd	2.292 s	`Health Tedge-Mapper-Collectd`
Compare PID change	0 s	`Health Tedge-Mapper-Collectd`
Stop watchdog service	0.156 s	`Health Tedge-Mapper-Collectd`
Remove entry from service file	0.08 s	`Health Tedge-Mapper-Collectd`
c8y-log-plugin health status	5.886 s	`MQTT health endpoints`
c8y-configuration-plugin health status	5.6129999999999995 s	`MQTT health endpoints`
Wrong package name	0.24 s	`Improve Tedge Apt Plugin Error Messages`
Wrong version	0.269 s	`Improve Tedge Apt Plugin Error Messages`
Wrong type	0.625 s	`Improve Tedge Apt Plugin Error Messages`
tedge_connect_test_positive	0.7 s	`Tedge Connect Test`
tedge_connect_test_negative	1.501 s	`Tedge Connect Test`
tedge_connect_test_sm_services	7.287 s	`Tedge Connect Test`
tedge_disconnect_test_sm_services	0.653 s	`Tedge Connect Test`
Install thin-edge.io	6.083 s	`Call Tedge`
call tedge -V	0.058 s	`Call Tedge`
call tedge -h	0.142 s	`Call Tedge`
call tedge -h -V	0.094 s	`Call Tedge`
call tedge help	0.116 s	`Call Tedge`
tedge config list	0.102 s	`Call Tedge Config List`
tedge config list --all	0.079 s	`Call Tedge Config List`
set/unset device.type	0.401 s	`Call Tedge Config List`
set/unset device.key.path	0.515 s	`Call Tedge Config List`
set/unset device.cert.path	0.531 s	`Call Tedge Config List`
set/unset c8y.root.cert.path	0.713 s	`Call Tedge Config List`
set/unset c8y.smartrest.templates	0.58 s	`Call Tedge Config List`
set/unset az.root.cert.path	0.447 s	`Call Tedge Config List`
set/unset az.mapper.timestamp	0.343 s	`Call Tedge Config List`
set/unset mqtt.bind_address	0.329 s	`Call Tedge Config List`
set/unset mqtt.port	0.37 s	`Call Tedge Config List`
set/unset tmp.path	0.342 s	`Call Tedge Config List`
set/unset logs.path	0.358 s	`Call Tedge Config List`
set/unset run.path	0.53 s	`Call Tedge Config List`
Get Put Delete	3.747 s	`Http File Transfer Api`

PradeepKiruvale · 2023-02-21T15:25:27Z

docs/src/architecture/mapper.md

@@ -160,7 +160,7 @@ sudo systemctl restart tedge-mapper-az.service

 The AWS mapper takes messages formatted in the [Thin Edge JSON](thin-edge-json.md) as input.
 It validates if the incoming message is correctly formatted Thin Edge JSON, then outputs the message.
-The validated messages are published on the topic `aws/messages/` from where they are forwarded to AWS.
+The validated messages are published on the topic `aws/td/` from where they are forwarded to AWS.


is aws/td/ a valid topic?, Because there is a forward slash at the end.

Right, that should probably say aws/td/#.

docs/src/references/bridged-topics.md

didier-wenzek

Approved

docs/src/references/bridged-topics.md

Bravo555 · 2023-02-24T10:37:21Z

Added tedge/alarms and tedge/events as the input topics to the AWS mapper, but currently I'm not enforcing the same schema as the cumulocity mapper does, and the mapper only adds the timestamp for any topic with tedge/alarms or tedge/events prefix. Should it remain like that or should I enforce the tedge/alarms/<severity>/<type> schema and similar for events as described in the data model document?

didier-wenzek · 2023-02-24T13:56:45Z

Added tedge/alarms and tedge/events as the input topics to the AWS mapper, but currently I'm not enforcing the same schema as the cumulocity mapper does, and the mapper only adds the timestamp for any topic with tedge/alarms or tedge/events prefix. Should it remain like that or should I enforce the tedge/alarms/<severity>/<type> schema and similar for events as described in the data model document?

The mapping has to be specific to each cloud adopting idioms and good practices for that specific cloud end-point. So that's okay. @reubenmiller what's your point of view here?

didier-wenzek

You need to add an explicit filter to uniquely match measurements, alarms and events.

crates/core/tedge_mapper/src/aws/converter.rs

Bravo555 · 2023-02-27T15:28:46Z

Also added tedge/health and tedge/health/+ as input topics so that health info gets bridged to the remote broker. I think it's ready for the final review.

didier-wenzek

Approved - unfortunately this cannot be merged till fixed the (independent) integration test issues.

Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>

PradeepKiruvale · 2023-02-28T02:09:43Z

crates/core/tedge_mapper/src/aws/converter.rs

+            "tedge/alarms/+/+/+",
+        ]
+        .try_into()
+        .unwrap()


Replacing unwrap with error handling is better.

In this specific case, unwrap is okay as all the topics are correct and known at compile time.

PradeepKiruvale · 2023-02-28T02:11:52Z

crates/core/tedge_mapper/src/aws/converter.rs

@@ -44,13 +58,44 @@ impl Converter for AwsConverter {
    }

    async fn try_convert(&mut self, input: &Message) -> Result<Vec<Message>, Self::Error> {
-        let () = self.size_threshold.validate(input)?;
+        self.size_threshold.validate(input)?;


Did not see a threshold value for the aws defined here. I feel the validation here is against the aws supported size.

Moved size validation to the end, so that the message is validated after it's transformed by the mapper.

PradeepKiruvale · 2023-02-28T02:15:17Z

crates/core/tedge_mapper/src/aws/converter.rs

+        };
+
+        let mut topic_iter = input.topic.name.split('/');
+        let td_type = topic_iter.nth(1).unwrap();


Better handle this unwrap

crates/core/tedge_mapper/src/aws/converter.rs

PradeepKiruvale · 2023-02-28T02:21:37Z

crates/core/tedge_mapper/src/aws/converter.rs

+
+        let mut topic_iter = input.topic.name.split('/');
+        let td_type = topic_iter.nth(1).unwrap();
+        let mut topic_suffix = topic_iter.collect::<Vec<_>>().join("/");


I feel suffix / is not needed here. Because the topic must be like aws/td/measurement right?

The intent was to handle things like tedge/alarms/<severity>/<alarm-type>/<child-id>, etc. Then td_type would be alarms, and topic_suffix would be /<severity>/<alarm-type>/<child-id>, but you are correct that it seems to be needlessly complicated, I'm not sure why I did it like that, as just splitting on the first / character and taking the string to the right would work. Will fix.

I now realize that by "suffix /" you meant that .join("/") at the end, which does not actually add text to the end of the string, but is actually called on a slice, to concatenate the elements with some string in between. So there is no / suffix there, but I've realized now that there is a better way to write it, so I'll change it

@PradeepKiruvale should be fixed now

But if you do split_once on tedge/alarms/<severity>/<alarm-type>/<child-id> then td_type will be tedge and topic_suffix will be alarms/<severity>/<alarm-type>/<child-id>. This does not address what you mentioned in your comment right?

But if you do split_once on tedge/alarms/<severity>/<alarm-type>/<child-id> then td_type will be tedge and topic_suffix will be alarms/<severity>/<alarm-type>/<child-id>.

I meant that it was needlessly complicated because it was splitting topic into td_type and topic_sufix, and then concatenating them anyway. So now, split_once returns ("tedge", "alarms/<severity>/<alarm-type>/<child-id>") and i just get the 2nd element of that tuple and use it to craft the final topic.

didier-wenzek · 2023-02-28T08:42:17Z

This PR has to be rebased to get the fix for test report generation.

Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>

Topics `tedge/alarms` and `tedge/events` were added to the input topic filter of the AWS mapper. These additional topics are currently not using Thin-Edge JSON, but are only parsed as regular JSON, and the mapper only adds the timestamp to the payload. Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>

Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>

didier-wenzek · 2023-02-28T16:24:16Z

A test is failing: aws::converter::tests::exceeding_threshold_returns_error' panicked at 'assertion failed

Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>

Fix wrong binary name in tedge-mapper-aws service

debe4b5

Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>

Bravo555 temporarily deployed to Test Pull Request February 20, 2023 16:36 — with GitHub Actions Inactive

Bravo555 marked this pull request as draft February 21, 2023 10:02

PradeepKiruvale reviewed Feb 21, 2023

View reviewed changes

Bravo555 force-pushed the aws-topic-mapping branch 2 times, most recently from dfb7661 to ed2c6b3 Compare February 21, 2023 16:10

Bravo555 temporarily deployed to Test Pull Request February 21, 2023 16:17 — with GitHub Actions Inactive

Bravo555 marked this pull request as ready for review February 21, 2023 16:43

Bravo555 requested a review from PradeepKiruvale February 23, 2023 12:44

PradeepKiruvale reviewed Feb 23, 2023

View reviewed changes

docs/src/references/bridged-topics.md Show resolved Hide resolved

didier-wenzek approved these changes Feb 24, 2023

View reviewed changes

docs/src/references/bridged-topics.md Show resolved Hide resolved

Bravo555 had a problem deploying to Test Pull Request February 24, 2023 10:36 — with GitHub Actions Failure

Bravo555 closed this Feb 24, 2023

Bravo555 reopened this Feb 24, 2023

Bravo555 temporarily deployed to Test Pull Request February 24, 2023 10:43 — with GitHub Actions Inactive

didier-wenzek requested changes Feb 24, 2023

View reviewed changes

crates/core/tedge_mapper/src/aws/converter.rs Outdated Show resolved Hide resolved

Bravo555 had a problem deploying to Test Pull Request February 27, 2023 14:39 — with GitHub Actions Failure

Bravo555 had a problem deploying to Test Pull Request February 27, 2023 15:35 — with GitHub Actions Failure

Bravo555 requested review from didier-wenzek and PradeepKiruvale and removed request for didier-wenzek and PradeepKiruvale February 27, 2023 15:42

didier-wenzek approved these changes Feb 27, 2023

View reviewed changes

aws-mapper: reroute tedge/measurements to aws/td/measurements

2986c08

Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>

Bravo555 force-pushed the aws-topic-mapping branch from 8d30603 to 5b83645 Compare February 27, 2023 17:37

Bravo555 temporarily deployed to Test Pull Request February 27, 2023 17:44 — with GitHub Actions Inactive

PradeepKiruvale reviewed Feb 28, 2023

View reviewed changes

Bravo555 force-pushed the aws-topic-mapping branch from 8ffb4c5 to 5b83645 Compare February 28, 2023 10:27

Bravo555 had a problem deploying to Test Pull Request February 28, 2023 10:34 — with GitHub Actions Failure

Bravo555 temporarily deployed to Test Pull Request February 28, 2023 11:49 — with GitHub Actions Inactive

Bravo555 added 3 commits February 28, 2023 15:52

Fix AWS topics used in documentation

c53441b

Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>

Add tedge/health to input topics

2884b90

Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>

Bravo555 force-pushed the aws-topic-mapping branch from 47e4249 to 6cf9791 Compare February 28, 2023 14:52

Bravo555 had a problem deploying to Test Pull Request February 28, 2023 15:00 — with GitHub Actions Failure

Use correct AWS payload size threshold

13592c1

Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>

Bravo555 force-pushed the aws-topic-mapping branch from 6cf9791 to 13592c1 Compare February 28, 2023 17:09

Bravo555 temporarily deployed to Test Pull Request February 28, 2023 17:15 — with GitHub Actions Inactive

didier-wenzek merged commit ebd31f0 into thin-edge:main Feb 28, 2023

Bravo555 deleted the aws-topic-mapping branch February 28, 2023 17:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Aws topic mapping #1739

Aws topic mapping #1739

Bravo555 commented Feb 20, 2023

github-actions bot commented Feb 21, 2023 •

edited

PradeepKiruvale Feb 21, 2023

Bravo555 Feb 21, 2023

didier-wenzek left a comment

Bravo555 commented Feb 24, 2023 •

edited

didier-wenzek commented Feb 24, 2023

didier-wenzek left a comment

Bravo555 commented Feb 27, 2023

didier-wenzek left a comment

PradeepKiruvale Feb 28, 2023

didier-wenzek Feb 28, 2023

PradeepKiruvale Feb 28, 2023

Bravo555 Feb 28, 2023

PradeepKiruvale Feb 28, 2023

Bravo555 Feb 28, 2023

PradeepKiruvale Feb 28, 2023

Bravo555 Feb 28, 2023

Bravo555 Feb 28, 2023

Bravo555 Feb 28, 2023

PradeepKiruvale Feb 28, 2023

Bravo555 Feb 28, 2023

didier-wenzek commented Feb 28, 2023

didier-wenzek commented Feb 28, 2023

Aws topic mapping #1739

Aws topic mapping #1739

Conversation

Bravo555 commented Feb 20, 2023

Proposed changes

Types of changes

Paste Link to the issue

Checklist

Further comments

github-actions bot commented Feb 21, 2023 • edited

Robot Results

Passed Tests

Choose a reason for hiding this comment

Choose a reason for hiding this comment

didier-wenzek left a comment

Choose a reason for hiding this comment

Bravo555 commented Feb 24, 2023 • edited

didier-wenzek commented Feb 24, 2023

didier-wenzek left a comment

Choose a reason for hiding this comment

Bravo555 commented Feb 27, 2023

didier-wenzek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

didier-wenzek commented Feb 28, 2023

didier-wenzek commented Feb 28, 2023

github-actions bot commented Feb 21, 2023 •

edited

Bravo555 commented Feb 24, 2023 •

edited