send measurement to main/child device services #2335

PradeepKiruvale · 2023-10-11T12:37:11Z

Proposed changes

Add a feature to push measurements to the child and main device services using the new te topic.

Added tests to verify the M/E/A to child/main device services
Added missing event test to the child device
Added tests to verify the M/E/A to nested child device/services

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

codecov · 2023-10-11T12:52:07Z

Codecov Report

Merging #2335 (aee467a) into main (57ae569) will increase coverage by 0.3%.
Report is 1 commits behind head on main.
The diff coverage is 90.7%.

Additional details and impacted files

Files	Coverage Δ
crates/extensions/c8y_mapper_ext/src/serializer.rs	`81.2% <100.0%> (ø)`
crates/extensions/c8y_mapper_ext/src/converter.rs	`81.2% <92.6%> (+1.6%)`	⬆️
crates/extensions/c8y_mapper_ext/src/tests.rs	`92.0% <89.9%> (-0.6%)`	⬇️

... and 8 files with indirect coverage changes

albinsuresh · 2023-10-11T12:41:31Z

crates/extensions/c8y_mapper_ext/src/converter.rs

+        let out_second_messages = converter.convert(&in_message).await;
+        assert_eq!(out_second_messages, vec![expected_c8y_json_message]);
+    }
+


Could you add some tests for events and alarms of services as well? They were working magically already, but we never had test coverage. The existing child device tests for the same could be copied and modified.

added in tests.rs

albinsuresh · 2023-10-11T12:44:04Z

crates/extensions/c8y_mapper_ext/src/converter.rs

