)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"678d20323972a96a8838b72f343fad7f63994278","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"e87d7ab0_050eaa8e","updated":"2024-02-15 23:44:53.000000000","message":"I honestly can\u0027t review this because of all the pep8 comments - I\u0027m sure you were hoping to push up something quick and get early feedback thinking it\u0027d be easy to go and polish, but it turns out getting review bandwidth is hard and when you get it you don\u0027t want to waste it having a bunch of noise between the reviewer and the code you want feedback on.  I\u0027d suggest you invest in getting your editor to help you automatically write clean linting code on every save so any diff you push won\u0027t get all tagged up with grafitti by zuul.\n\nI think most of them are just \"start your inline comments with \u0027  #\u0027\" i.e. two spaces and the pound E261.  I think it also likes it if you have a space *after* the pound sign E262.  Maybe the take away is don\u0027t leave commented code lying around when you\u0027re trying to share a diff - it\u0027s in version control; red vs green is the best way to see what was there before.\n\nI think from what I can see I like this *idea* but I don\u0027t think it\u0027s in the right place to shine.  You need to move it up further before labeled metrics ever added the legacy format kwarg and re-wrote _send.\n\nWhat would be *really* swanky is if both this patch and 896968: stats: Allow emission of labeled metrics | https://review.opendev.org/c/openstack/swift/+/896968 wanted to *share* some implementation - and THAT got moved into a \"common\" ancestor that we eventually turn into \"how we want to do labeled metrics.  All the stuff that this change \"reverts\" from 896968 should stay in 896968 and everything from 896968 that it wants to keep should go in a precusor that both patches get rebased on.\n\nOne thing that would be really useful and help me would be moving everything out of swift/common/utils into swift/common/utils/stats and pull the statsd client tests out of test.unit.common.test_utils into a new test.unit.common.utils.test_stats or something like that.  the existing test.unit.common.test_utils is 10K lines and running my linter every save on that file takes like 8s 😜  A big \"move everything around but don\u0027t change it before we start working on it\" change could probably get MERGED if you put that at the front.","commit_id":"c5fcbc64518445de7f0cf11f4cf9096f0dc3c30f"}],"swift/common/utils/__init__.py":[{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"678d20323972a96a8838b72f343fad7f63994278","unresolved":true,"context_lines":[{"line_number":1547,"context_line":"        return line"},{"line_number":1548,"context_line":""},{"line_number":1549,"context_line":"    def _send(self, m_name, m_value, m_type, sample_rate):"},{"line_number":1550,"context_line":"              #labels\u003dNone, legacy_format\u003dNone"},{"line_number":1551,"context_line":"        \"\"\""},{"line_number":1552,"context_line":"        :returns: bytes sent; if not sent due to sample rate"},{"line_number":1553,"context_line":"        \"\"\""}],"source_content_type":"text/x-python","patch_set":2,"id":"c9aef23b_cff62614","line":1550,"updated":"2024-02-15 23:44:53.000000000","message":"this commented out stuff is really gross/distracting.  we\u0027re in version control - i\u0027m looking at a diff that\u0027s already trying to highlight the changes - i want the old code to show up RED and the new code in GREE.  Having old code in green but also commented is bad compromise.","commit_id":"c5fcbc64518445de7f0cf11f4cf9096f0dc3c30f"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"678d20323972a96a8838b72f343fad7f63994278","unresolved":true,"context_lines":[{"line_number":1549,"context_line":"    def _send(self, m_name, m_value, m_type, sample_rate):"},{"line_number":1550,"context_line":"              #labels\u003dNone, legacy_format\u003dNone"},{"line_number":1551,"context_line":"        \"\"\""},{"line_number":1552,"context_line":"        :returns: bytes sent; if not sent due to sample rate"},{"line_number":1553,"context_line":"        \"\"\""},{"line_number":1554,"context_line":"        if sample_rate is None:"},{"line_number":1555,"context_line":"            sample_rate \u003d self._default_sample_rate"}],"source_content_type":"text/x-python","patch_set":2,"id":"b34b82b3_d65dbcb1","line":1552,"updated":"2024-02-15 23:44:53.000000000","message":"this would be more clear as:\n\n    :returns: a tuple of \"bytes sent\" and \"dropped due to sample rate\" in the form of (int, bool)\n    N.B. when legacy metrics are disabled we always tell the same lie (0, False)","commit_id":"c5fcbc64518445de7f0cf11f4cf9096f0dc3c30f"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"678d20323972a96a8838b72f343fad7f63994278","unresolved":true,"context_lines":[{"line_number":1572,"context_line":"        bytes \u003d 0"},{"line_number":1573,"context_line":"        with closing(self._open_socket()) as sock:"},{"line_number":1574,"context_line":"            try:"},{"line_number":1575,"context_line":"                bytes \u003d sock.sendto(b\u0027|\u0027.join(parts), self._target)"},{"line_number":1576,"context_line":"            except IOError as err:"},{"line_number":1577,"context_line":"                if self.logger:"},{"line_number":1578,"context_line":"                    self.logger.warning("}],"source_content_type":"text/x-python","patch_set":2,"id":"25160507_0f893a94","line":1575,"updated":"2024-02-15 23:44:53.000000000","message":"this part actually looks a lot like master; I think the ideal way to show this patch would be at the same level as 896968: stats: Allow emission of labeled metrics | https://review.opendev.org/c/openstack/swift/+/896968 - a true alternative with it\u0027s own follow up \"*ADD* labeled metrics to proxy-logging\" as opposed to 896969: *MOVE* proxy-logging to labeled metrics | https://review.opendev.org/c/openstack/swift/+/896969\n\nmy guess is this approach will be much less disruptive to both common/utils *and* proxy-logging.","commit_id":"c5fcbc64518445de7f0cf11f4cf9096f0dc3c30f"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"678d20323972a96a8838b72f343fad7f63994278","unresolved":true,"context_lines":[{"line_number":1578,"context_line":"                    self.logger.warning("},{"line_number":1579,"context_line":"                        \u0027Error sending UDP message to %(target)r: %(err)s\u0027,"},{"line_number":1580,"context_line":"                        {\u0027target\u0027: self._target, \u0027err\u0027: err})"},{"line_number":1581,"context_line":"        return bytes, False"},{"line_number":1582,"context_line":""},{"line_number":1583,"context_line":"    def _send_tag(self, m_name, m_value, m_type, sample_rate,"},{"line_number":1584,"context_line":"                  labels):"}],"source_content_type":"text/x-python","patch_set":2,"id":"e2103b5d_70658302","line":1581,"updated":"2024-02-15 23:44:53.000000000","message":"is the tuple return stuff *new* - I don\u0027t see it in the original patch.  Is a drive-by improvement?  Is it needed for testing?","commit_id":"c5fcbc64518445de7f0cf11f4cf9096f0dc3c30f"}],"test/unit/common/test_utils.py":[{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"678d20323972a96a8838b72f343fad7f63994278","unresolved":true,"context_lines":[{"line_number":5547,"context_line":"        while not got:"},{"line_number":5548,"context_line":"            bytes, retrySample \u003d sender_fn(*args, **kwargs)"},{"line_number":5549,"context_line":"            if not bytes and not retrySample:"},{"line_number":5550,"context_line":"                break"},{"line_number":5551,"context_line":"            try:"},{"line_number":5552,"context_line":"                while True:"},{"line_number":5553,"context_line":"                    got.append(self.queue.get(timeout\u003d0.1))"}],"source_content_type":"text/x-python","patch_set":2,"id":"558cd41e_a701d5ab","line":5550,"updated":"2024-02-15 23:44:53.000000000","message":"is this *just* an early exit optimization?  It seems like this code used to want the caller to keep sending metrics until the sample rate got hit; how we just return with `got \u003d []`\n\nis that significant?","commit_id":"c5fcbc64518445de7f0cf11f4cf9096f0dc3c30f"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"678d20323972a96a8838b72f343fad7f63994278","unresolved":true,"context_lines":[{"line_number":5559,"context_line":"            got \u003d got[0]"},{"line_number":5560,"context_line":"        return got"},{"line_number":5561,"context_line":""},{"line_number":5562,"context_line":"    def assertStat(self, expected, sender_fn, *args, **kwargs):"},{"line_number":5563,"context_line":"        got \u003d self._send_and_get(sender_fn, *args, **kwargs)"},{"line_number":5564,"context_line":"        return self.assertEqual(expected, got)"},{"line_number":5565,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"0e39da6c_3606607e","line":5562,"updated":"2024-02-15 23:44:53.000000000","message":"ok, so the signature for this helper is:\n\n    expected (typically a single string, the stat line on the wire)\n    sender_fn (the logger.increment call or whatever)\n    *args, **kwargs (passed to the sender_fn)\n\nso you\u0027d expect to see\n\n    assertStat(\u0027some.funky:100|c\u0027, logger.increment, \u0027some.funky\u0027, 100)\n\nor something like that...","commit_id":"c5fcbc64518445de7f0cf11f4cf9096f0dc3c30f"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"678d20323972a96a8838b72f343fad7f63994278","unresolved":true,"context_lines":[{"line_number":5788,"context_line":"            self.assertStat([], #\u0027my_prefix.pfx.some.counter:-1|c\u0027,"},{"line_number":5789,"context_line":"                            self.logger.decrement, *inc_dec_args, labels\u003dlabels)"},{"line_number":5790,"context_line":"                            #legacy_format\u003d\u0027{action}.counter\u0027"},{"line_number":5791,"context_line":"            self.assertStat([], #\u0027my_prefix.pfx.some.timing:6280.0|ms\u0027,"},{"line_number":5792,"context_line":"                            self.logger.timing, *timing_args, labels\u003dlabels)"},{"line_number":5793,"context_line":"                            #legacy_format\u003d\u0027{action}.timing\u0027"},{"line_number":5794,"context_line":"            self.assertStat([], #\u0027my_prefix.pfx.some.stat:3|c\u0027,"}],"source_content_type":"text/x-python","patch_set":2,"id":"4c003574_30a0e14c","line":5791,"in_reply_to":"6aae4472_c1ac4654","updated":"2024-02-15 23:44:53.000000000","message":"\u003e pep8: E116 unexpected indentation (comment)\n\nPlease fix.  These pep8 comments are really distracting to reviewers.","commit_id":"c5fcbc64518445de7f0cf11f4cf9096f0dc3c30f"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"678d20323972a96a8838b72f343fad7f63994278","unresolved":true,"context_lines":[{"line_number":5790,"context_line":"                            #legacy_format\u003d\u0027{action}.counter\u0027"},{"line_number":5791,"context_line":"            self.assertStat([], #\u0027my_prefix.pfx.some.timing:6280.0|ms\u0027,"},{"line_number":5792,"context_line":"                            self.logger.timing, *timing_args, labels\u003dlabels)"},{"line_number":5793,"context_line":"                            #legacy_format\u003d\u0027{action}.timing\u0027"},{"line_number":5794,"context_line":"            self.assertStat([], #\u0027my_prefix.pfx.some.stat:3|c\u0027,"},{"line_number":5795,"context_line":"                            self.logger.update_stats, *update_args, labels\u003dlabels)"},{"line_number":5796,"context_line":"                            #legacy_format\u003d\u0027{action}.stat\u0027"}],"source_content_type":"text/x-python","patch_set":2,"id":"ca854374_e9255d99","line":5793,"updated":"2024-02-15 23:44:53.000000000","message":"again with the comments; makes it harder for a review to see through to the heart of the change - i\u0027m still trying to understand why retrySample is about.","commit_id":"c5fcbc64518445de7f0cf11f4cf9096f0dc3c30f"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"678d20323972a96a8838b72f343fad7f63994278","unresolved":true,"context_lines":[{"line_number":5857,"context_line":"                        self.logger.update_stats, \u0027some.stat\u0027, 3)"},{"line_number":5858,"context_line":"        self.assertStat("},{"line_number":5859,"context_line":"            \u0027my_prefix_the_counter;action\u003dsome;log_name\u003dpfx;result\u003dok:1|c\u0027,"},{"line_number":5860,"context_line":"            self.logger.increment, *inc_dec_args, labels\u003dlabels)"},{"line_number":5861,"context_line":"            #legacy_format\u003d\u0027{action}.counter\u0027"},{"line_number":5862,"context_line":"        self.assertStat("},{"line_number":5863,"context_line":"            \u0027my_prefix_the_counter;action\u003dsome;log_name\u003dpfx;result\u003dok:-1|c\u0027,"}],"source_content_type":"text/x-python","patch_set":2,"id":"147f4f08_1eda6477","line":5860,"updated":"2024-02-15 23:44:53.000000000","message":"this is kind of interesting; we\u0027re not *just* adding labels to the existing call; we\u0027re also changing the inc_dec_args to be \"the_counter\" instead of \"some.counter\"\n\nalso \"my_prefix\" shows up \"log_statsd_metric_prefix\" but the \"pfx\" bit still goes in log_name; I don\u0027t understand if that\u0027s desirable or just a side-effect of reverting the changes in the _send method","commit_id":"c5fcbc64518445de7f0cf11f4cf9096f0dc3c30f"}]}