Consistent handling of custom MQTT topic root

[Bravo555]

Is your refactoring request related to a problem? Please describe.

Because all the components operate on MQTT messages and topics directly, the introduction of the requirement to support custom MQTT root made it so that all these components now need to query mqtt.topic_root config option and use it when producing and consuming topics, which leads to the following bad things:

• significant boilerplate
• it's easy to forget to handle it
• it's sometimes hard handle it correctly in places that don't have access to a tedge_config object

Describe the solution you'd like

The MQTT topic root should be handled in a single place which is dedicated to reading MQTT messages from the MQTT broker and then passing it to the rest of the system: the MQTT actor

The actor, when receiving MQTT messages from other actors should append the topic root, and only then send them to the MQTT broker, and when receiving messages from the broker, it should strip out the prefix and only then send the message to other actors.

But, continuing to use mqtt_channel::Message, just with the caveat that "topic does not include MQTT topic root" will be very awkward and troublesome to use. Instead, we should create a new type, called e.g. ThinEdgeMessage, which actors will use to communicate with the MQTT actor, and then the MQTT actor would convert between ThinEdgeMessage and MqttMessage, in a single place, depending on a single piece of state read from the config.

ThinEdgeMessage would be used for communication between local thin-edge services and the mapper, but it shouldn't be used for communication between the mapper and the cloud, as there, a different topic scheme is used. As such, the mappers should be able to send both ThinEdgeMessages and MqttMessages to their MQTT actors, while local thin-edge services should only be able to send ThinEdgeMessages.

Describe alternatives you've considered

ThinEdgeMessage could be used to even further abstract away MQTT use thin-edge-api directly:

struct ThinEdgeMessage {
    sender: EntityTopicId,
    kind: MessageKind
}

enum MessageKind {
    Measurement(ThinEdgeMeasurement),
    Event(ThinEdgeEvent),
    Alert(ThinEdgeAlert),
    Health(ThinEdgeHealth),
}

making it easier for actors to decode and act on different messages they receive. Information about an entity which sent the message could be accessible at its own field, without the need to use MqttSchema to parse the topic, and the Channel portion of the topic could be combined with the payload to create a more direct representation of the message.

This kind of refactor would necessitate significant effort however, as all the actors would have to be rewritten to use this new message type due to significant structural changes. In contrast, a limited ThinEdgeMessage type retaining MQTT topic/payload structure, only encoding the caveat that it doesn't include the topic root, which will be appended by the MQTT actor when sending the message over MQTT, will be easier to do.

I expect following changes will have to be made:

• Create a new ThinEdgeMessage type for MQTT messages without the topic root
• Introduce a new variant of the MQTT actor with a channel of type ThinEdgeMessage
• Provide MqttSchema::entity_channel_of/topic_for alternative which operates on ThinEdgeMessage
• Replace current MqttActor<MqttMessage> with MqttActor<ThinEdgeMessage> for all local thin-edge services.

At this point MQTT topic root would only be handled by the MQTT actor, but as some services still operate on MqttMessages natively, MqttSchema could be used as a compatibility layer for services which are still due to be rewritten to use ThinEdgeMessage natively.

Additional context

This is an issue made in response to https://github.com/thin-edge/thin-edge.io/pull/2353#discussion_r1366491853, but I just wanted to get this stuff out of my head. Maybe I'll write a more detailed proposal in the future, but for now, it's necessary to track where we still don't handle the MQTT topic root correctly, so here it goes.

Places where custom MQTT topic root is not handled:

• C8y Mapper MQTT Bridge
  • /workspace/crates/core/c8y_api/src/utils.rs:5
  • /workspace/crates/core/tedge/src/cli/connect/bridge_config_c8y.rs:101 However, as there is one bridge per mapper, and only one mapper of a given type, per thin-edge instance, there will not ever be multiple tedge-mapper-c8y or mosquitto-bridge-c8y clients connected to the same MQTT broker and publishing under different MQTT topic roots. I think it necessitates some discussion.
• Entity store (crates/core/tedge_api/src/entity_store.rs:33)
• tedge_to_te_converter
• tedge-mapper-collectd (crates/core/tedge_mapper/src/collectd/mapper.rs:15)

The list is possibly incomplete, I've searched for regex "te(\/|") in *.rs files excluding tests.rs files, but there are still test modules in other files where te/ is used.

[didier-wenzek]

This proposal contains both really good and wrong suggestions.

• I'm definitely against the idea of an MQTT actor that adds/removes a te suffix to all messages. This actor is also used to send/receive messages that are unrelated to tedge.
• The suggested alternative for ThinEdgeMessage is a really nice suggestion and this is the way to go (mid-term) in combination with a connection to the (regular) MQTTActor which translates on the fly these ThinEdgeMessage into Mqtt::Message, back and forth (using the fact that connection between actors is done is using a dyn Sender).

This kind of refactor would necessitate significant effort however

I'm afraid this will be the case. Hence this cannot be the short term solution.

Places where custom MQTT topic root is not handled

We also have to look for use of MqttSchema::default()

However, as there is one bridge per mapper, and only one mapper of a given type, per thin-edge instance, there will not ever be multiple tedge-mapper-c8y or mosquitto-bridge-c8y clients connected to the same MQTT broker and publishing under different MQTT topic roots. I think it necessitates some discussion.

Yes, we need to discuss this.

[didier-wenzek]

Related point that also need to be fixed:

• Remove const MQTT_ROOT: &str = "te"; defined by the EntityStore. The root prefix should be provided by an MqttSchema.