-
Notifications
You must be signed in to change notification settings - Fork 54
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 configurable tedge-mapper MQTT topics #2033
Support configurable tedge-mapper MQTT topics #2033
Conversation
Codecov Report
Additional details and impacted files
|
Robot Results
Passed Tests
|
db59319
to
9e80f45
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.
I have a question: what's the expected behavior of the mapper if the user adds a topic that is not a topic related to measurements/alarms/events?
99a9ece
to
d412801
Compare
tests/RobotFramework/tests/mqtt/custom_sub_topics_tedge-mapper-aws.robot
Outdated
Show resolved
Hide resolved
tests/RobotFramework/tests/mqtt/custom_sub_topics_tedge-mapper-az.robot
Outdated
Show resolved
Hide resolved
tests/RobotFramework/tests/mqtt/custom_sub_topics_tedge-mapper-c8y.robot
Outdated
Show resolved
Hide resolved
tests/RobotFramework/tests/mqtt/custom_sub_topics_tedge-mapper-az.robot
Outdated
Show resolved
Hide resolved
*** Test Cases *** | ||
Publish events to subscribed topic | ||
Execute Command mosquitto_pub -t tedge/events/event-type -m '{"text": "Event"}' | ||
Should Have MQTT Messages c8y/s/us |
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.
You have to be more specific with your comparison as there might be other messages that are published to c8y/s/us
which will cause the test to pass.
The Should Have MQTT Messages
supports a message pattern named argument, message_pattern
:
Should Have MQTT Messages c8y/s/us message_pattern=.*Event.*
Or something to that effect (not 100% sure on what the expected payload is in this case, I would have expected it to be published on the json via mqtt topic c8y/event/events
...but I can't remember the exact implementation for 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.
Actually, event is converted to SmartREST like below.
[c8y/s/us] 400,event-type,"Event",2023-06-28T17:30:21.529621713Z
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.
Though I’m sure there are other messages being sent to the same topic, so we have to be sure the assertion is specific enough.
tests/RobotFramework/tests/mqtt/custom_sub_topics_tedge-mapper-c8y.robot
Outdated
Show resolved
Hide resolved
The cloud-specific mappers subscribe to the reserved MQTT topics and convert incoming MQTT messages to cloud-specific messages. | ||
In an advanced use case, such as using more than one cloud mappers for the same device, | ||
you may want to customize the MQTT topics that each cloud mapper subscribes to. |
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.
Though doesn't this setting control which tedge topics are subscribe to by each mapper, and not the data coming from each cloud?
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.
True. For example, c8y/s/us
is not a candidate to be configured by tedge-mapper-c8y.
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.
Just some minor tweaks and improvements to the robot tests and some minor doc updates.
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 the code being correct. I let you address the changes requested by Reuben for docs and tests.
vec![ | ||
"tedge/measurements", | ||
"tedge/measurements/+", | ||
"tedge/health", | ||
"tedge/health/+", | ||
"tedge/events/+", | ||
"tedge/events/+/+", | ||
"tedge/alarms/+/+", | ||
"tedge/alarms/+/+/+", | ||
] |
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.
A bit annoying to see this topic list moved into the tedge-config crate as a default value while this is key for the behavior of this mapper. Unfortunately, I see no immediate way to define the list in this crate and use it as a default in tedge-config.
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
2356c6f
to
c4a00a1
Compare
c4a00a1
to
d61077c
Compare
d61077c
to
0926c94
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
Some users want c8y-mapper to subscribe selected topics. Introduce `c8y.topics` tedge config key to support such use-case. Change those topics to be configurable, and they are the default value of `c8y.topics`. - tedge/measurements - tedge/measurements/+ - tedge/alarms/+/+ - tedge/alarms/+/+/+ - tedge/events/+ - tedge/events/+/+ - tedge/health/+ - tedge/health/+/+ Signed-off-by: Rina Fujino <18257209+rina23q@users.noreply.github.com>
Some users want az-mapper to subscribe selected topics. Introduce `az.topics` tedge config key to support such use-case. Change those topics to be configurable, and they are the default value of `az.topics`. - tedge/measurements - tedge/measurements/+ Signed-off-by: Rina Fujino <18257209+rina23q@users.noreply.github.com>
Some users want aws-mapper to subscribe selected topics. Introduce `aws.topics` tedge config key to support such use-case. Change those topics to be configurable, and they are the default value of `aws.topics`. - tedge/measurements - tedge/measurements/+ - tedge/alarms/+/+ - tedge/alarms/+/+/+ - tedge/events/+ - tedge/events/+/+ - tedge/health - tedge/health/+ Signed-off-by: Rina Fujino <18257209+rina23q@users.noreply.github.com>
All cloud mappers subscribe to - tedge/health/+ - tedge/health/+/+ Signed-off-by: Rina Fujino <18257209+rina23q@users.noreply.github.com>
Signed-off-by: Rina Fujino <18257209+rina23q@users.noreply.github.com>
Signed-off-by: Rina Fujino <18257209+rina23q@users.noreply.github.com>
Signed-off-by: Rina Fujino <18257209+rina23q@users.noreply.github.com>
0926c94
to
e56f6e3
Compare
Proposed changes
This PR introduces 3 new tedge config keys:
The default values of them are the topics that currently the mapper subscribes. So, by default, user will find no difference.
If you want to subscribe limited topics, try something like this.
Then, c8y-mapper will subscribe the only configured topics.
Progress
Minor changes from the current implementation
tedge/health topic variants
Currently,
tedge/health/+
andtedge/health/+/+
.tedge/health
andtedge/health/+
Source:
c8y: https://github.com/thin-edge/thin-edge.io/blob/main/crates/extensions/c8y_mapper_ext/src/config.rs#L80-L81
az: https://github.com/thin-edge/thin-edge.io/blob/main/crates/extensions/az_mapper_ext/src/converter.rs#L51
aws: https://github.com/thin-edge/thin-edge.io/blob/main/crates/extensions/aws_mapper_ext/src/converter.rs#L42-L43
But this PR changes all to subscribe to
tedge/health/+
andtedge/health/+/+
by default.Types of changes
Paste Link to the issue
Part 2 of #1969
Checklist
cargo fmt
as mentioned in CODING_GUIDELINEScargo clippy
as mentioned in CODING_GUIDELINESFurther comments