+        );
+        let expected_c8y_json_message = Message::new(
+            &Topic::new_unchecked("c8y/measurement/measurements/create"),
+            r#"{"externalSource":{"externalId":"test-device:device:child1:service:app1","type":"c8y_Serial"},"temp":{"temp":{"value":1.0}},"time":"2021-11-16T17:45:40.571760714+01:00","type":"m_type"}"#,


I'd strongly recommend the json! macro to wrap such complex JSONs. I'd use it even for simpler ones with more than a couple of keys. I hope the macro doesn't change the order of keys so that your assertions work seamlessly, without needing assert_json_matches!

fixed, using json! and to_string used, so don't need to use assert_json_matches.

albinsuresh · 2023-10-11T12:46:23Z

crates/extensions/c8y_mapper_ext/src/converter.rs

+
+        let expected_c8y_json_message = Message::new(
+            &Topic::new_unchecked("c8y/measurement/measurements/create"),
+            r#"{"externalSource":{"externalId":"test-device:device:main:service:appm","type":"c8y_Serial"},"temp":{"temp":{"value":1.0}},"time":"2021-11-16T17:45:40.571760714+01:00","type":"m_type"}"#,


Suggest to use json!.

albinsuresh · 2023-10-11T12:46:59Z

crates/extensions/c8y_mapper_ext/src/serializer.rs

-            // "externalSource" to tell c8Y identity API to use child-device
+        if entity.r#type == EntityType::ChildDevice || entity.r#type == EntityType::Service {
+            let entity_id = &entity.external_id;
+            // In case the measurement is addressed to a child-device or for the service use fragment


Suggested change

// In case the measurement is addressed to a child-device or for the service use fragment

// In case the measurement is addressed to a child-device or a service, use fragment

crates/extensions/c8y_mapper_ext/src/converter.rs

albinsuresh · 2023-10-11T12:53:31Z

tests/RobotFramework/tests/cumulocity/telemetry/child_device_telemetry.robot

+    Cumulocity.Device Should Exist    ${DEVICE_SN}:device:${CHILD_SN}:service:app6
+    Execute Command    tedge mqtt pub te/device/${CHILD_SN}/service/app6/e/event_002 '{"text": "test event"}'
+    Device Should Have Event/s    expected_text=test event    type=event_002    minimum=1    maximum=1
+


You may want to add similar tests for services on nested child devices as well. The "unregistered nested child service" case is not even needed as nested child devices must have explicit registration.

I will do it as part of another PR as there are unit tests are also missing.

No we should include the tests here as well so that we are sure the code supports it before we merge it.

Since that is not in the scope of this task, I do not want to bloat this PR. The nested child devices are a completely different thing, I would do it as part of another PR because this needs Unit tests and integration tests.

One question: can nested child devices also have the services? they need telemetry support as well right?

No I would disagree, it is very much in the task as a child device also reference to any child device regardless if it is an immediate child or a nested child.

And generally we should get in the habit of ensuring the code does what it is meant to before it gets merged.

One question: can nested child devices also have the services? they need telemetry support as well right?

Yes, services are supported on any type of child device (immediate or nested)

ok, thanks for the clarification. I will add the tests.

ok, thanks for the clarification. I will update the tests.

github-actions · 2023-10-11T13:57:16Z

Robot Results

✅ Passed	❌ Failed	⏭️ Skipped	Total	Pass %	⏱️ Duration
339	0	3	339	100	55m34.419s

albinsuresh

Happy to approve after a little bit of cleanup.

albinsuresh · 2023-10-18T04:39:00Z

crates/extensions/c8y_mapper_ext/src/converter.rs

+        let expected_smart_rest_message_child = Message::new(
+            &Topic::new_unchecked("c8y/s/us"),
+            "101,test-device:device:child1,child1,thin-edge.io-child",
+        );
+        let expected_smart_rest_message_service = Message::new(
+            &Topic::new_unchecked("c8y/s/us/test-device:device:child1"),
+            "102,test-device:device:child1:service:app1,systemd,app1,up",
+        );
+        let expected_c8y_json_message = Message::new(
+            &Topic::new_unchecked("c8y/measurement/measurements/create"),
+            json!({
+                "externalSource":{
+                    "externalId":"test-device:device:child1:service:app1",
+                    "type":"c8y_Serial"
+                },
+                "temp":{"temp":{"value":1.0}},
+                "time":"2021-11-16T17:45:40.571760714+01:00",
+                "type":"m_type"})
+            .to_string(),
+        );
+
+        // Test the first output messages contains SmartREST and C8Y JSON.
+        let out_first_messages: Vec<_> = converter
+            .convert(&in_message)
+            .await
+            .into_iter()
+            .filter(|m| m.topic.name.starts_with("c8y"))
+            .collect();
+        assert_eq!(
+            out_first_messages,
+            vec![
+                expected_smart_rest_message_child,
+                expected_smart_rest_message_service,
+                expected_c8y_json_message.clone()
+            ]
+        );


Suggested change

let expected_smart_rest_message_child = Message::new(

&Topic::new_unchecked("c8y/s/us"),

"101,test-device:device:child1,child1,thin-edge.io-child",

);

let expected_smart_rest_message_service = Message::new(

&Topic::new_unchecked("c8y/s/us/test-device:device:child1"),

"102,test-device:device:child1:service:app1,systemd,app1,up",

);

let expected_c8y_json_message = Message::new(

&Topic::new_unchecked("c8y/measurement/measurements/create"),

json!({

"externalSource":{

"externalId":"test-device:device:child1:service:app1",

"type":"c8y_Serial"

},

"temp":{"temp":{"value":1.0}},

"time":"2021-11-16T17:45:40.571760714+01:00",

"type":"m_type"})

.to_string(),

);

// Test the first output messages contains SmartREST and C8Y JSON.

let out_first_messages: Vec<_> = converter

.convert(&in_message)

.await

.into_iter()

.filter(|m| m.topic.name.starts_with("c8y"))

.collect();

assert_eq!(

out_first_messages,

vec![

expected_smart_rest_message_child,

expected_smart_rest_message_service,

expected_c8y_json_message.clone()

]

);

let out_first_messages: Vec<_> = converter

.convert(&in_message)

.await;

assert_messages_matching(

out_first_messages,

[

(

"c8y/s/us",

"101,test-device:device:child1,child1,thin-edge.io-child".into()

),

(

""c8y/s/us/test-device:device:child1"",

"102,test-device:device:child1:service:app1,systemd,app1,up".into()

),

(

""c8y/measurement/measurements/create"",

json!({

"externalSource":{

"externalId":"test-device:device:child1:service:app1",

"type":"c8y_Serial"

},

"temp":{"temp":{"value":1.0}},

"time":"2021-11-16T17:45:40.571760714+01:00",

"type":"m_type"

}).into()

),

]

)

You could reduce a lot a of clutter using the new test helper APIs like that. I know that this API wasn't available while you were writing these tests.

That can be done in the second phase as a followup task, as there are a lot of tests need to be ported.

albinsuresh · 2023-10-18T04:47:36Z

crates/extensions/c8y_mapper_ext/src/converter.rs

+            .convert(&in_message)
+            .await
+            .into_iter()
+            .filter(|m| m.topic.name.starts_with("c8y"))


I've seen this filter in many other tests as well. I wonder why we only care about the messages pushed to c8y and not others. Why not assert the other messages like the auto-registration messages sent to the te/ topics as well, if there is one? That's also part of the convert API contract that we're testing. Skipping them might lead to these tests not catching some unexpected side-effects in future. So, I'll advice against that filter. At least remove them from the new ones that you've added.

If not filtered all the messages will get accumulated. And the assert will fail. For this particular test, the purpose of the tests is to assert only the output messages. So, I feel filtering is OK.

the purpose of the tests is to assert only the output messages

Yes, but all the converted messages. We could skip all messages before or after the conversion, if not relevant for the test, but at least all the converted messages should have been asserted. But here, with this filter we're only asserting the messages sent to c8y, skipping any messges sent to te/ (typically auto-registration messages) or any other topics during the conversion. So, the validation is not complete if we don't assert all the converted messages.

Ok, I have updated only to the tests.

albinsuresh · 2023-10-18T04:47:52Z

crates/extensions/c8y_mapper_ext/src/converter.rs

@@ -1686,6 +1686,224 @@ mod tests {
        assert_eq!(out_second_messages, vec![expected_c8y_json_message]);
    }

+    #[tokio::test]
+    async fn convert_measurement_with_nested_child_id() {


Suggested change

async fn convert_measurement_with_nested_child_id() {

async fn convert_measurement_with_nested_child_device() {

albinsuresh · 2023-10-18T05:49:13Z

crates/extensions/c8y_mapper_ext/src/tests.rs

+    .await
+    .unwrap();
+
+    mqtt.skip(2).await;


Suggested change

mqtt.skip(2).await;

assert_received_contains_str(

&mut mqtt,

[(

"expected/topic",

"",

)],

)

I'd advise against skips like these and do loose asserts here instead, to be really sure that we're skipping what we think we're skipping. Sometimes I've encountered errors with blinds skips like this were messages were sent to tedge/errors indicating an actual issue, but we just skipped it. Since string.contains("") will always pass, we're essentially just validating the topic.

But the order of messages is fixed for any given operation. So, I feel that skipping is OK here instead of just asserting the topics. If we do not skip, the assert happens on the wrong topic and tests will fail. The assert_received_contains_str has to be enhanced to wait for the right message to assert. But this will be kind of wait forever is the worst case right?

If we do not skip, the assert happens on the wrong topic and tests will fail

This is exactly what we're trying to prevent. When you're skipping 2 messages here, you're not skipping any two messages. You're specifically skipping 2 messages that are probably not relevant for the test. But, still it is important to be sure that those 2 messages are exactly what you expected. Here, for example, you expect to skip the first 2 registration messages. What if one of them failed and sent an error message to tedge/errors instead? It'd fail in the next assertion but with some really weird errors than what you expected. So, that's why I'm suggesting a loose assertion on ("c8y/s/us", "101") topic.

ok, updated the tests

albinsuresh · 2023-10-18T05:51:24Z

crates/extensions/c8y_mapper_ext/src/tests.rs

+    .await
+    .unwrap();
+
+    mqtt.skip(2).await;


Same comment as above for all the skips in the rest of the tests.

albinsuresh · 2023-10-18T05:53:37Z

crates/extensions/c8y_mapper_ext/src/tests.rs

+    )
+    .await;
+}
+


Most of these newly added tests could have been written at the converter level itself, with far less clutter and overhead of the actors, with the help of the new assert_messages_matching test helper. Not asking you to port them now, as it's already written. More of an FYI, for future.

Sure, then there were no assert_messages_matching APIs, it was tough to assert because of the order issues in the payload. Maybe we can do it as a part of a follow-up task. There are a lot of tests that can be ported.

albinsuresh · 2023-10-18T05:55:27Z

tests/RobotFramework/tests/cumulocity/telemetry/child_device_telemetry.robot

+    Device Should Have Event/s    expected_text=test event    type=event_002    minimum=1    maximum=1
+
+# Nested child devices
+Nested child devices support sending simple measurements


Suggested change

Nested child devices support sending simple measurements

Nested child devices support sending measurement

albinsuresh · 2023-10-18T05:59:16Z

tests/RobotFramework/tests/cumulocity/telemetry/child_device_telemetry.robot

+    Cumulocity.Device Should Exist    ${CHILD_SN}
+    Cumulocity.Should Have Services    min_count=1    max_count=1    name=app1
+
+    Cumulocity.Device Should Exist    ${DEVICE_SN}:device:${CHILD_SN}:service:app1


Suggested change

Cumulocity.Device Should Exist ${DEVICE_SN}:device:${CHILD_SN}:service:app1

Cumulocity.Service Should Exist ${DEVICE_SN}:device:${CHILD_SN}:service:app1

It really should have been a new API like that, or even Cumulocity.Managed Object Should Exist (which is what the API actually does). Not under the scope of this PR though.

albinsuresh · 2023-10-20T06:27:09Z

crates/extensions/c8y_mapper_ext/src/converter.rs

+        );
+
+        // Test the first output messages contains SmartREST and C8Y JSON.
+        let out_first_messages: Vec<_> = converter.convert(&in_message).await.into_iter().collect();


Suggested change

let out_first_messages: Vec<_> = converter.convert(&in_message).await.into_iter().collect();

let out_first_messages: Vec<_> = converter.convert(&in_message).await;

Why iterate and collect when the output of convert is already a vector? The old code was doing this as they wanted to collect the filtered c8y messages. But, that's not the case anymore.

yeah, fixed

albinsuresh · 2023-10-20T06:29:41Z

crates/extensions/c8y_mapper_ext/src/converter.rs

+        // Test the second output messages doesn't contain SmartREST child device creation.
+        let out_second_messages = converter.convert(&in_message).await;
+        assert_eq!(out_second_messages, vec![expected_c8y_json_message]);


The first convert also did not have the SmartREST child creation message, as the nested child device does not support auto-registeration and had to be explicitly registered. So, this assert can be removed.

albinsuresh · 2023-10-20T06:30:37Z

crates/extensions/c8y_mapper_ext/src/converter.rs

+        );
+
+        // Test the first output messages contains SmartREST and C8Y JSON.
+        let out_first_messages: Vec<_> = converter.convert(&in_message).await.into_iter().collect();


Unnecessary iter and collect.

albinsuresh · 2023-10-20T06:32:28Z

crates/extensions/c8y_mapper_ext/src/converter.rs

+        // Test the second output messages doesn't contain SmartREST child device creation.
+        let out_second_messages = converter.convert(&in_message).await;
+        assert_eq!(out_second_messages, vec![expected_c8y_json_message]);


This assertion is also pointless for the same reason mentioned above, unless this test was relying on auto-registration for the service on nested child device. Auto-registration is not supported for nested child devices, but it is supported for services on any child device, if they follow the default topic scheme.

albinsuresh · 2023-10-20T06:33:34Z

crates/extensions/c8y_mapper_ext/src/converter.rs

+        );
+
+        // Test the first output messages contains SmartREST and C8Y JSON.
+        let out_first_messages: Vec<_> = converter.convert(&in_message).await.into_iter().collect();


