Update c8y-mapper to consume `te` measurements #2169

didier-wenzek · 2023-08-18T11:42:29Z

The main point of this PR is to implement a concrete use-case of the EntityStore and be in a better position to evaluate and improve this new API.

Proposed changes

Update the c8y-mapper to translate the measurements received on the te/+/+/+/+/m/+ topics.

Move the EntityStore from the mapper to its converter
Subscribe to te/+/+/+/+/m/+ and no more on tedge/measurements/#
Combine local device registration with cloud registration
~~Fix the behavior of device registration when the message triggering the registration is ill-formed~~
List suggestions of improvement for EntityStore: Improve EntityStore API #2179

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

#2082

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

didier-wenzek · 2023-08-18T11:48:59Z

crates/core/tedge_api/src/entity.rs

@@ -61,6 +61,16 @@ impl EntityTopic {
            _ => None,
        }
    }
+
+    pub fn is_measurement(topic: &mqtt_channel::Topic) -> bool {


I'm not happy with this function.

It's signature is too specific. It would be better to get the channel kind of the topic.

It's implementation seems a bit complex compared to the task. The current channel type is not appropriate.

didier-wenzek · 2023-08-18T11:51:07Z

crates/core/tedge_api/src/entity.rs

-        if r#type.is_empty() {
-            return Err(ChannelError::TooShort);
-        }


This must be removed.

te/device/main///m/ is a valid topic name which channel is of measurements with an empty type.

didier-wenzek · 2023-08-18T11:52:45Z

crates/core/tedge_api/src/entity_store.rs

@@ -97,13 +98,27 @@ impl EntityStore {
        self.get(mqtt_id)
    }

