)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":5202,"name":"Erno Kuvaja","email":"jokke@usr.fi","username":"jokke"},"change_message_id":"8464dc789436bc40d32d82eb8b1f566837d7b7b4","unresolved":true,"context_lines":[{"line_number":4,"context_line":"Commit:     Jaromír Wysoglad \u003cjwysogla@redhat.com\u003e"},{"line_number":5,"context_line":"CommitDate: 2023-07-12 13:48:28 +0000"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Make TCP publisher log warning instead of failing."},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"Change-Id: I56ca759c2d43830f4e342130f27b75bdba65c469"},{"line_number":10,"context_line":"(cherry picked from commit 8699146e1aee5bbd5b65a96c4109b6c02b0e1fe1)"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"af4af06a_6a4d4db6","line":7,"range":{"start_line":7,"start_character":23,"end_line":7,"end_character":30},"updated":"2023-08-03 08:41:43.000000000","message":"NIT: The code logs error, not warning.","commit_id":"0bf3991de191ab25ab2b99835bda517805517c17"},{"author":{"_account_id":34975,"name":"Jaromír Wysoglad","email":"jwysogla@redhat.com","username":"jwysogla"},"change_message_id":"73483bde93d510d5e7d63042b8c548a8b98e2bc5","unresolved":false,"context_lines":[{"line_number":4,"context_line":"Commit:     Jaromír Wysoglad \u003cjwysogla@redhat.com\u003e"},{"line_number":5,"context_line":"CommitDate: 2023-07-12 13:48:28 +0000"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"Make TCP publisher log warning instead of failing."},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"Change-Id: I56ca759c2d43830f4e342130f27b75bdba65c469"},{"line_number":10,"context_line":"(cherry picked from commit 8699146e1aee5bbd5b65a96c4109b6c02b0e1fe1)"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"a12eb499_bd558240","line":7,"range":{"start_line":7,"start_character":23,"end_line":7,"end_character":30},"in_reply_to":"af4af06a_6a4d4db6","updated":"2023-08-03 09:30:20.000000000","message":"Ack","commit_id":"0bf3991de191ab25ab2b99835bda517805517c17"}],"/PATCHSET_LEVEL":[{"author":{"_account_id":5202,"name":"Erno Kuvaja","email":"jokke@usr.fi","username":"jokke"},"change_message_id":"605f4bc9956f971861cb9320f540957624d3efcf","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":1,"id":"dbbae388_22fe7864","updated":"2023-08-03 08:10:31.000000000","message":"As per https://docs.openstack.org/project-team-guide/stable-branches.html#appropriate-fixes I think this is appropriate fix to backport, but we really would need a bug for it. \n\nAs this change effectively ignores the initial connection failure I would love to see test covering the case where we do fail to create the self.socket rather than fail to connect as I\u0027m expecting that failure not being obvious as all the way through the retry we catch everything rather than socket specific exceptions and log very generic errors.","commit_id":"0bf3991de191ab25ab2b99835bda517805517c17"},{"author":{"_account_id":4264,"name":"Matthias Runge","email":"mrunge@redhat.com","username":"mrunge"},"change_message_id":"b2aad308734f752df82a9ebc378d8726dab128ba","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"e2dec75f_5f624e7c","updated":"2023-08-07 06:09:09.000000000","message":"Thank you for the reviews. I agree with you Erno on the topic. Since this is a cherry-pick, I would ask Jaromir to submit a follow-up patch to master.","commit_id":"0bf3991de191ab25ab2b99835bda517805517c17"},{"author":{"_account_id":34975,"name":"Jaromír Wysoglad","email":"jwysogla@redhat.com","username":"jwysogla"},"change_message_id":"69df1c867fc3047ddfe2b9c323634af95515659b","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"45a14652_3bb32d71","updated":"2023-07-20 05:59:50.000000000","message":"recheck","commit_id":"0bf3991de191ab25ab2b99835bda517805517c17"},{"author":{"_account_id":34975,"name":"Jaromír Wysoglad","email":"jwysogla@redhat.com","username":"jwysogla"},"change_message_id":"73483bde93d510d5e7d63042b8c548a8b98e2bc5","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":1,"id":"fd042d32_43bcd11c","in_reply_to":"dbbae388_22fe7864","updated":"2023-08-03 09:30:20.000000000","message":"Before submitting this backport, I forgot this patch in master existed and did some investigation of this (again). And I wrote a test case for this, which I planed to submit after these backports are merged. If this was master, I\u0027d just add it here.","commit_id":"0bf3991de191ab25ab2b99835bda517805517c17"}],"ceilometer/publisher/tcp.py":[{"author":{"_account_id":5202,"name":"Erno Kuvaja","email":"jokke@usr.fi","username":"jokke"},"change_message_id":"605f4bc9956f971861cb9320f540957624d3efcf","unresolved":true,"context_lines":[{"line_number":53,"context_line":"            self.addr_family \u003d socket.AF_INET"},{"line_number":54,"context_line":"        try:"},{"line_number":55,"context_line":"            self.create_and_connect()"},{"line_number":56,"context_line":"        except Exception:"},{"line_number":57,"context_line":"            LOG.error(_(\"Unable to connect to the \""},{"line_number":58,"context_line":"                      \"remote endpoint\"))"},{"line_number":59,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"118474e8_9fff5227","line":56,"range":{"start_line":56,"start_character":15,"end_line":56,"end_character":24},"updated":"2023-08-03 08:10:31.000000000","message":"In general not fond of umbrella catch of \"Exception\" especially as socket has pretty concise set of exceptions it raises which could be used to provide valuable information in the logging.\n\nAs this is backport and the master had been already approved and it seems to be the trend in this module anyways I don\u0027t see it as blocker for merging this.","commit_id":"0bf3991de191ab25ab2b99835bda517805517c17"},{"author":{"_account_id":34975,"name":"Jaromír Wysoglad","email":"jwysogla@redhat.com","username":"jwysogla"},"change_message_id":"73483bde93d510d5e7d63042b8c548a8b98e2bc5","unresolved":true,"context_lines":[{"line_number":53,"context_line":"            self.addr_family \u003d socket.AF_INET"},{"line_number":54,"context_line":"        try:"},{"line_number":55,"context_line":"            self.create_and_connect()"},{"line_number":56,"context_line":"        except Exception:"},{"line_number":57,"context_line":"            LOG.error(_(\"Unable to connect to the \""},{"line_number":58,"context_line":"                      \"remote endpoint\"))"},{"line_number":59,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"5b536626_49bc645a","line":56,"range":{"start_line":56,"start_character":15,"end_line":56,"end_character":24},"in_reply_to":"118474e8_9fff5227","updated":"2023-08-03 09:30:20.000000000","message":"I agree. This kind of exception handling wasn\u0027t very good.","commit_id":"0bf3991de191ab25ab2b99835bda517805517c17"},{"author":{"_account_id":5202,"name":"Erno Kuvaja","email":"jokke@usr.fi","username":"jokke"},"change_message_id":"605f4bc9956f971861cb9320f540957624d3efcf","unresolved":true,"context_lines":[{"line_number":81,"context_line":"            try:"},{"line_number":82,"context_line":"                self.socket.send(msg_len + encoded_msg)"},{"line_number":83,"context_line":"            except Exception:"},{"line_number":84,"context_line":"                LOG.error(_(\"Unable to send sample over TCP,\""},{"line_number":85,"context_line":"                          \"trying to reconnect and resend the message\"))"},{"line_number":86,"context_line":"                self.create_and_connect()"},{"line_number":87,"context_line":"                try:"}],"source_content_type":"text/x-python","patch_set":1,"id":"c73e40f9_1c7524ee","line":84,"range":{"start_line":84,"start_character":20,"end_line":84,"end_character":25},"updated":"2023-08-03 08:10:31.000000000","message":"I\u0027m not convinced changing the logging level in stable backport is the right thing to do, but will leave that to ceilometer stable cores to decide. Especially as this is not hard error condition. As we are retrying to establish the socket connection and the following failure gets logged at high level.","commit_id":"0bf3991de191ab25ab2b99835bda517805517c17"},{"author":{"_account_id":34975,"name":"Jaromír Wysoglad","email":"jwysogla@redhat.com","username":"jwysogla"},"change_message_id":"73483bde93d510d5e7d63042b8c548a8b98e2bc5","unresolved":true,"context_lines":[{"line_number":81,"context_line":"            try:"},{"line_number":82,"context_line":"                self.socket.send(msg_len + encoded_msg)"},{"line_number":83,"context_line":"            except Exception:"},{"line_number":84,"context_line":"                LOG.error(_(\"Unable to send sample over TCP,\""},{"line_number":85,"context_line":"                          \"trying to reconnect and resend the message\"))"},{"line_number":86,"context_line":"                self.create_and_connect()"},{"line_number":87,"context_line":"                try:"}],"source_content_type":"text/x-python","patch_set":1,"id":"c9cb99ae_ee59f104","line":84,"range":{"start_line":84,"start_character":20,"end_line":84,"end_character":25},"in_reply_to":"c73e40f9_1c7524ee","updated":"2023-08-03 09:30:20.000000000","message":"I was pretty undecided on which level to use (either warning or error). When I submitted this to master, I was asked to change this to error, because the inability to connect should attract more attention than just a warning. In addition, an error unifies the log level with the error from the constructor.","commit_id":"0bf3991de191ab25ab2b99835bda517805517c17"},{"author":{"_account_id":5202,"name":"Erno Kuvaja","email":"jokke@usr.fi","username":"jokke"},"change_message_id":"c1d2a206ad48b2e4aa0f4acd2190aa3be512c462","unresolved":true,"context_lines":[{"line_number":81,"context_line":"            try:"},{"line_number":82,"context_line":"                self.socket.send(msg_len + encoded_msg)"},{"line_number":83,"context_line":"            except Exception:"},{"line_number":84,"context_line":"                LOG.error(_(\"Unable to send sample over TCP,\""},{"line_number":85,"context_line":"                          \"trying to reconnect and resend the message\"))"},{"line_number":86,"context_line":"                self.create_and_connect()"},{"line_number":87,"context_line":"                try:"}],"source_content_type":"text/x-python","patch_set":1,"id":"b94a055e_eb65c6dd","line":84,"range":{"start_line":84,"start_character":20,"end_line":84,"end_character":25},"in_reply_to":"c9cb99ae_ee59f104","updated":"2023-08-03 09:59:35.000000000","message":"I hope this is useful for the future:\n\nhttps://docs.openstack.org/oslo.log/2023.1/user/guidelines.html#definition-of-log-levels\n\nI know the guidelines are still bit vague but the main difference between warning and error is somewhere around the lines of error being message that should be investigated by admin right away as operation failed, when it\u0027s something trivial like \"socket closed in between\" and we already know to retry it with high likelyhood of success, it should be logged as warning (in this case specifically as we do log the retry failure as log.exception which is error level logging with stack trace). These kind of situations happen and there\u0027s been even arguments that something trivial that the service self corrects with retry could even be info level log, but I at least personally think warn is appropriate as info is generally treated as success and warning as potential issue.\n\nThe exception logging with stack trace should be pretty exceptional and seeing such in logs in general should be treated as something warranting a bug to be filed, in this specific piece of code it seems like something that should be replaced with catching specific exceptions and logging useful message to the operator.","commit_id":"0bf3991de191ab25ab2b99835bda517805517c17"}]}