Unnecessary iter and collect.

albinsuresh · 2023-10-20T06:33:47Z

crates/extensions/c8y_mapper_ext/src/converter.rs

+        );
+
+        // Test the first output messages contains SmartREST and C8Y JSON.
+        let out_first_messages: Vec<_> = converter.convert(&in_message).await.into_iter().collect();


Unnecessary iter and collect.

Signed-off-by: Pradeep Kumar K J <[email protected]>

PradeepKiruvale requested review from albinsuresh and reubenmiller October 11, 2023 12:38

PradeepKiruvale temporarily deployed to Test Pull Request October 11, 2023 12:50 — with GitHub Actions Inactive

albinsuresh reviewed Oct 11, 2023

View reviewed changes

reubenmiller mentioned this pull request Oct 12, 2023

feat: support publishing measurements to services #2333

Closed

14 tasks

PradeepKiruvale had a problem deploying to Test Pull Request October 12, 2023 13:30 — with GitHub Actions Failure

PradeepKiruvale force-pushed the service_measurement branch from 1f13817 to cf77cff Compare October 13, 2023 05:42

PradeepKiruvale had a problem deploying to Test Pull Request October 13, 2023 05:53 — with GitHub Actions Failure

PradeepKiruvale had a problem deploying to Test Pull Request October 13, 2023 07:12 — with GitHub Actions Failure

