feat: flow enabled c8y mapper#3934
Conversation
Robot Results
|
c1e9cf1 to
666848b
Compare
38bdaf8 to
01fba3f
Compare
albinsuresh
left a comment
There was a problem hiding this comment.
LGTM. Huge achievement, considering the complexity of this mapper with a lot of legacy code that isn't the most intuitive. There are a few points that could be improved, but as future iterations.
| let pending_messages = self.cache.take(); | ||
| for message in pending_messages.into_iter() { | ||
| if message.topic.starts_with(entity_topic) { |
There was a problem hiding this comment.
With this logic, if there are some cached messages for some entities (e.g: child1 and child2) in this cache, and when messages are received for another entity (e.g: child0) that is already registered, we'll be unnecessarily draining the cache first, only to put them all back. And we repeat that for every single message.
We could make this logic more efficient, by changing the cache from a simple RingBuffer<Message> to HasMap<EntityTopicId, RingBuffer<Message>> so that this step only consumes the messages for the current entity and leave the ones from other entities untouched. But, this is just a matter of efficiency and not correctness. So, it can be improved in a later PR.
There was a problem hiding this comment.
With this logic, if there are some cached messages for some entities (e.g:
child1andchild2) in this cache, and when messages are received for another entity (e.g:child0) that is already registered, we'll be unnecessarily draining the cache first, only to put them all back. And we repeat that for every single message.
The normal case is to have an empty cache and if not empty this will be for not too long, the main purpose of this cache being to solve races between registration and mea messages.
I see one annoying case though. A measurement which has been sent by mistake on behalf of a non-existing entity on a device configured not to use auto-registration. No registration message will ever be sent and the measurement sent by mistake will stay in the cache for ever. Worse, this measurement will be considered again and again for all cache drains.
In other word, I see more a cache pollution problem than a performance problem.
We could make this logic more efficient, by changing the
cachefrom a simpleRingBuffer<Message>toHasMap<EntityTopicId, RingBuffer<Message>>so that this step only consumes the messages for the current entity and leave the ones from other entities untouched. But, this is just a matter of efficiency and not correctness. So, it can be improved in a later PR.
Indeed, this would solve the issue of the unknown-source measurement polluting the drain process for all messages and all entities.
There was a problem hiding this comment.
We could make this logic more efficient, by changing the
cachefrom a simpleRingBuffer<Message>toHasMap<EntityTopicId, RingBuffer<Message>>so that this step only consumes the messages for the current entity and leave the ones from other entities untouched.
Done af732fe
| let message_topic = te.topic_for( | ||
| mapper, | ||
| &Channel::Status { | ||
| component: "entities".to_string(), | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Decision deferred for later.
| context: &FlowContextHandle, | ||
| ) -> Result<Vec<Message>, FlowError> { | ||
| match self.mqtt_schema.entity_channel_of(&message.topic) { | ||
| Ok((_, Channel::Status { component })) if component == "entities" => { |
There was a problem hiding this comment.
Do we also run into issues here if we have multiple c8y mappers running simultaneously (e.g. with multiple profiles)? Should we not be checking the service topic ID is for this mapper, and ignoring birth messages for other mappers?
There was a problem hiding this comment.
Agree. This is related to #3934 (comment)
As the birth messages are related to a c8y mapper instance, we have to use a c8y related topic for those. Precisely this needs to be a sub-topic of c8y.bridge.topic_prefix for the appropriate profile.
There was a problem hiding this comment.
Actually, the current code is working because the flows using this cache subscribe to a c8y mapper instance topic (aka te/device/main/service/tedge-mapper-c8y/status/entities).
- This can made more obvious with a check on the entity sending the birth message (instead of ignoring is it with
_). - Independently , we can also change the topic as proposed by @albinsuresh feat: flow enabled c8y mapper #3934 (comment)
There was a problem hiding this comment.
- This can made more obvious with a check on the entity sending the birth message (instead of ignoring is it with
_).
I think this is wise. It feels like a bit of a hack to rely on "we don't subscribe to other conflicting topics" to ensure we only respond to this mapper's messages.
There was a problem hiding this comment.
- This can made more obvious with a check on the entity sending the birth message (instead of ignoring is it with
_).
I think this is wise. It feels like a bit of a hack to rely on "we don't subscribe to other conflicting topics" to ensure we only respond to this mapper's messages.
Done: 61d7259
Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
This is a first step toward a mapper context populated by the mapper using a specific implementation (say to read the entity cache), while the context for a script or a flow is a simple key value store. Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
so the flow context can be populated by the mapper, notably with entity cache entries. Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
The goal is to group the methods acting on the content of the entity cache, putting apart all the business logic related to entity metadata. The next step will be to connect the EntityCache to the FlowContext, populating the latter with entity metadata. Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
Starting only with `Mqtt { qos: QoS, retain: bool }`,
the plan begin to add `Process { command: String }`
and, possibly, `Http { url: String }`
Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
preventing tedge flows to start. Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
reubenmiller
left a comment
There was a problem hiding this comment.
Approved. Everything works nicely and this allows users to also edit existing tedge-mapper-c8y logic (which was not possible before)
Proposed changes
The c8y mapper can be extended using flows.
c8y.topicssettings to process only configured MEA messagesTypes of changes
Paste Link to the issue
Checklist
just prepare-devonce)just formatas mentioned in CODING_GUIDELINESjust checkas mentioned in CODING_GUIDELINESFurther comments