thin-edge / thin-edge.io

The open edge framework for lightweight IoT devices
https://thin-edge.io
Apache License 2.0
221 stars 54 forks source link

tedge alarm does not transform message fragment to text in the tedge-mapper-c8y #1710

Closed reubenmiller closed 1 year ago

reubenmiller commented 1 year ago

Is your feature improvement request related to a problem? Please describe.

The .message fragment in tedge alarms is no longer respected correctly in the tedge-mapper-c8y service. This was caused by merging #1699 (after the official release of 0.9.0, so it does not affect any official releases just yet)

Officially the tedge specification for alarms states that a tedge alarm can have the .message fragment to provide the alarm description. The expectation is the .message fragment is then used by each cloud mapper to map to the cloud providers data model. In the case of Cumulocity IoT, this means using the .message fragment as the .text field when creating a Cumulocity IoT Alarm.

Below is the documented example of the alarm data structure (from the spec)

Topic:   tedge/alarms/<severity-string>/<type-string>
Payload: {
  "message":  <message-string>,
  "status":   <status-string>,  
  "time":     <time-string>
}

The online user documentation seems to be vague on the correct schema, it is only mentioned in an example about publishing data using tedge mqtt pub, here is the example

tedge mqtt pub --retain --qos 1 tedge/alarms/critical/high_temperature '{"message": "Temperature is critical"}'

The tedge-mapper-c8y service is translating the .message fragment to the .text fragment when pushing the alarm to Cumulocity IoT, however since #1699 was merged, which added support for custom fragments in alarms, the translation of the .message field was lost.

Describe the solution you'd like

I am open to how we solve this, e.g. if we remove support for publishing the alarm message on the .message and use the .text field, or we stick with the original spec.

The following solutions are with regarding the handling of an json alarm payload being published on the following topic (for both the main and child device topics):

# main device
tedge/alarms/<severity-string>/<type-string>

# child devices
tedge/alarms/<severity-string>/<type-string>/<child-id>

Option 1: Keep supporting the .message fragment

The tedge-mapper-c8y should transform the .message fragment to the .text fragment, if the user does not already specify the .text field.

The transformation should adhere to the following rules:

If the alarm payload:

Examples of the tranformation (only the .text and .message fragments will be used in the examples, and the .type, .time information will be ignored just for simplicity.

Tedge Alarm (input) Cumulocity IoT Alarm (output)
{} {"text": "Alarm of type 'temperature_high' raised"}
{"message": "Temperature high"} {"text": "Temperature high"}
{"text": "Temperature high"} {"text": "Temperature high"}
{"message": "Some other text", "text": "Temperature high"} {"message": "Some other text", "text": "Temperature high"}

Option 2: Remove support for the .message fragment translation If we remove support for this, then we need to document it properly by doing the following:

Describe alternatives you've considered

N/A Additional context

reubenmiller commented 1 year ago

Extended the bug description to include a note that this is only a bug on the current main branch (merged after 0.9.0 release) and does not affect any official releases yet.

reubenmiller commented 1 year ago

The decision has been made to go with Option 1 (re-introduce the .message fragment handling)

Update: Switched to implement Option 2 instead see reasoning

reubenmiller commented 1 year ago

Also the documentation should be updated to better highlight this interaction

reubenmiller commented 1 year ago

After careful inspection of the previous behaviour (tested in 0.8.1 and 0.7.5), the .message wasn't actually being mapped to the .text fragment when creating the Cumulocity IoT alarm, however from a logical standpoint, the .message (from the tedge alarm scheme) contains the information that Cumulocity IoT expects in the .text fragment (in the outgoing alarm message)

So it definitely makes sense that the outgoing c8y alarm message contains a human readable .text fragment where possible.

didier-wenzek commented 1 year ago

Also the documentation should be updated to better highlight this interaction

We should promote as doc the alarm specs.

And this should be done for all the specs https://github.com/thin-edge/thin-edge.io/issues/1547 as proposed here https://github.com/thin-edge/thin-edge.io/discussions/799.

reubenmiller commented 1 year ago

After a further discussion and confusion around alarm handling we will implement "Option 2", where the official tedge alarm spec will change to expect the alarm description being provided in the .text fragment (not .message). The user can still provide additional custom fragments (including .message).

Reason for switching to Option 2

didier-wenzek commented 1 year ago

After a further discussion and confusion around alarm handling we will implement "Option 2", where the official tedge alarm spec will change to expect the alarm description being provided in the .text fragment (not .message). The user can still provide additional custom fragments (including .message).

I'm okay with that even if not convinced by the name size argument ;-). I only want to stress that conflicting names and resolution as with option 1 is not something unusual. On the contrary, this is something that will be more an more frequent as thin-edge will integrate more clouds and data sources.

reubenmiller commented 1 year ago

The spec was updated to remove reference to the .message fragment, as the code has been using the .text fragment.

gligorisaev commented 1 year ago

Created test: https://github.com/thin-edge/thin-edge.io/pull/1887