PradeepKiruvale force-pushed the service_measurement branch from 913ad9a to a80d5f5 Compare October 17, 2023 12:58

PradeepKiruvale had a problem deploying to Test Pull Request October 17, 2023 13:09 — with GitHub Actions Failure

PradeepKiruvale temporarily deployed to Test Pull Request October 17, 2023 16:48 — with GitHub Actions Inactive

albinsuresh reviewed Oct 18, 2023

View reviewed changes

PradeepKiruvale temporarily deployed to Test Pull Request October 19, 2023 01:55 — with GitHub Actions Inactive

PradeepKiruvale temporarily deployed to Test Pull Request October 20, 2023 02:01 — with GitHub Actions Inactive

albinsuresh reviewed Oct 20, 2023

View reviewed changes

PradeepKiruvale force-pushed the service_measurement branch from aea206b to 2c0fe47 Compare October 20, 2023 14:01

PradeepKiruvale temporarily deployed to Test Pull Request October 20, 2023 14:12 — with GitHub Actions Inactive

albinsuresh approved these changes Oct 20, 2023

View reviewed changes

main/child/nested-child device service measurement

aee467a

Signed-off-by: Pradeep Kumar K J <[email protected]>

PradeepKiruvale force-pushed the service_measurement branch from 2c0fe47 to aee467a Compare October 20, 2023 15:48

