azure tedge mapper fails to translate health status messages #2273

PradeepKiruvale · 2023-09-20T04:46:53Z

Proposed changes

Issue:
Azure mapper converter was failing to translate the service health message timestamp, because it's an integer number.

Proposed solution/fix:
Forward the health status message payload to Azure Cloud without translating.

As a followup tasks

Unify the timestamps to RFC3339 format across thin-edge.io tedge-mapper-aws health message uses two different timestamp formats for up and down messages #2237
Send meaningful health status messages to Azure cloud by adding more information about the service name in the health status message Improve the health status messages to azure cloud #2283

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

#2256

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-09-20T05:01:38Z

Codecov Report

Merging #2273 (47248f9) into main (7e6da86) will increase coverage by 0.0%.
Report is 19 commits behind head on main.
The diff coverage is 100.0%.

Additional details and impacted files

Files Changed	Coverage Δ
crates/extensions/az_mapper_ext/src/error.rs	`0.0% <ø> (ø)`
crates/extensions/az_mapper_ext/src/converter.rs	`97.3% <100.0%> (+0.4%)`	⬆️

... and 28 files with indirect coverage changes

github-actions · 2023-09-20T09:17:21Z

Robot Results

✅ Passed	❌ Failed	⏭️ Skipped	Total	Pass %	⏱️ Duration
273	0	5	273	100	52m49.136s

didier-wenzek · 2023-09-20T12:24:11Z

crates/extensions/az_mapper_ext/src/converter.rs

+            let mut payload_json: Map<String, Value> =
+                serde_json::from_slice(input.payload.as_bytes())?;
+            if let Some(timestamp) = default_timestamp {
+                let timestamp = timestamp
+                    .format(&time::format_description::well_known::Rfc3339)?
+                    .as_str()
+                    .into();
+                payload_json.entry("time").or_insert(timestamp);
+            }
+            let payload = serde_json::to_string(&payload_json)?;
+            Ok(vec![
+                (MqttMessage::new(&self.mapper_config.out_topic, payload)),
+            ])


Why not simply publish the health status message unchanged?

This piece of code will only add a timestamp to down status (the only ones without a time field).
And this done using a different time format as for the up status (as demonstrated by the converting_service_health_status test).

An alternative, is to rewrite the time field if not using the RFC 3339 format.

Yes, we can push the whole time stamp without doing any sort of translation.

Why not simply publish the health status message unchanged?

This piece of code will only add a timestamp to down status (the only ones without a time field). And this done using a different time format as for the up status (as demonstrated by the converting_service_health_status test).

An alternative, is to rewrite the time field if not using the RFC 3339 format.

@reubenmiller what's your thought on this? Shall we translate the time to RFC 3339 format or send out the message as it comes in?

We need to be consistent, otherwise we'll have the same problem as we have in #2237.

I would align to what the majority is or if you can find best practises in azure about timestamps. RFC3339 is more human readable but less machine readable.

@didier-wenzek The root cause is the API health_status_up_message(daemon_name: &str) where the timestamp is added as a Unix timestamp. So, shall we add in RFC 3339 format while adding a timestamp to the status message? then it will resolve all the issues and no need to re-write while parsing.

So, shall we add in RFC 3339 format while adding a timestamp to the status message?

Indeed an idea to consider.

rina23q

Some comments.

Tests are missing! I believe adding them to converter.rs is the right thing.
I'm not fully sure if I knew the requirements correctly. So, azure converter should translate these kind of messages?

From

[tedge/health/tedge-mapper-az] {"pid":51367,"status":"down"}
[tedge/health/tedge-mapper-az] {"pid":13280,"status":"up","time":1675330667}

To

[az/messages/events/] {"pid":51367,"status":"down"}
[az/messages/events/] {"pid":13280,"status":"up","time": 2023-09-20T14:37:49+00:00}

rina23q · 2023-09-20T14:25:49Z

