-
Notifications
You must be signed in to change notification settings - Fork 55
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support custom fragments in alarm creation #1699
Support custom fragments in alarm creation #1699
Conversation
@PradeepKiruvale Look ok from a non-rust pov. I will let @albinsuresh (or some other rust enabled person) to formally approve to prevent it from being merged prematurely. |
crates/core/tedge_api/src/alarm.rs
Outdated
pub struct ThinEdgeAlarmData { | ||
pub text: Option<String>, | ||
|
||
#[serde(default)] | ||
#[serde(with = "time::serde::rfc3339::option")] | ||
pub time: Option<Timestamp>, | ||
|
||
#[serde(flatten)] | ||
pub extras: HashMap<String, Value>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this named extras
? We use the term fragment in the documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, we can rename it as fragments
. But it will also contain the child device
info as part of the message. Can this child device info also be called a fragment? I am not sure. So, I kept it in line with the events
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the child-device-id
is stored as an entry in this extras
map, what's the content of the source
? I though it was the id of the child device sending the alarm?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I though it was the id of the child device sending the alarm?
Yes it is. Not just for child devices, but even for the parent device.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I renamed extras
as fragments
@@ -35,6 +35,7 @@ tracing = { version = "0.1", features = ["attributes", "log"] } | |||
anyhow = "1.0" | |||
assert-json-diff = "2.0" | |||
assert_matches = "1.5" | |||
maplit = "1.0.2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fine as a dev dependency. But just FYI: you could statically create HashMap using the from
method as follows:
let solar_distance = HashMap::from([
("Mercury", 0.4),
("Venus", 0.7),
("Earth", 1.0),
("Mars", 1.5),
]);
crates/core/tedge_api/src/alarm.rs
Outdated
@@ -106,10 +126,18 @@ impl ThinEdgeAlarm { | |||
Some(serde_json::from_str(mqtt_payload)?) | |||
}; | |||
|
|||
// The 4th part of the topic name is the event source - if any |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// The 4th part of the topic name is the event source - if any | |
// The 4th part of the topic name is the alarm source - if any |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
); | ||
output_messages.push(Message::new(&c8y_alarm_topic, smartrest_alarm)); | ||
} else { | ||
dbg!(&c8y_alarm); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't forget to remove this at the end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
crates/core/c8y_api/src/json_c8y.rs
Outdated
fn update_the_source( | ||
source_name: &str, | ||
) -> Result<Option<HashMap<String, String>>, SMCumulocityMapperError> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name and the type signature of this function hardly convey its purpose.
I would simply return a hash.
fn update_the_source( | |
source_name: &str, | |
) -> Result<Option<HashMap<String, String>>, SMCumulocityMapperError> { | |
fn make_c8y_source_fragment( | |
source_name: &str, | |
) -> HashMap<String, String> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
crates/core/c8y_api/src/json_c8y.rs
Outdated
#[serde(skip_serializing_if = "Option::is_none")] | ||
#[serde(rename = "externalSource")] | ||
pub source: Option<HashMap<String, String>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would consider to remove the Option type, using an empty map instead of None.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I replaced the hash map with a SourceInfo struct
for the source.
crates/core/c8y_api/src/json_c8y.rs
Outdated
#[derive(Debug, Serialize, Deserialize, PartialEq, Eq)] | ||
#[serde(rename_all = "camelCase")] | ||
pub struct C8yCreateAlarm { | ||
#[serde(skip_serializing_if = "Option::is_none")] | ||
pub source: Option<C8yManagedObject>, | ||
#[serde(rename = "externalSource")] | ||
pub source: Option<HashMap<String, String>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of using a generic Hashmap
here, I'd use a struct that replicates externalSource
JSON structure:
"externalSource":{
"externalId":"<child-device-id>",
"type":"c8y_Serial"
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed
@PradeepKiruvale I found the reason why the alarm is not visible in the Cumulocity UI, the I just tested with your version here, and below is the json that I saw was being pushed to the {
"externalSource": {
"externalId": "TST-extend_pure_loop_child1",
"type": "c8y_Serial"
},
"severity": "critical", // <<<<<<<<<<<<<<<<< needs to be UPPERCASE
"type": "myCustomAlarmType",
"time": "2023-02-01T21:08:14.800229538Z",
"text": "Some test alarm",
"someOtherCustomFragment": {
"nested": {
"value": "extra info"
}
}
} |
Good catch, but I remember testing with all CAPS. But still, it was not working then, Now it works, strange. Anyways I have updated the code to address this issue. |
a6b539a
to
d40ed80
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved
Signed-off-by: Pradeep Kumar K J <pradeepkumar.kj@softwareag.com>
e244f63
to
d09703f
Compare
Signed-off-by: Reuben Miller <reuben.d.miller@gmail.com>
Signed-off-by: Reuben Miller <reuben.d.miller@gmail.com>
Signed-off-by: Reuben Miller <reuben.d.miller@gmail.com>
Signed-off-by: Reuben Miller <reuben.d.miller@gmail.com>
Signed-off-by: Reuben Miller <reuben.d.miller@gmail.com>
Signed-off-by: Reuben Miller <reuben.d.miller@gmail.com>
Signed-off-by: Reuben Miller <reuben.d.miller@gmail.com>
Signed-off-by: Reuben Miller <reuben.d.miller@gmail.com>
Signed-off-by: Reuben Miller <reuben.d.miller@gmail.com>
Signed-off-by: Reuben Miller <reuben.d.miller@gmail.com>
Signed-off-by: Reuben Miller <reuben.d.miller@gmail.com>
Signed-off-by: Rina Fujino <18257209+rina23q@users.noreply.github.com>
chore: add maintainer scripts from existing debian packages for later comparison
…attempt Use JSON over MQTT to listen C8Y operations
Signed-off-by: Pradeep Kumar K J pradeepkumar.kj@softwareag.com
Proposed changes
Create an alarm with custom fragments
Supports for both
thin-edge
devices as well as for thechild
devices.For example, if an alarm message is published with a custom fragment as below a child device it must create an alarm with the custom message.
Types of changes
Paste Link to the issue
#1682
Checklist
cargo fmt
as mentioned in CODING_GUIDELINEScargo clippy
as mentioned in CODING_GUIDELINESFurther comments