thin-edge / thin-edge.io

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

Health status message mapping doesn't work when topic contains `bridge` #2370

Open Bravo555 opened 1 year ago

Bravo555 commented 1 year ago

Describe the bug

When any part of the topic contains the word bridge, message mapping is disabled, due to this check:

https://github.com/thin-edge/thin-edge.io/blob/d3ed45f4f1f8909a113693840bfec18b5bbc55c6/crates/extensions/c8y_mapper_ext/src/service_monitor.rs#L30-L33

To Reproduce

  1. Publish a health status message for an entity with a topic id that doesn't contain bridge: tedge mqtt pub te/device/main/service/my-custom-service/status/health '{"status": "up"}'

  2. The following 3 messages are published by the mapper (local registration message, cloud service registration message, service health status cloud update message):

    [te/device/main/service/my-custom-service] {"@id":"marcel_dev_container:device:main:service:my-custom-service","@parent":"device/main//","@type":"service","name":"my-custom-service","type":"service"}
    [c8y/s/us] 102,marcel_dev_container:device:main:service:my-custom-service,service,my-custom-service,up
    [c8y/s/us/marcel_dev_container:device:main:service:my-custom-service] 104,up
  3. Publish a health status message for an entity with a topic id that does contain bridge but is not an actual mosquitto bridge notification topic: tedge mqtt pub te/device/main/service/my-custom-bridge/status/health '{"status": "up"}'

  4. The following 2 message are published by the mapper (just local registration and cloud service creation):

    [te/device/main/service/my-custom-bridge] {"@id":"marcel_dev_container:device:main:service:my-custom-bridge","@parent":"device/main//","@type":"service","name":"my-custom-bridge","type":"service"}
    [c8y/s/us] 102,marcel_dev_container:device:main:service:my-custom-bridge,service,my-custom-bridge,up
  5. Successive health status messages for this entity are not mapped (no 104 smartrest messages are published by the mapper):

    $ tedge mqtt pub te/device/main/service/my-custom-bridge/status/health '{"status": "down"}
    [te/device/main/service/my-custom-bridge/status/health] {"status": "down"}
    $ tedge mqtt pub te/device/main/service/my-custom-bridge/status/health '{"status": "up"}'
    [te/device/main/service/my-custom-bridge/status/health] {"status": "up"}

Expected behavior

Health status should be correctly mapped for services which contain bridge in their topic ids.

Additional context

I'm wondering if using thin-edge topics for messages which are not valid thin-edge messages is a good idea in the first place. Special handling for messages published by special components might introduce non-trivial amount of complexity, as if we assume we can connect to multiple clouds at the same time, then all the mappers have to be aware that bridge notification health messages are special and to not try to map them the same as other health status messages.

We could maybe move mosquitto bridge notification messages out of "tree", by using some other root, e.g. $te for messages which are not the usual thin-edge messages, and more code would have to be added to special handling of these messages.

In any case, we have to be explicit about these contraints and any solution we go for, should be properly documented.

To me, it seems there is no obvious and simple solution for doing this "correctly" in a way that produces a minimal amount of unnecessary conflicts, so all suggestions are welcome.

PradeepKiruvale commented 1 year ago

If bridge is considered one of the main device services, then we can push that status to the cloud. In that case, we don't need all this segregation. @reubenmiller what is your opinion about this?

Note: There might be frequent changes in the bridge status then the status has to be pushed to the cloud, this may be a bit overkill though.

reubenmiller commented 9 months ago

thin-edge.io knows the name of the bridge services, so we should be able to narrow down the matching criterial rather than using "contains".