Events for c8y are consumed from te/+/+/+/+/e/+ #2221

rina23q · 2023-09-05T15:19:05Z

Proposed changes

Events for c8y should be consumed from the mqtt topic te/+/+/+/+/e/+ instead of tedge/events/#.

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

#2214

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 · 2023-09-05T15:23:07Z

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

@@ -21,13 +21,13 @@ Child devices support sending custom measurements


 Child devices support sending custom events
-    Execute Command    tedge mqtt pub tedge/events/myCustomType1/${CHILD_SN} '{ "text": "Some test event", "someOtherCustomFragment": {"nested":{"value": "extra info"}} }'
+    Execute Command    tedge mqtt pub te/device/${CHILD_SN}///e/myCustomType1 '{ "text": "Some test event", "someOtherCustomFragment": {"nested":{"value": "extra info"}} }'
    ${events}=    Device Should Have Event/s    expected_text=Some test event    with_attachment=False    minimum=1    maximum=1    type=myCustomType1    fragment=someOtherCustomFragment
    Log    ${events}


 Child devices support sending custom events overriding the type


This test gives false-positive to the current main branch. It cannot detect if two "type" fields are added as the test check only if the payload "contains" a type=otherType.

In my opinion, this test can be covered by Rust code as below.

thin-edge.io/crates/extensions/c8y_mapper_ext/src/converter.rs

Lines 1480 to 1496 in 993a7c2

async fn convert_event_use_event_type_from_payload() {

let tmp_dir = TempTedgeDir::new();

let (mut converter, _http_proxy) = create_c8y_converter(&tmp_dir).await;

let event_topic = "te/device/main///e/topic_event";

let event_payload = r#"{ ".type": "payload_event", "text": "Someone clicked", "time": "2020-02-02T01:02:03+05:30" }"#;

let event_message = Message::new(&Topic::new_unchecked(event_topic), event_payload);

let converted_events = converter.convert(&event_message).await;

assert_eq!(converted_events.len(), 1);

let converted_event = converted_events.get(0).unwrap();

assert_eq!(converted_event.topic.name, "c8y/s/us");

assert_eq!(

converted_event.payload_str().unwrap(),

r#"400,"payload_event","Someone clicked",2020-02-02T01:02:03+05:30"#

);

}

rina23q · 2023-09-05T15:23:45Z

tests/RobotFramework/tests/cumulocity/telemetry/thin-edge_device_telemetry.robot

@@ -27,13 +27,13 @@ Thin-edge devices support sending custom measurements


 Thin-edge devices support sending custom events
-    Execute Command    tedge mqtt pub tedge/events/myCustomType1 '{ "text": "Some test event", "someOtherCustomFragment": {"nested":{"value": "extra info"}} }'
+    Execute Command    tedge mqtt pub te/device/main///e/myCustomType1 '{ "text": "Some test event", "someOtherCustomFragment": {"nested":{"value": "extra info"}} }'
    ${events}=    Device Should Have Event/s    expected_text=Some test event    with_attachment=False    minimum=1    maximum=1    type=myCustomType1    fragment=someOtherCustomFragment
    Log    ${events}


 Thin-edge devices support sending custom events overriding the type


This test is also giving false-positive.

What is the false positive / how is it a false positive? Is the someOtherCustomFragment property not included in the event?

Actually the current main branch has a bug. If user publishes an event with type field in the payload, the mapper creates "type" field twice.

"type":"myCustomType"

"type":"otherType"

But the test passed because the test checks if contains type=otherType.

Here is the trace.

[tedge/events/myCustomType] {"type": "otherType", "text": "Some test event", "someOtherCustomFragment": {"nested":{"value": "extra info"}} } [c8y/event/events/create] {"type":"myCustomType","time":"2023-09-06T09:29:59.89872988Z","text":"Some test event","type":"otherType","someOtherCustomFragment":{"nested":{"value":"extra info"}}}

Oh, duplicating keys definitely should be avoided, whilst it is not forbidden it is not recommended and it causes massive issues with different libraries (some support duplicate object keys, some don't)..as the JSON spec says The names within an object SHOULD be unique.

@rina23q So is the fix to avoid duplicated keys also included in the PR?

@rina23q So is the fix to avoid duplicated keys also included in the PR?

Yes ofc :) Rust test case convert_event_use_event_type_from_payload_to_c8y_json() is covering this.