+    /// Returns the entity attached to a topic, if any
+    pub fn get_entity_from_topic(&self, topic: &Topic) -> Option<&EntityMetadata> {


The starting point for a caller is always a topic.

didier-wenzek · 2023-08-18T11:54:29Z

crates/core/tedge_api/src/entity_store.rs

    /// Returns the MQTT identifier of the main device.
    ///
    /// The main device is an entity with `@type: "device"`.
    pub fn main_device(&self) -> EntityIdRef {
        self.main_device.as_str()
    }

+    /// Returns the name of main device.
+    pub fn main_device_name(&self) -> &str {


The method that returns the main device identifier is a bit useless for a user of the EntityStore, because returning an identifier and not an EntityMetadata.

The intent behind that was for the method to return the key that you can use in get:

entity_store.get(entity_store.main_device())

But probably 99% this is what we want, so main_device could be changed to just return EntityMetadata. One problem is that EntityMetadata doesn't contain entity MQTT identifier, but that can perhaps be added later in a way that doesn't just duplicate the string

But probably 99% this is what we want, so main_device could be changed to just return EntityMetadata.

Yes, that would be better. Especially, because entity_store.get(entity_store.main_device()) returns an Option despite there is always some main device.

One problem is that EntityMetadata doesn't contain entity MQTT identifier, but that can perhaps be added later in a way that doesn't just duplicate the string

Yes, this will also be really convenient.

didier-wenzek · 2023-08-18T11:59:12Z

crates/extensions/c8y_mapper_ext/src/actor.rs

        Self {
            converter,
            messages,
            mqtt_publisher,
            timer_sender,
-            entity_store: EntityStore::with_main_device(main_device).unwrap(),
        }
    }

    async fn process_mqtt_message(&mut self, message: MqttMessage) -> Result<(), RuntimeError> {


This method has been moved into the Converter.

This will have to be updated after #2164

didier-wenzek · 2023-08-18T12:02:37Z

crates/extensions/c8y_mapper_ext/src/json.rs

+pub fn from_thin_edge_json(
    input: &str,
-    child_id: &str,
+    entity: &EntityMetadata,
 ) -> Result<String, CumulocityJsonError> {


The changes in this file highlights how the new EntityStore API can simplify and make more robust the mappers.

didier-wenzek · 2023-08-18T12:04:13Z

crates/extensions/c8y_mapper_ext/src/tests.rs

-        "",
+        r#"{"foo":25}"#,


A proper measurements is required otherwise the registration message is rejected by the converter.

didier-wenzek · 2023-08-18T12:07:27Z

crates/extensions/c8y_mapper_ext/src/converter.rs

+            let device_register_payload =
+                format!("{{ \"@type\":\"{device_type}\", \"@id\":\"{device_name}\"}}");


Compared to the original version https://github.com/thin-edge/thin-edge.io/pull/2169/files#diff-1d1eb076015e0658044f7b03b0b805fce4f0253f4048587797fc5dd42019f908L149

the device type is now correct for the main device.

the device name is compatible with the current version of thin-edge (to avoid breaking the tests)

didier-wenzek · 2023-08-18T12:13:39Z

crates/extensions/c8y_mapper_ext/src/converter.rs

-                add_external_device_registration_message(
-                    child_id,
-                    &mut self.children,
-                    &mut mqtt_messages,
-                );


This has been moved to the try_convert() method https://github.com/thin-edge/thin-edge.io/pull/2169/files#diff-1d1eb076015e0658044f7b03b0b805fce4f0253f4048587797fc5dd42019f908L149.

Pros: The registration can then be done for all kind of messages (measurements/alarms/events)

Cons: Now an ill-formed message trigger the auto-registration while it should not.

Because of that the test convert_first_measurement_invalid_then_valid_with_child_id must be ignored https://github.com/thin-edge/thin-edge.io/pull/2169/files#diff-08a20a8877f5e1e1bce1aac2c6fc0e1a63a36cc9311e7e60cb3aacacf3c8c57cR1361

didier-wenzek · 2023-08-18T12:17:36Z

crates/extensions/c8y_mapper_ext/src/converter.rs

    fn try_convert_measurement(
        &mut self,
        input: &Message,
    ) -> Result<Vec<Message>, ConversionError> {


My goal is to reach a point where this method is no more processing a raw MQTT message, but a Measurement message that has been fully decoded by the tedge-api and gives:

the topic identifier of the source

the measurement type

the measurement value

github-actions · 2023-08-18T12:33:06Z

Robot Results

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

Bravo555 · 2023-08-21T11:10:56Z

crates/extensions/c8y_mapper_ext/src/converter.rs

+        }
+
+        let mut messages = match &message.topic {
+            topic if EntityTopic::is_measurement(topic) => self.try_convert_measurement(message),


Could also be done like this without adding a new method. It's a mouthful, but to be honest we're inside a match arm and we have to do all of this in a single expression, usually we would have different statements for creating the EntityTopic and looking at the type of channel it has.

Suggested change

topic if EntityTopic::is_measurement(topic) => self.try_convert_measurement(message),

topic if EntityTopic::from_str(&topic.name).map_or(false, |t| {

t.channel().map(|c| c.category) == Some(ChannelCategory::Measurement)

}) =>

{

self.try_convert_measurement(message)

}

Here, I would prefer to have a fully different match expression.

Instead of having methods working on raw topics and messages, my goal is to do all the parsing inside the tedge-api crate with some function taking an MqttMessage and returning an error or a plain struct with the message source (e.g. device/main//) the channel (e.g. Measurement { r#type }) and the decoded message (e.g the measurement). The converter main method would be then a breeze focused on the translation into the format expected by the cloud and no more a mix of parsing / validation / translation.

crates/core/tedge_api/src/entity_store.rs

Bravo555 · 2023-08-21T11:24:04Z

crates/core/tedge_api/src/entity_store.rs

+    pub fn main_device(external_id: String) -> Self {
+        Self {
+            external_id,
+            r#type: EntityType::MainDevice,
+            parent: None,
+            other: serde_json::json!({}),
+        }
+    }
+
+    /// Creates a entity metadata for a child device.
+    pub fn child_device(external_id: String) -> Self {
+        Self {
+            external_id,
+            r#type: EntityType::ChildDevice,
+            parent: Some("device/main//".to_string()),
+            other: serde_json::json!({}),
+        }
+    }
+}
+


I see this functions are only used in tests. Would you say that we'll need to construct EntityMetadata in normal code as well? My intent was for only EntityStore to be able to construct it, when parsing a registration message, but this can change.

I see this functions are only used in tests

Indeed, currently only used for testing. Mostly because creating an EntityRegistrationMessage where a bit more painful.

Would you say that we'll need to construct EntityMetadata in normal code as well? My intent was for only EntityStore to be able to construct it, when parsing a registration message, but this can change.

Not sure to see the benefits of EntityRegistrationMessage.

Bravo555 · 2023-08-21T11:29:24Z

crates/core/tedge_api/src/entity_store.rs

    /// Returns the MQTT identifier of the main device.
    ///
    /// The main device is an entity with `@type: "device"`.
    pub fn main_device(&self) -> EntityIdRef {
        self.main_device.as_str()
    }

+    /// Returns the name of main device.
+    pub fn main_device_name(&self) -> &str {


The intent behind that was for the method to return the key that you can use in get:

entity_store.get(entity_store.main_device())

But probably 99% this is what we want, so main_device could be changed to just return EntityMetadata. One problem is that EntityMetadata doesn't contain entity MQTT identifier, but that can perhaps be added later in a way that doesn't just duplicate the string

Bravo555 · 2023-08-21T11:30:29Z

crates/core/tedge_mapper/src/collectd/mapper.rs

@@ -12,7 +12,7 @@ use tedge_config::TEdgeConfig;

 const COLLECTD_MAPPER_NAME: &str = "tedge-mapper-collectd";
 const COLLECTD_INPUT_TOPICS: &str = "collectd/#";
-const COLLECTD_OUTPUT_TOPIC: &str = "tedge/measurements";
+const COLLECTD_OUTPUT_TOPIC: &str = "te/device/main///m/ ";


Hardcoded MQTT root

Again, it would be good to get this topic from the entity store.

entity_store.main_device().measurement_topic()

This will be done in a follow-up task: #2179

Bravo555 · 2023-08-21T12:18:51Z

crates/extensions/c8y_mapper_ext/src/converter.rs

@@ -796,6 +888,15 @@ impl Converter for CumulocityConverter {
    }
 }

+/// Check if a message is an entity registration message.
+fn is_entity_register_message(message: &Message) -> bool {


Could be moved to entity_store.rs

Moved to entity_store.rs

Bravo555 · 2023-08-21T12:20:34Z

crates/extensions/c8y_mapper_ext/src/converter.rs

@@ -885,6 +986,13 @@ fn add_external_device_registration_message(
    false
 }

+fn external_device_registration_message(entity: &EntityMetadata) -> Option<Message> {


Could be moved to entity_store.rs

No, looking closer, this method is c8y related. The point is to register the device on the cloud.

The function that should be moved to entity_store.rs is auto_register_entity().

The function that should be moved to entity_store.rs is auto_register_entity().

Done

codecov · 2023-08-21T16:13:20Z

Codecov Report

Merging #2169 (96f7f89) into main (75127c6) will decrease coverage by 0.1%.
The diff coverage is 88.1%.

❗ Current head 96f7f89 differs from pull request most recent head 98f1735. Consider uploading reports for the commit 98f1735 to get more accurate results

Additional details and impacted files

Files Changed	Coverage Δ
crates/core/tedge_api/src/topic.rs	`94.5% <ø> (-0.8%)`	⬇️
crates/core/tedge_mapper/src/collectd/mapper.rs	`0.0% <ø> (ø)`
crates/extensions/c8y_mapper_ext/src/actor.rs	`70.6% <ø> (+2.4%)`	⬆️
crates/extensions/c8y_mapper_ext/src/converter.rs	`76.7% <79.1%> (-2.4%)`	⬇️
crates/core/tedge_api/src/entity_store.rs	`91.6% <92.1%> (+2.7%)`	⬆️
crates/extensions/c8y_mapper_ext/src/json.rs	`98.0% <94.7%> (-0.4%)`	⬇️
.../tedge_config/src/tedge_config_cli/tedge_config.rs	`87.2% <100.0%> (ø)`
crates/core/tedge_api/src/entity.rs	`84.2% <100.0%> (+2.0%)`	⬆️
crates/extensions/c8y_mapper_ext/src/config.rs	`62.5% <100.0%> (-0.5%)`	⬇️
crates/extensions/c8y_mapper_ext/src/serializer.rs	`81.5% <100.0%> (+0.7%)`	⬆️
... and 1 more

... and 7 files with indirect coverage changes

Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>

didier-wenzek requested a review from Bravo555 August 18, 2023 11:42

didier-wenzek had a problem deploying to Test Pull Request August 18, 2023 11:56 — with GitHub Actions Failure

didier-wenzek commented Aug 18, 2023

View reviewed changes

didier-wenzek changed the title ~~Update c8y-mapper to consume te measurements~~ Update c8y-mapper to consume te measurements Aug 18, 2023

didier-wenzek had a problem deploying to Test Pull Request August 18, 2023 13:11 — with GitHub Actions Failure

didier-wenzek force-pushed the feat/mapper-consume-te-measurements branch from d4cc6dd to 915d6d1 Compare August 18, 2023 13:29

didier-wenzek had a problem deploying to Test Pull Request August 18, 2023 13:50 — with GitHub Actions Failure

didier-wenzek had a problem deploying to Test Pull Request August 18, 2023 14:28 — with GitHub Actions Failure

didier-wenzek had a problem deploying to Test Pull Request August 21, 2023 08:23 — with GitHub Actions Failure

Bravo555 requested changes Aug 21, 2023

View reviewed changes

didier-wenzek had a problem deploying to Test Pull Request August 21, 2023 16:24 — with GitHub Actions Failure

didier-wenzek had a problem deploying to Test Pull Request August 22, 2023 08:05 — with GitHub Actions Failure

didier-wenzek had a problem deploying to Test Pull Request August 22, 2023 12:52 — with GitHub Actions Failure

didier-wenzek mentioned this pull request Aug 22, 2023

Improve EntityStore API #2179

Closed

8 tasks

didier-wenzek added 2 commits August 22, 2023 15:24

Move the EntityStore into the c8y converter

0945043

Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>

Move auto-registration logic inside the converter

e31c121

Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>

didier-wenzek force-pushed the feat/mapper-consume-te-measurements branch from ac86e38 to ce14ef6 Compare August 22, 2023 14:05

didier-wenzek had a problem deploying to Test Pull Request August 22, 2023 14:18 — with GitHub Actions Failure

didier-wenzek had a problem deploying to Test Pull Request August 22, 2023 15:07 — with GitHub Actions Failure

didier-wenzek added 3 commits August 22, 2023 17:50

Update c8y-mapper to translate te/+/+/+/+/m/+ measurements

e980464

Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>

Remove all reference to tedge/measurements for c8y and collect

a854dce

Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>

Move tedge specific code from c8y-mapper-ext to tedge-api

98f1735

Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>

didier-wenzek force-pushed the feat/mapper-consume-te-measurements branch from 96f7f89 to 98f1735 Compare August 22, 2023 15:52

didier-wenzek temporarily deployed to Test Pull Request August 22, 2023 16:02 — with GitHub Actions Inactive

Bravo555 approved these changes Aug 22, 2023

View reviewed changes

didier-wenzek merged commit ea68aac into thin-edge:main Aug 22, 2023
15 of 16 checks passed

didier-wenzek deleted the feat/mapper-consume-te-measurements branch August 22, 2023 16:44

didier-wenzek mentioned this pull request Feb 22, 2024

If the first message sent by a child device is ill-formed, the child device is never registered #2737

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update c8y-mapper to consume `te` measurements #2169

Update c8y-mapper to consume `te` measurements #2169

didier-wenzek commented Aug 18, 2023 •

edited

didier-wenzek Aug 18, 2023

didier-wenzek Aug 18, 2023

didier-wenzek Aug 18, 2023

didier-wenzek Aug 18, 2023

Bravo555 Aug 21, 2023

didier-wenzek Aug 21, 2023

didier-wenzek Aug 18, 2023

didier-wenzek Aug 18, 2023

didier-wenzek Aug 18, 2023

didier-wenzek Aug 18, 2023

didier-wenzek Aug 18, 2023

didier-wenzek Aug 18, 2023

github-actions bot commented Aug 18, 2023 •

edited

Bravo555 Aug 21, 2023

didier-wenzek Aug 21, 2023

Bravo555 Aug 21, 2023

didier-wenzek Aug 21, 2023

Bravo555 Aug 21, 2023

Bravo555 Aug 21, 2023

didier-wenzek Aug 21, 2023

didier-wenzek Aug 22, 2023

Bravo555 Aug 21, 2023

didier-wenzek Aug 22, 2023

Bravo555 Aug 21, 2023

didier-wenzek Aug 22, 2023

didier-wenzek Aug 22, 2023

didier-wenzek Aug 22, 2023

codecov bot commented Aug 21, 2023 •

edited

		let device_register_payload =
		format!("{{ \"@type\":\"{device_type}\", \"@id\":\"{device_name}\"}}");

Update c8y-mapper to consume te measurements #2169

Update c8y-mapper to consume te measurements #2169

Conversation

didier-wenzek commented Aug 18, 2023 • edited

Proposed changes

Types of changes

Paste Link to the issue

Checklist

Further comments

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

github-actions bot commented Aug 18, 2023 • edited

Robot Results

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

Choose a reason for hiding this comment

codecov bot commented Aug 21, 2023 • edited

Codecov Report

Update c8y-mapper to consume `te` measurements #2169

Update c8y-mapper to consume `te` measurements #2169

didier-wenzek commented Aug 18, 2023 •

edited

github-actions bot commented Aug 18, 2023 •

edited

codecov bot commented Aug 21, 2023 •

edited