-
Notifications
You must be signed in to change notification settings - Fork 55
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
Attach topic to c8y message conversion error #3217
Attach topic to c8y message conversion error #3217
Conversation
Robot Results
|
86261fc
to
6d04337
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files📢 Thoughts on this report? Let us know! |
let error = MessageConversionError { | ||
error, | ||
topic: input.topic.name.clone(), | ||
}; |
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.
How about moving this MessageConversionError
creation logic into the convert
function and propagate the same type into wrap_errors
and new_error_message
functions as well, since the new_error_message
function already has the logic below that creates the message on the error topic. That
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.
wrap_errors
is also used in other places where we don't have a message we're converting, for example auto entity registration. Then using the topic of a message that causes auto-registration (for example some measurement) next to any errors about registration could cause confusion
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.
Okay, makes sense.
In that case, we can just change the signature of the new_error_message
function to accept a std::error::Error
instead of ConversionError
so that this whole function can be shortened as:
message_or_err
.map_err(
|error| MessageConversionError {
error,
topic: input.topic.name.clone(),
})
.unwrap_or_else(|error| self.new_error_message(error))
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.
Ah, you're right, so I implemented your suggestion.
let error = MessageConversionError { | ||
error, | ||
topic: input.topic.name.clone(), | ||
}; |
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.
Okay, makes sense.
In that case, we can just change the signature of the new_error_message
function to accept a std::error::Error
instead of ConversionError
so that this whole function can be shortened as:
message_or_err
.map_err(
|error| MessageConversionError {
error,
topic: input.topic.name.clone(),
})
.unwrap_or_else(|error| self.new_error_message(error))
Improve error reporting in c8y mapper by printing the topic when there was an error converting a message on that topic. Example: when publishing an invalid command metadata message: `$ tedge mqtt pub -r te/device/main///cmd/config_snapshot 'a'` before: `ERROR: Mapping error: expected value at line 1 column 1` after: `ERROR: Mapping error: Failed to convert a message on topic 'te/device/main///cmd/config_snapshot': expected value at line 1 column 1` Signed-off-by: Marcel Guzik <[email protected]>
6d04337
to
443ec28
Compare
Proposed changes
Improve error reporting in c8y mapper by printing the topic when there was an error converting a message on that topic.
Example: when publishing an invalid command metadata message:
$ tedge mqtt pub -r te/device/main///cmd/config_snapshot 'a'
before:
ERROR: Mapping error: expected value at line 1 column 1
after:
ERROR: Mapping error: Failed to convert a message on topic 'te/device/main///cmd/config_snapshot': expected value at line 1 column 1
Types of changes
Paste Link to the issue
Checklist
cargo fmt
as mentioned in CODING_GUIDELINEScargo clippy
as mentioned in CODING_GUIDELINESFurther comments