-
Notifications
You must be signed in to change notification settings - Fork 30
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
Add logging to MQTT #68
Conversation
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 log statements are all there and in the right places, just need ids in a lot of them so they can be correlated, and a few fixups.
@@ -308,18 +333,21 @@ struct aws_mqtt_client_connection *aws_mqtt_client_connection_new(struct aws_mqt | |||
|
|||
if (aws_mutex_init(&connection->pending_requests.mutex)) { | |||
|
|||
goto handle_error; | |||
AWS_LOGF_ERROR(AWS_LS_MQTT_CLIENT, "Failed to initialize pending_requests mutex"); | |||
goto failed_init_pending_requests_mutex; |
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.
Given the ZERO_STRUCT call here, it seems like this could be cleaned up to have a single on_error that destroys the connection, which would simplify the logic a lot. Your destroy might need to be made a bit more paranoid in spots to make sure it handles the partial construction cases.
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.
When deciding what should be TRACE/DEBUG/INFO, I found it helpful to look at the output from running the same test at each of those 3 log levels. When you see everything together, it helps give a good feel for what belongs and what doesn't.
Take a look at the description on the HTTP-logging PR. I put what the logs look like at each level.
Would you mind if I harassed you to paste what the logs looked like at TRACE/DEBUG/INFO levels when running something simple like a pub/sub sample? connect, sub, pub, receive, disconnect |
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.
fix & ship
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.