PradeepKiruvale temporarily deployed to Test Pull Request October 20, 2023 15:59 — with GitHub Actions Inactive

didier-wenzek merged commit d3ed45f into thin-edge:main Oct 23, 2023

reubenmiller mentioned this pull request Oct 25, 2023

Support publishing service telemetry data #1971

Closed

	// In case the measurement is addressed to a child-device or for the service use fragment
	// In case the measurement is addressed to a child-device or a service, use fragment

	async fn convert_measurement_with_nested_child_id() {
	async fn convert_measurement_with_nested_child_device() {

-    mqtt.skip(2).await;
+    assert_received_contains_str(
+        &mut mqtt,
+        [(
+            "expected/topic",
+            "",
+        )],
+    )

	Nested child devices support sending simple measurements
	Nested child devices support sending measurement

	Cumulocity.Device Should Exist ${DEVICE_SN}:device:${CHILD_SN}:service:app1
	Cumulocity.Service Should Exist ${DEVICE_SN}:device:${CHILD_SN}:service:app1

	let out_first_messages: Vec<_> = converter.convert(&in_message).await.into_iter().collect();
	let out_first_messages: Vec<_> = converter.convert(&in_message).await;

send measurement to main/child device services #2335

send measurement to main/child device services #2335

Conversation

PradeepKiruvale commented Oct 11, 2023 • edited by reubenmiller Loading

Proposed changes

Types of changes

Paste Link to the issue

Checklist

Further comments

codecov bot commented Oct 11, 2023 • edited Loading

Codecov Report

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

PradeepKiruvale Oct 13, 2023 • edited Loading

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

github-actions bot commented Oct 11, 2023 • edited Loading

Robot Results

albinsuresh 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

albinsuresh Oct 19, 2023 • edited Loading

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

PradeepKiruvale Oct 19, 2023 • edited Loading

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

albinsuresh Oct 20, 2023 • edited Loading

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

PradeepKiruvale commented Oct 11, 2023 •

edited by reubenmiller

Loading

codecov bot commented Oct 11, 2023 •

edited

Loading

PradeepKiruvale Oct 13, 2023 •

edited

Loading

github-actions bot commented Oct 11, 2023 •

edited

Loading

albinsuresh Oct 19, 2023 •

edited

Loading

PradeepKiruvale Oct 19, 2023 •

edited

Loading

albinsuresh Oct 20, 2023 •

edited

Loading