tedge service init session error message is not verbose enough #1912
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
PradeepKiruvale
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
Comment on lines
43
to
48
| eprintln!("Connection Error {}", err); | ||
| if err.to_string().contains("Connection refused") { | ||
| eprintln!("Couldn't connect to mqtt broker due to {}", err); | ||
| } else { | ||
| eprintln!("Connection Error {}", err); | ||
| } |
There was a problem hiding this comment.
As you are doing nothing specific to change the error message, why not simply:
eprintln!("Couldn't connect to mqtt broker due to {}", err);
Did you try to translate the error message using the actual message error? This will be less fragile and possibly more helpful. See:
There was a problem hiding this comment.
I went with this because I just wanted to address the mqtt connection issue when it refused. I feel now it makes sense to just have eprintln!("Couldn't connect to mqtt broker due to {}", err); for all the cases.
There was a problem hiding this comment.
If there is no mqtt broker during --init then the error thrown is rumqttc::ConnectionError::Io(_). So, the ConnectReturnCode is not useful.
There was a problem hiding this comment.
Though since we will be supported authenticated mqtt brokers in the near future, we need to also handle the ConnectReturnCode style errors as well. For example if the broker requires authentication, then it is useful to know if the password is wrong or not, or if it is a protocol version issue etc. These kinds of errors are covered by ConnectReturnCode (as mentioned by @didier-wenzek)
There was a problem hiding this comment.
Updated the error messages to use the ConnectReturnCode.
PradeepKiruvale
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
rina23q
reviewed
Apr 13, 2023
There was a problem hiding this comment.
I believe just changing the message to less scary way like in this PR is the right direction like I suggested in #1778 (comment), however, we need to a feedback from @reubenmiller!
I already made some investigation how hard to address error patterns.
#1778 (comment)
PradeepKiruvale
force-pushed
the
init_message
branch
from
50b8139
to
3e6a431
Compare
PradeepKiruvale
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
Robot Results
Passed Tests
|
didier-wenzek
requested changes
Apr 26, 2023
| @@ -16,6 +16,7 @@ futures = "0.3" | |||
| rumqttc = "0.18" | |||
| thiserror = "1.0" | |||
| tokio = { version = "1.23", features = ["rt", "time"] } | |||
| tracing = "0.1.37" | |||
There was a problem hiding this comment.
Remove this dependency.
Comment on lines
43
to
48
| eprintln!("Connection Error {}", err); | ||
| match err { | ||
| rumqttc::ConnectionError::ConnectionRefused(ConnectReturnCode::BadClientId) => { | ||
| warn!("Failed to initialize the session with MQTT Broker due to bad client id"); | ||
| } |
There was a problem hiding this comment.
The init function is returning a Result<(), MqttError>, so it's weird to log the errors and to return ().
These error messages are a real improvement but they must be captured by an Error type.
PradeepKiruvale
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
PradeepKiruvale
force-pushed
the
init_message
branch
from
f5ecf13
to
7c4cf3a
Compare
PradeepKiruvale
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
Comment on lines
43
to
48
| Err(err) => match err { | ||
| rumqttc::ConnectionError::ConnectionRefused(ConnectReturnCode::BadClientId) => { | ||
| return Err(MqttError::InitSessionError { | ||
| reason: "bad client id".to_string(), | ||
| }); | ||
| } | ||
| rumqttc::ConnectionError::ConnectionRefused( | ||
| ConnectReturnCode::BadUserNamePassword, | ||
| ) => { | ||
| return Err(MqttError::InitSessionError { | ||
| reason: "bad user name and password".to_string(), | ||
| }); | ||
| } | ||
| rumqttc::ConnectionError::ConnectionRefused(ConnectReturnCode::NotAuthorized) => { | ||
| return Err(MqttError::InitSessionError { | ||
| reason: " not authorized".to_string(), | ||
| }); | ||
| } | ||
| rumqttc::ConnectionError::ConnectionRefused( | ||
| ConnectReturnCode::RefusedProtocolVersion, | ||
| ) => { | ||
| return Err(MqttError::InitSessionError { | ||
| reason: " protocol version mismatch".to_string(), | ||
| }); | ||
| } | ||
| rumqttc::ConnectionError::ConnectionRefused( | ||
| ConnectReturnCode::ServiceUnavailable, | ||
| ) => { | ||
| return Err(MqttError::InitSessionError { | ||
| reason: " service not available".to_string(), | ||
| }); | ||
| } | ||
| rumqttc::ConnectionError::ConnectionRefused(ConnectReturnCode::Success) => {} | ||
| e => { | ||
| return Err(MqttError::InitSessionError { | ||
| reason: e.to_string(), | ||
| }); | ||
| } | ||
| }, |
There was a problem hiding this comment.
Nice. I would only move this code into an impl From<rumqttc::ConnectionError> for MqttError>.
There was a problem hiding this comment.
Refactored this code introducing a function to convert/map the errors.
PradeepKiruvale
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
| @@ -90,3 +94,37 @@ impl From<futures::channel::mpsc::SendError> for MqttError { | |||
| MqttError::SendOnClosedConnection | |||
| } | |||
| } | |||
|
|
|||
| pub fn from_connection_error(err: rumqttc::ConnectionError) -> Result<(), MqttError> { | |||
There was a problem hiding this comment.
I would wrap this as an MqttError function so this will be invoked as MqttError::from_connection_error (otherwise the name sounds incomplete: what is built from the connection error?).
The type signature is also unusual for a from function. I read this as building a unit value with conversion error that are MqttError. While what you meant is building an MqttError.
Suggested change
| pub fn from_connection_error(err: rumqttc::ConnectionError) -> Result<(), MqttError> { | |
| impl MqttError { | |
| pub fn from_connection_error(err: rumqttc::ConnectionError) -> MqttError { |
I agree that there is a wart to address. This is the case of ConnectionError::ConnectionRefused(ConnectReturnCode::Success). I would simply treat this case as an error in the from_connection_error function and check this specific case in the init_session function.
There was a problem hiding this comment.
I had to return the Result in an error conversion function because of the `Success code'. Now I refactored as suggested above.
PradeepKiruvale
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
Signed-off-by: Pradeep Kumar K J <pradeepkumar.kj@softwareag.com>
PradeepKiruvale
force-pushed
the
init_message
branch
2 times, most recently
from
a284020
to
4fee69d
Compare
PradeepKiruvale
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
Signed-off-by: Pradeep Kumar K J <pradeepkumar.kj@softwareag.com>
PradeepKiruvale
force-pushed
the
init_message
branch
from
4fee69d
to
bdfa91f
Compare
PradeepKiruvale
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
PradeepKiruvale
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
|
Tested and verified |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Proposed changes
Update the log message that is published during init session (
--init) if there is nomosquittobroker running.For example
Types of changes
Paste Link to the issue
#1778
Checklist
cargo fmtas mentioned in CODING_GUIDELINEScargo clippyas mentioned in CODING_GUIDELINESFurther comments