crates/extensions/az_mapper_ext/src/converter.rs

        if is_bridge_health(&input.topic.name) {
            Ok(vec![])
+        } else if input.topic.name.starts_with("tedge/health") {


This check is too wild. The mapper subscribes to tedge/health/+ and tedge/health/+/+. Therefore, for example, I think this c8y message will be translated by the azure mapper.

tedge mqtt pub tedge/health/tedge-mapper-c8y '{"pid":19753,"status":"up","time":1694586060}'

@rina23q Though shouldn't the az mapper forward all service status messages that are published in the json format? It should not care which service is actually behind it, as you can use Azure to monitor the c8y service.

@reubenmiller Ah okay, but then simple forwarding the payload {"pid":19753,"status":"up","time":1694586060} to the topic az/messages/events/ is not enough. How can user know which service is behind the message?

Honestly speaking, I'm still confused what is preferred conversion output for azure.

@rina23q very good point regarding the sourceless messages about the service, though I think this will require more thought. Child devices are modelled as components using the $.sub nomenclature, but there isn't a great strategy for handling services of components.

It may be for another PR, but I feel maybe we can use the {property-bag} messages to send extra information. Find more info here.

Yes we can do the adjustments to how to communicate which service it is in a follow service. Though someone needs to point some thought into how to do this following the best practices of Azure IoT Hub.

@PradeepKiruvale Can you create a follow up ticket?

crates/extensions/az_mapper_ext/src/converter.rs

PradeepKiruvale · 2023-09-21T05:51:55Z

Some comments.

Tests are missing! I believe adding them to converter.rs is the right thing.

I'm not fully sure if I knew the requirements correctly. So, azure converter should translate these kind of messages?

From
[tedge/health/tedge-mapper-az] {"pid":51367,"status":"down"}
[tedge/health/tedge-mapper-az] {"pid":13280,"status":"up","time":1675330667}
To
[az/messages/events/] {"pid":51367,"status":"down"}
[az/messages/events/] {"pid":13280,"status":"up","time": 2023-09-20T14:37:49+00:00}

I already added a test with time.I will add it without a time stamp.

rina23q

Looks a good bug fix!

crates/extensions/az_mapper_ext/src/error.rs

crates/extensions/az_mapper_ext/src/converter.rs

Signed-off-by: Pradeep Kumar K J <pkj@softwareag.com>

didier-wenzek

Approved

PradeepKiruvale requested review from rina23q and didier-wenzek September 20, 2023 04:46

PradeepKiruvale temporarily deployed to Test Pull Request September 20, 2023 04:58 — with GitHub Actions Inactive

didier-wenzek reviewed Sep 20, 2023

View reviewed changes

rina23q reviewed Sep 20, 2023

View reviewed changes

PradeepKiruvale marked this pull request as draft September 22, 2023 01:30

PradeepKiruvale marked this pull request as ready for review September 22, 2023 03:11

PradeepKiruvale temporarily deployed to Test Pull Request September 22, 2023 03:17 — with GitHub Actions Inactive

rina23q approved these changes Sep 22, 2023

View reviewed changes

didier-wenzek reviewed Sep 22, 2023

View reviewed changes

crates/extensions/az_mapper_ext/src/error.rs Outdated Show resolved Hide resolved

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

Translate service health message

47248f9

Signed-off-by: Pradeep Kumar K J <pkj@softwareag.com>

PradeepKiruvale force-pushed the azure_health_bug branch from f0232af to 47248f9 Compare September 22, 2023 10:08

PradeepKiruvale temporarily deployed to Test Pull Request September 22, 2023 10:19 — with GitHub Actions Inactive

didier-wenzek approved these changes Sep 22, 2023

View reviewed changes

PradeepKiruvale merged commit be351da into thin-edge:main Sep 25, 2023
18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

azure tedge mapper fails to translate health status messages #2273

azure tedge mapper fails to translate health status messages #2273

PradeepKiruvale commented Sep 20, 2023 •

edited

codecov bot commented Sep 20, 2023 •

edited

github-actions bot commented Sep 20, 2023 •

edited

didier-wenzek Sep 20, 2023

PradeepKiruvale Sep 21, 2023

PradeepKiruvale Sep 21, 2023

reubenmiller Sep 21, 2023

PradeepKiruvale Sep 22, 2023

didier-wenzek Sep 22, 2023

rina23q left a comment

rina23q Sep 20, 2023

reubenmiller Sep 20, 2023

rina23q Sep 21, 2023 •

edited

reubenmiller Sep 21, 2023

PradeepKiruvale Sep 21, 2023

reubenmiller Sep 22, 2023 •

edited

PradeepKiruvale commented Sep 21, 2023

rina23q left a comment

didier-wenzek left a comment

azure tedge mapper fails to translate health status messages #2273

azure tedge mapper fails to translate health status messages #2273

Conversation

PradeepKiruvale commented Sep 20, 2023 • edited

Proposed changes

Types of changes

Paste Link to the issue

Checklist

Further comments

codecov bot commented Sep 20, 2023 • edited

Codecov Report

github-actions bot commented Sep 20, 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

rina23q left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rina23q Sep 21, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reubenmiller Sep 22, 2023 • edited

Choose a reason for hiding this comment

PradeepKiruvale commented Sep 21, 2023

rina23q left a comment

Choose a reason for hiding this comment

didier-wenzek left a comment

Choose a reason for hiding this comment

PradeepKiruvale commented Sep 20, 2023 •

edited

codecov bot commented Sep 20, 2023 •

edited

github-actions bot commented Sep 20, 2023 •

edited

rina23q Sep 21, 2023 •

edited

reubenmiller Sep 22, 2023 •

edited