codecov · 2023-09-05T15:36:04Z

Codecov Report

Merging #2221 (8f6d256) into main (8edc2d8) will increase coverage by 1.1%.
Report is 12 commits behind head on main.
The diff coverage is 83.6%.

Additional details and impacted files

Files Changed	Coverage Δ
crates/extensions/c8y_mapper_ext/src/converter.rs	`79.9% <77.1%> (+1.7%)`	⬆️
crates/core/c8y_api/src/json_c8y.rs	`83.6% <84.0%> (-0.3%)`	⬇️
crates/core/tedge_api/src/event.rs	`95.1% <97.4%> (+4.0%)`	⬆️
.../tedge_config/src/tedge_config_cli/tedge_config.rs	`89.3% <100.0%> (ø)`
crates/extensions/c8y_mapper_ext/src/config.rs	`56.1% <100.0%> (-0.5%)`	⬇️

... and 24 files with indirect coverage changes

github-actions · 2023-09-05T16:22:03Z

Robot Results

✅ Passed	❌ Failed	⏭️ Skipped	Total	Pass %	⏱️ Duration
263	0	5	263	100	47m50.221s

crates/core/c8y_api/src/json_c8y.rs

albinsuresh · 2023-09-06T05:26:36Z

crates/common/tedge_config/src/tedge_config_cli/tedge_config.rs

-        #[tedge_config(example = "tedge/alarms/#,te/+/+/+/+/m/+")]
-        #[tedge_config(default(value = "te/+/+/+/+/m/+,tedge/alarms/+/+,tedge/alarms/+/+/+,tedge/events/+,tedge/events/+/+,tedge/health/+,tedge/health/+/+"))]
+        #[tedge_config(example = "tedge/alarms/#,te/+/+/+/+/m/+,te/+/+/+/+/e/+")]
+        #[tedge_config(default(value = "te/+/+/+/+/m/+,te/+/+/+/+/e/+,tedge/alarms/+/+,tedge/alarms/+/+/+,tedge/health/+,tedge/health/+/+"))]


Wondering why we're replacing the tedge/events topics with the te/.../e topics. Shouldn't they both coexist until we fully stop supporting the tedge/... topics? Else who will subscribe to the tedge/events topics and map those messages to their te/.../e equivalent?

This is the configuration per cloud, so I believe it's safe to replace tedge/events with te/.../e topic for only c8y mapper. Still the default config of the aws and az mappers are subscribing to tedge/events (and tedge/measurements).

Also, don't miss that tedge-agent converts from tedge/... topics to te/... topics. This means c8y-mapper users can still use tedge/events topics to create events.

The agent does the translation from tedge/# to te/#.

This has been implemented by #2125

crates/core/tedge_api/src/event.rs

albinsuresh · 2023-09-06T05:37:16Z

crates/extensions/c8y_mapper_ext/src/config.rs

-            "tedge/events/+",
-            "tedge/events/+/+",


Seeing these tedge/ topics replaced by the te/ topics, while the former is only deprecated for the next release but not removed, I feel I'm missing something here. Especially since the same was done for tedge/measurements as well.

The agent does the translation from tedge/# to te/#.

This has been implemented by #2125

albinsuresh · 2023-09-06T05:45:07Z

crates/extensions/c8y_mapper_ext/src/converter.rs

