-
Notifications
You must be signed in to change notification settings - Fork 11
Add collector mqtt exporter #417
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
base: master
Are you sure you want to change the base?
Conversation
dc33450 to
9709f0f
Compare
73728f1 to
ceb2ace
Compare
ceb2ace to
a851f99
Compare
nbuffon
left a comment
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.
The PR description should definitely tell what's to be tested related to the changes and not only redirect to the README
| let string_topic = self.topic.to_string(); | ||
| let parts = string_topic.split('/').collect::<Vec<&str>>(); | ||
| let mut new_parts = parts.clone(); | ||
| if level as usize <= parts.len() { |
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.
These might be included in StrTopic implementation
impl StrTopic {
pub fn parts() -> &[&str] {...}
pub fn len() { self.parts().len() }
}| } else { | ||
| let string_topic = self.topic.to_string(); | ||
| // Use the specified number of levels | ||
| let parts: Vec<&str> = string_topic.split('/').collect(); |
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.
Same here, we could use an upper level len() and/or parts() method(s) on StrTopic
| CollectorStrTopic::Level5(topic) => topic.as_route(), | ||
| CollectorStrTopic::Default(topic) => topic.as_route(), | ||
| } | ||
| } |
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.
This is a trait implementation, just put this code directly in the trait implementation
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 don't understand : it's an enum implementation for me :(
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.
as_route comes from Topic trait which is implemented for this enum later in this file by just calling this method.
What I mean is that you don't need this method as all its content belongs to the impl
impl Topic for CollectorStrTopic {
fn as_route() {
// here
}
}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.
@tigroo Last point remaining as explained yesterday do you want to push it now so I can approve this PR ?
|
Just to mention, the collector example code need Rust 1.90 to work, it does not build under 1.85 |
Simplifying to not build artificial sections just for call. Signed-off-by: Frédéric Gardes <[email protected]>
Signed-off-by: Frédéric Gardes <[email protected]>
|
Signed-off-by: Frédéric Gardes <[email protected]>
Depending on a ROUTE_LEVEL, the topic can be routed differently (and not statically as the default str topic). Signed-off-by: Frédéric Gardes <[email protected]>
Add a getter to a list. Signed-off-by: Frédéric Gardes <[email protected]>
Fix minor comments. Create a collector str topic allowing 5 levels of routed str topic. Signed-off-by: Frédéric Gardes <[email protected]>
Signed-off-by: Frédéric Gardes <[email protected]>
a851f99 to
3df7326
Compare
|
And in parallell, I try : #452 |
Changes
Close #331
Test
How to test