-        } else {
-            false
+        let event_type = match event_type.is_empty() {
+            true => DEFAULT_EVENT_TYPE,


That's new. Earlier this was an error case, reported as a parsing error on tedge/errors topic. It was different for tedge/measurements as the type wasn't mandatory for it. But for events and alarms, the type was mandatory. So I hope this was intentional. @reubenmiller could you confirm?

Yes having a default type used for events will useful as it makes it easier to publish an event, and it aligns with the measurement type handling so it makes for a nice and consistent API.

And I'm pretty sure no one is actually subscribing to the tedge/errors topics, so usually they just wonder why the event is not being published.

* If input event type is empty, the converter uses "ThinEdgeEvent" as the event type. * Event converter uses entity store and no longer need to care if a child device is already created. Signed-off-by: Rina Fujino <18257209+rina23q@users.noreply.github.com>

Signed-off-by: Rina Fujino <18257209+rina23q@users.noreply.github.com>

crates/core/c8y_api/src/json_c8y.rs

didier-wenzek · 2023-09-06T15:49:40Z

crates/extensions/c8y_mapper_ext/src/converter.rs

+            // If the message doesn't contain any fields other than `text` and `time`, convert to SmartREST
+            let message = if c8y_event.extras.is_empty() {
+                let smartrest_event = Self::serialize_to_smartrest(&c8y_event)?;
+                let smartrest_topic = Topic::new_unchecked(SMARTREST_PUBLISH_TOPIC);
+                Message::new(&smartrest_topic, smartrest_event)
+            } else {
+                // If the message contains extra fields other than `text` and `time`, convert to Cumulocity JSON
+                let cumulocity_event_json = serde_json::to_string(&c8y_event)?;
+                let json_mqtt_topic = Topic::new_unchecked(C8Y_JSON_MQTT_EVENTS_TOPIC);
+                Message::new(&json_mqtt_topic, cumulocity_event_json)
+            };


We agreed offline to simplify this by systematically publishing the events over the c8y json topic. This will be done in a follow-up PR, though.

crates/extensions/c8y_mapper_ext/src/converter.rs

didier-wenzek

Approved. Thank you

reubenmiller

Looks good, though do we have a robot test case to cover publishing without the type in the topic?

rina23q · 2023-09-07T08:40:23Z

Looks good, though do we have a robot test case to cover publishing without the type in the topic?

Robot case no. But I added the case to Rust convert_event_without_given_event_type()

Updated: Now two robot tests are added to cover it.

Child devices support sending custom events without type in topic
Thin-edge devices support sending custom events without type in topic

Replace `tedge/events/#` topics used in Robot Framework tests for c8y-mapper by `te/+/+/+/+/e/+`. Signed-off-by: Rina Fujino <18257209+rina23q@users.noreply.github.com>

Signed-off-by: Rina Fujino <18257209+rina23q@users.noreply.github.com>

rina23q commented Sep 5, 2023

View reviewed changes

rina23q temporarily deployed to Test Pull Request September 5, 2023 15:32 — with GitHub Actions Inactive

reubenmiller reviewed Sep 5, 2023

View reviewed changes

crates/core/c8y_api/src/json_c8y.rs Outdated Show resolved Hide resolved

albinsuresh reviewed Sep 6, 2023

View reviewed changes

rina23q added 2 commits September 6, 2023 12:02

Update tedge config to subscribes to te/+/+/+/+/e/+ topic

53dbd87

Signed-off-by: Rina Fujino <18257209+rina23q@users.noreply.github.com>

rina23q force-pushed the feature/2082/c8y-mapper-forward-events-with-new-mqtt-scheme branch from 993a7c2 to 6332046 Compare September 6, 2023 10:04

rina23q requested review from albinsuresh, reubenmiller and didier-wenzek September 6, 2023 10:05

rina23q had a problem deploying to Test Pull Request September 6, 2023 10:14 — with GitHub Actions Failure

rina23q had a problem deploying to Test Pull Request September 6, 2023 13:35 — with GitHub Actions Failure

rina23q temporarily deployed to Test Pull Request September 6, 2023 15:28 — with GitHub Actions Inactive

didier-wenzek reviewed Sep 6, 2023

View reviewed changes

rina23q temporarily deployed to Test Pull Request September 6, 2023 17:25 — with GitHub Actions Inactive

didier-wenzek approved these changes Sep 6, 2023

View reviewed changes

reubenmiller reviewed Sep 7, 2023

View reviewed changes

rina23q force-pushed the feature/2082/c8y-mapper-forward-events-with-new-mqtt-scheme branch from e9feb77 to 55d2306 Compare September 7, 2023 10:16

rina23q had a problem deploying to Test Pull Request September 7, 2023 10:25 — with GitHub Actions Failure

rina23q added 2 commits September 7, 2023 10:30

Update Robot tests to use new event topic schema

89597cb

Replace `tedge/events/#` topics used in Robot Framework tests for c8y-mapper by `te/+/+/+/+/e/+`. Signed-off-by: Rina Fujino <18257209+rina23q@users.noreply.github.com>

Use event type from payload if it's given in payload as "type"

8f6d256

Signed-off-by: Rina Fujino <18257209+rina23q@users.noreply.github.com>

rina23q force-pushed the feature/2082/c8y-mapper-forward-events-with-new-mqtt-scheme branch from 55d2306 to 8f6d256 Compare September 7, 2023 10:31

rina23q temporarily deployed to Test Pull Request September 7, 2023 10:41 — with GitHub Actions Inactive

rina23q merged commit 7f58cd2 into thin-edge:main Sep 7, 2023
18 checks passed

rina23q deleted the feature/2082/c8y-mapper-forward-events-with-new-mqtt-scheme branch September 7, 2023 11:39

rina23q mentioned this pull request Sep 8, 2023

Events for c8y are consumed from te/+/+/+/+/e/+ #2214

Closed

rina23q mentioned this pull request Sep 11, 2023

C8Y Mapper converts alarms/events to only JSON over MQTT #2242

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Events for c8y are consumed from te/+/+/+/+/e/+ #2221

Events for c8y are consumed from te/+/+/+/+/e/+ #2221

rina23q commented Sep 5, 2023 •

edited

rina23q Sep 5, 2023

rina23q Sep 5, 2023

reubenmiller Sep 5, 2023

rina23q Sep 6, 2023 •

edited

reubenmiller Sep 7, 2023

rina23q Sep 7, 2023

codecov bot commented Sep 5, 2023 •

edited

github-actions bot commented Sep 5, 2023 •

edited

albinsuresh Sep 6, 2023

rina23q Sep 6, 2023 •

edited

didier-wenzek Sep 6, 2023

albinsuresh Sep 6, 2023

didier-wenzek Sep 6, 2023

albinsuresh Sep 6, 2023

reubenmiller Sep 6, 2023

reubenmiller Sep 6, 2023

didier-wenzek Sep 6, 2023

didier-wenzek left a comment

reubenmiller left a comment

rina23q commented Sep 7, 2023 •

edited

	async fn convert_event_use_event_type_from_payload() {
	let tmp_dir = TempTedgeDir::new();
	let (mut converter, _http_proxy) = create_c8y_converter(&tmp_dir).await;
	let event_topic = "te/device/main///e/topic_event";
	let event_payload = r#"{ ".type": "payload_event", "text": "Someone clicked", "time": "2020-02-02T01:02:03+05:30" }"#;
	let event_message = Message::new(&Topic::new_unchecked(event_topic), event_payload);

	let converted_events = converter.convert(&event_message).await;
	assert_eq!(converted_events.len(), 1);
	let converted_event = converted_events.get(0).unwrap();
	assert_eq!(converted_event.topic.name, "c8y/s/us");

	assert_eq!(
	converted_event.payload_str().unwrap(),
	r#"400,"payload_event","Someone clicked",2020-02-02T01:02:03+05:30"#
	);
	}

Events for c8y are consumed from te/+/+/+/+/e/+ #2221

Events for c8y are consumed from te/+/+/+/+/e/+ #2221

Conversation

rina23q commented Sep 5, 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

rina23q Sep 6, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 5, 2023 • edited

Codecov Report

github-actions bot commented Sep 5, 2023 • edited

Robot Results

Choose a reason for hiding this comment

rina23q Sep 6, 2023 • edited

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 left a comment

Choose a reason for hiding this comment

reubenmiller left a comment

Choose a reason for hiding this comment

rina23q commented Sep 7, 2023 • edited

rina23q commented Sep 5, 2023 •

edited

rina23q Sep 6, 2023 •

edited

codecov bot commented Sep 5, 2023 •

edited

github-actions bot commented Sep 5, 2023 •

edited

rina23q Sep 6, 2023 •

edited

rina23q commented Sep 7, 2023 •

edited