)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"2d895e51b9b3454e7281bf6fd38d0808875c80be","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"f38386cc_be5c1c6e","updated":"2025-04-07 14:08:51.000000000","message":"Thanks for the docstrings and cleanup.\n\nTest failure: There\u0027s some ugly stat aggregation in FakeStatsdClient that relied on the increments calling update_stats :/\n\nI think this fixes the replicator test:\n\n```\ndiff --git a/swift/common/statsd_client.py b/swift/common/statsd_client.py\nindex c551f4d97..d9f91bac5 100644\n--- a/swift/common/statsd_client.py\n+++ b/swift/common/statsd_client.py\n@@ -645,4 +645,4 @@ class LabeledStatsdClient(AbstractStatsdClient):\n         \"\"\"\n         return super().transfer_rate(metric, elapsed_time, byte_xfer,\n                                      labels\u003dlabels,\n-                                     sample_rate\u003dsample_rate)\n\\ No newline at end of file\n+                                     sample_rate\u003dsample_rate)\ndiff --git a/test/debug_logger.py b/test/debug_logger.py\nindex d92160ab3..191e95a7c 100644\n--- a/test/debug_logger.py\n+++ b/test/debug_logger.py\n@@ -93,6 +93,16 @@ class BaseFakeStatsdClient:\n         # convert to normal dict for better failure messages\n         return dict(counts)\n\n+    def get_decrement_counts(self):\n+        # note: this method reports the sum of stats sent via the decrement\n+        # method only; consider using get_stats_counts instead to get the sum\n+        # of stats sent via both the decrement and update_stats methods\n+        counts \u003d defaultdict(int)\n+        for call in self.calls[\u0027decrement\u0027]:\n+            counts[call[0][0]] +\u003d 1\n+        # convert to normal dict for better failure messages\n+        return dict(counts)\n+\n     def get_update_stats(self):\n         return [call[0][:2] for call in self.calls[\u0027update_stats\u0027]]\n\n@@ -100,6 +110,10 @@ class BaseFakeStatsdClient:\n         counts \u003d defaultdict(int)\n         for metric, step in self.get_update_stats():\n             counts[metric] +\u003d step\n+        for metric, step in self.get_increment_counts().items():\n+            counts[metric] +\u003d step\n+        for metric, step in self.get_decrement_counts().items():\n+            counts[metric] -\u003d step\n         # convert to normal dict for better failure messages\n         return dict(counts)\n```","commit_id":"c02ce09a46fdada196b7ca8d33e9209deddf6c78"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"6b510e602704a9bf0960b062396ce35df29b09fc","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"374fcf1e_ee969f24","updated":"2025-04-08 12:20:56.000000000","message":"This is great and the test helper changes provide more clarity of stats assertions. I only voted -1 because I question documenting the code alternative in BaseFakeStatstdClient. It doesn\u0027t seem necessary and not a practice I\u0027d want to encourage in general. Maybe I missed the reason?\n\nI\u0027d quite like to get rid of get_increment_counts() : https://review.opendev.org/c/openstack/swift/+/946621","commit_id":"acfed0da0e1c1c6e2c38e54abf88b27a2fcee710"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"96628b88c4bc10aa992ffd3402a9161d74735488","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"b0733a96_294199ff","updated":"2025-04-07 19:13:01.000000000","message":"everything turns into more than you hoped; there\u0027s a lot of distractions going on in this changed that have everything to do with test faking helpers and nothing to do with re-organization of the code we\u0027re testing for better doc-strings.","commit_id":"acfed0da0e1c1c6e2c38e54abf88b27a2fcee710"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"b9bd4202357fee91789e13551cb37576e914111d","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":5,"id":"283c5792_4be6df52","updated":"2025-04-09 00:51:30.000000000","message":"I think i addressed the outstanding issues.","commit_id":"563c1ca6564ac94c1fe85ceb45bf7b8585fe3fbb"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"b8bce895dd89146bab1207e3fe3274b9a8fdae71","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":5,"id":"546789b2_15bbe96f","updated":"2025-04-09 09:26:39.000000000","message":"LGTM","commit_id":"563c1ca6564ac94c1fe85ceb45bf7b8585fe3fbb"}],"swift/common/statsd_client.py":[{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"2d895e51b9b3454e7281bf6fd38d0808875c80be","unresolved":true,"context_lines":[{"line_number":316,"context_line":""},{"line_number":317,"context_line":"    def _update_stats(self, metric, value, **kwargs):"},{"line_number":318,"context_line":"        # This method was added to disagregate *crement metrics when testing"},{"line_number":319,"context_line":"        return self._send(metric, value, \u0027c\u0027, **kwargs)"},{"line_number":320,"context_line":""},{"line_number":321,"context_line":"    def update_stats(self, metric, value, **kwargs):"},{"line_number":322,"context_line":"        self._update_stats(metric, value, **kwargs)"}],"source_content_type":"text/x-python","patch_set":3,"id":"9424b5f5_2ec9249e","line":319,"updated":"2025-04-07 14:08:51.000000000","message":"good call","commit_id":"c02ce09a46fdada196b7ca8d33e9209deddf6c78"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"96628b88c4bc10aa992ffd3402a9161d74735488","unresolved":false,"context_lines":[{"line_number":316,"context_line":""},{"line_number":317,"context_line":"    def _update_stats(self, metric, value, **kwargs):"},{"line_number":318,"context_line":"        # This method was added to disagregate *crement metrics when testing"},{"line_number":319,"context_line":"        return self._send(metric, value, \u0027c\u0027, **kwargs)"},{"line_number":320,"context_line":""},{"line_number":321,"context_line":"    def update_stats(self, metric, value, **kwargs):"},{"line_number":322,"context_line":"        self._update_stats(metric, value, **kwargs)"}],"source_content_type":"text/x-python","patch_set":3,"id":"fadf7f36_b00c1a08","line":319,"in_reply_to":"9424b5f5_2ec9249e","updated":"2025-04-07 19:13:01.000000000","message":"Acknowledged","commit_id":"c02ce09a46fdada196b7ca8d33e9209deddf6c78"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"2d895e51b9b3454e7281bf6fd38d0808875c80be","unresolved":true,"context_lines":[{"line_number":491,"context_line":"        Update a timing metric, aggregated percentiles recalculated."},{"line_number":492,"context_line":""},{"line_number":493,"context_line":"        This is a timing metric, but adjusts the timing data per kB transferred"},{"line_number":494,"context_line":"        (ms/kB) for each non-zero-byte update.  Allegedly this could be used to"},{"line_number":495,"context_line":"        monitor problematic devices, where higher is bad."},{"line_number":496,"context_line":""},{"line_number":497,"context_line":"        :param metric: name of the metric"}],"source_content_type":"text/x-python","patch_set":3,"id":"bceb7974_279bddf1","line":494,"range":{"start_line":494,"start_character":9,"end_line":494,"end_character":14},"updated":"2025-04-07 14:08:51.000000000","message":"very good to have this called out as time/byte rather than byte/sec","commit_id":"c02ce09a46fdada196b7ca8d33e9209deddf6c78"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"96628b88c4bc10aa992ffd3402a9161d74735488","unresolved":false,"context_lines":[{"line_number":491,"context_line":"        Update a timing metric, aggregated percentiles recalculated."},{"line_number":492,"context_line":""},{"line_number":493,"context_line":"        This is a timing metric, but adjusts the timing data per kB transferred"},{"line_number":494,"context_line":"        (ms/kB) for each non-zero-byte update.  Allegedly this could be used to"},{"line_number":495,"context_line":"        monitor problematic devices, where higher is bad."},{"line_number":496,"context_line":""},{"line_number":497,"context_line":"        :param metric: name of the metric"}],"source_content_type":"text/x-python","patch_set":3,"id":"65e54393_728c658e","line":494,"range":{"start_line":494,"start_character":9,"end_line":494,"end_character":14},"in_reply_to":"bceb7974_279bddf1","updated":"2025-04-07 19:13:01.000000000","message":"Acknowledged","commit_id":"c02ce09a46fdada196b7ca8d33e9209deddf6c78"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"2d895e51b9b3454e7281bf6fd38d0808875c80be","unresolved":true,"context_lines":[{"line_number":566,"context_line":"            sample_rate,"},{"line_number":567,"context_line":"            sorted(all_labels.items()))"},{"line_number":568,"context_line":""},{"line_number":569,"context_line":"    def update_stats(self, metric, value, *, labels\u003dNone, sample_rate\u003dNone):"},{"line_number":570,"context_line":"        \"\"\""},{"line_number":571,"context_line":"        Update a counter, aggregated metric changed by value."},{"line_number":572,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"1dc33235_64f998f1","line":569,"updated":"2025-04-07 14:08:51.000000000","message":"ok, TIL https://peps.python.org/pep-3102/","commit_id":"c02ce09a46fdada196b7ca8d33e9209deddf6c78"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"96628b88c4bc10aa992ffd3402a9161d74735488","unresolved":false,"context_lines":[{"line_number":566,"context_line":"            sample_rate,"},{"line_number":567,"context_line":"            sorted(all_labels.items()))"},{"line_number":568,"context_line":""},{"line_number":569,"context_line":"    def update_stats(self, metric, value, *, labels\u003dNone, sample_rate\u003dNone):"},{"line_number":570,"context_line":"        \"\"\""},{"line_number":571,"context_line":"        Update a counter, aggregated metric changed by value."},{"line_number":572,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"e52f11e5_54d531a6","line":569,"in_reply_to":"1dc33235_64f998f1","updated":"2025-04-07 19:13:01.000000000","message":"Acknowledged","commit_id":"c02ce09a46fdada196b7ca8d33e9209deddf6c78"}],"test/debug_logger.py":[{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"6b510e602704a9bf0960b062396ce35df29b09fc","unresolved":true,"context_lines":[{"line_number":83,"context_line":""},{"line_number":84,"context_line":"    def get_increments(self):"},{"line_number":85,"context_line":"        \"\"\""},{"line_number":86,"context_line":"        Checky helper to avoid spelling a tricky list comprehension"},{"line_number":87,"context_line":""},{"line_number":88,"context_line":"        :returns: a list of the \"metric\" arg for all calls to increment"},{"line_number":89,"context_line":"        \"\"\""}],"source_content_type":"text/x-python","patch_set":4,"id":"f4f8d802_605f1f66","line":86,"updated":"2025-04-08 12:20:56.000000000","message":"type? Cheeky?","commit_id":"acfed0da0e1c1c6e2c38e54abf88b27a2fcee710"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"b9bd4202357fee91789e13551cb37576e914111d","unresolved":false,"context_lines":[{"line_number":83,"context_line":""},{"line_number":84,"context_line":"    def get_increments(self):"},{"line_number":85,"context_line":"        \"\"\""},{"line_number":86,"context_line":"        Checky helper to avoid spelling a tricky list comprehension"},{"line_number":87,"context_line":""},{"line_number":88,"context_line":"        :returns: a list of the \"metric\" arg for all calls to increment"},{"line_number":89,"context_line":"        \"\"\""}],"source_content_type":"text/x-python","patch_set":4,"id":"8bfe73d8_097e2d8f","line":86,"in_reply_to":"f4f8d802_605f1f66","updated":"2025-04-09 00:51:30.000000000","message":"Done","commit_id":"acfed0da0e1c1c6e2c38e54abf88b27a2fcee710"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"6b510e602704a9bf0960b062396ce35df29b09fc","unresolved":true,"context_lines":[{"line_number":100,"context_line":"            counters \u003d defaultdict(int)"},{"line_number":101,"context_line":"            value_map \u003d {"},{"line_number":102,"context_line":"                \u0027increment\u0027: 1,"},{"line_number":103,"context_line":"                \u0027decrement\u0027: -1,"},{"line_number":104,"context_line":"            }"},{"line_number":105,"context_line":"            for method, calls in self.calls.items():"},{"line_number":106,"context_line":"                for args, _kwargs in calls:"}],"source_content_type":"text/x-python","patch_set":4,"id":"d602455e_31abdfa5","line":103,"updated":"2025-04-08 12:20:56.000000000","message":"FWIW I don\u0027t think we have an callers of ``decrement`` !!","commit_id":"acfed0da0e1c1c6e2c38e54abf88b27a2fcee710"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"b9bd4202357fee91789e13551cb37576e914111d","unresolved":false,"context_lines":[{"line_number":100,"context_line":"            counters \u003d defaultdict(int)"},{"line_number":101,"context_line":"            value_map \u003d {"},{"line_number":102,"context_line":"                \u0027increment\u0027: 1,"},{"line_number":103,"context_line":"                \u0027decrement\u0027: -1,"},{"line_number":104,"context_line":"            }"},{"line_number":105,"context_line":"            for method, calls in self.calls.items():"},{"line_number":106,"context_line":"                for args, _kwargs in calls:"}],"source_content_type":"text/x-python","patch_set":4,"id":"2c9c2e19_1ed5991f","line":103,"in_reply_to":"d602455e_31abdfa5","updated":"2025-04-09 00:51:30.000000000","message":"Acknowledged","commit_id":"acfed0da0e1c1c6e2c38e54abf88b27a2fcee710"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"6b510e602704a9bf0960b062396ce35df29b09fc","unresolved":true,"context_lines":[{"line_number":112,"context_line":"                    else:"},{"line_number":113,"context_line":"                        continue"},{"line_number":114,"context_line":"                    counters[metric] +\u003d value"},{"line_number":115,"context_line":"            return counters"},{"line_number":116,"context_line":"        \"\"\""},{"line_number":117,"context_line":"        self.counters[metric] +\u003d value"},{"line_number":118,"context_line":"        return super()._update_stats(metric, value, *args, **kwargs)"}],"source_content_type":"text/x-python","patch_set":4,"id":"f0d8ac59_775a44f0","line":115,"updated":"2025-04-08 12:20:56.000000000","message":"I don\u0027t think it is necessary to justify the implementation by including an alternative. If you really wanted it for the record I\u0027d suggest it should be a code comment rather than docstring.","commit_id":"acfed0da0e1c1c6e2c38e54abf88b27a2fcee710"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"b9bd4202357fee91789e13551cb37576e914111d","unresolved":false,"context_lines":[{"line_number":112,"context_line":"                    else:"},{"line_number":113,"context_line":"                        continue"},{"line_number":114,"context_line":"                    counters[metric] +\u003d value"},{"line_number":115,"context_line":"            return counters"},{"line_number":116,"context_line":"        \"\"\""},{"line_number":117,"context_line":"        self.counters[metric] +\u003d value"},{"line_number":118,"context_line":"        return super()._update_stats(metric, value, *args, **kwargs)"}],"source_content_type":"text/x-python","patch_set":4,"id":"23ed1fbb_f5c79493","line":115,"in_reply_to":"f0d8ac59_775a44f0","updated":"2025-04-09 00:51:30.000000000","message":"mainly i just wanted to call it out in review - i had assumed there might be some additional iteration depending on the reception of the solution I came up with recognizing that there is obviously more than one way to do it.\n\nIn the vein of \"writing down what we did NOT do\" I thought the prose explaining the difference between `get_increments` and `get_update_stats` might be useful in the future and the example code was just a side-effect of \"diffs work best\"\n\n\u003e not a practice I\u0027d want to encourage in general\n\nFair enough, I\u0027ll remove it.","commit_id":"acfed0da0e1c1c6e2c38e54abf88b27a2fcee710"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"6b510e602704a9bf0960b062396ce35df29b09fc","unresolved":true,"context_lines":[{"line_number":115,"context_line":"            return counters"},{"line_number":116,"context_line":"        \"\"\""},{"line_number":117,"context_line":"        self.counters[metric] +\u003d value"},{"line_number":118,"context_line":"        return super()._update_stats(metric, value, *args, **kwargs)"},{"line_number":119,"context_line":""},{"line_number":120,"context_line":"    # some aliases for backwards compat"},{"line_number":121,"context_line":"    get_stats_counts \u003d get_increment_counts \u003d lambda self: self.counters"}],"source_content_type":"text/x-python","patch_set":4,"id":"5a5e8352_8c2cd958","line":118,"updated":"2025-04-08 12:20:56.000000000","message":"+1 this is much better than my suggestion","commit_id":"acfed0da0e1c1c6e2c38e54abf88b27a2fcee710"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"b9bd4202357fee91789e13551cb37576e914111d","unresolved":false,"context_lines":[{"line_number":115,"context_line":"            return counters"},{"line_number":116,"context_line":"        \"\"\""},{"line_number":117,"context_line":"        self.counters[metric] +\u003d value"},{"line_number":118,"context_line":"        return super()._update_stats(metric, value, *args, **kwargs)"},{"line_number":119,"context_line":""},{"line_number":120,"context_line":"    # some aliases for backwards compat"},{"line_number":121,"context_line":"    get_stats_counts \u003d get_increment_counts \u003d lambda self: self.counters"}],"source_content_type":"text/x-python","patch_set":4,"id":"3dbb4abc_b99069e3","line":118,"in_reply_to":"5a5e8352_8c2cd958","updated":"2025-04-09 00:51:30.000000000","message":"Acknowledged","commit_id":"acfed0da0e1c1c6e2c38e54abf88b27a2fcee710"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"6b510e602704a9bf0960b062396ce35df29b09fc","unresolved":true,"context_lines":[{"line_number":118,"context_line":"        return super()._update_stats(metric, value, *args, **kwargs)"},{"line_number":119,"context_line":""},{"line_number":120,"context_line":"    # some aliases for backwards compat"},{"line_number":121,"context_line":"    get_stats_counts \u003d get_increment_counts \u003d lambda self: self.counters"},{"line_number":122,"context_line":""},{"line_number":123,"context_line":""},{"line_number":124,"context_line":"class FakeStatsdClient(BaseFakeStatsdClient, statsd_client.StatsdClient):"}],"source_content_type":"text/x-python","patch_set":4,"id":"6a12cac5_478192cf","line":121,"range":{"start_line":121,"start_character":23,"end_line":121,"end_character":43},"updated":"2025-04-08 12:20:56.000000000","message":"This changes the contract of ``get_increment_counts`` vs ``get_stats_counts``. It works because apparently it\u0027s never been used in a context where the increment() and update_stats() have both been used (or maybe in isolated cases fixed in this patch). That in itself is testament to ``get_increment_counts`` being a little suspect vs ``get_update_stats`` as indeed the comment on master suggests.\n\nI would prefer to get rid of ``get_increment_counts``, rather than preserve it but with a different meaning. There\u0027ll be some churn but I fear that in future someone (I) will use get_increment_counts, thinking (or not) that it is *just* the calls to increment (which it used to be). Let\u0027s migrate to the new pattern of asserting ``self.counters`` which captures the whole story (increments and update_stats).\n\nThis ^^ can be done in a follow up: 946621: tests: Remove BaseFakeStatsdClient.get_increment_counts | https://review.opendev.org/c/openstack/swift/+/946621\n\nI\u0027m not so bothered about keeping get_stats_counts because it still returns the same thing.","commit_id":"acfed0da0e1c1c6e2c38e54abf88b27a2fcee710"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"b9bd4202357fee91789e13551cb37576e914111d","unresolved":false,"context_lines":[{"line_number":118,"context_line":"        return super()._update_stats(metric, value, *args, **kwargs)"},{"line_number":119,"context_line":""},{"line_number":120,"context_line":"    # some aliases for backwards compat"},{"line_number":121,"context_line":"    get_stats_counts \u003d get_increment_counts \u003d lambda self: self.counters"},{"line_number":122,"context_line":""},{"line_number":123,"context_line":""},{"line_number":124,"context_line":"class FakeStatsdClient(BaseFakeStatsdClient, statsd_client.StatsdClient):"}],"source_content_type":"text/x-python","patch_set":4,"id":"d820caa3_88f3bd51","line":121,"range":{"start_line":121,"start_character":23,"end_line":121,"end_character":43},"in_reply_to":"6a12cac5_478192cf","updated":"2025-04-09 00:51:30.000000000","message":"\u003e it\u0027s never been used in a context where the increment() and update_stats() have both been used (or maybe in isolated cases fixed in this patch)\n\nthat was my evaluation as well, and the \"caveat\" suggested authors probably should NOT use get_increment_counts when get_update_stats is probably a stronger assertion.\n\n\u003e I would prefer to get rid of get_increment_counts, rather than preserve it but with a different meaning\n\nsame, although I had less distaste for \"preserve as an alias\" since so few existing tests even seemed to acknowledge the difference much less depend on it - we use increment far more frequently than update_stats so the get_increment_counts as an alias might match someone\u0027s thinking when the go to reach for a helper method.\n\n\u003e thinking (or not) that it is just the calls to increment (which it used to be)\n\nI mean, there\u0027s some evidence to suggest that wouldn\u0027t matter anyway - but when it does you\u0027d get an assertion error and can decide what you actually want to assert.  I think the more common case was assert-get-increments just accidentally misses some update_stats counters: i.e. the existing behavior sucked\n\n\u003e Let\u0027s migrate to the new pattern of asserting self.counters which captures the whole story (increments and update_stats).\n\nfair, FWIW I think either assert-counters or assert-increments are the most intuitive patterns to carry forward tho given the prevalence of increment calls; the `get_stats_counts` alias is hard for me say and I don\u0027t understand the name beyond roughly the implementation detail of increment sharing some linage with the (now private) `_update_stats` method.\n\n\u003e This ^^ can be done in a follow up\n\nthank goodness ;)","commit_id":"acfed0da0e1c1c6e2c38e54abf88b27a2fcee710"}],"test/unit/common/middleware/s3api/test_s3api.py":[{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"96628b88c4bc10aa992ffd3402a9161d74735488","unresolved":true,"context_lines":[{"line_number":1754,"context_line":""},{"line_number":1755,"context_line":"        self.assertEqual("},{"line_number":1756,"context_line":"            [\u0027403.AccessDenied.invalid_credential\u0027],"},{"line_number":1757,"context_line":"            self.s3api.logger.statsd_client.get_increments())"},{"line_number":1758,"context_line":"        self.assertEqual(\u0027s3:err:AccessDenied.invalid_credential\u0027,"},{"line_number":1759,"context_line":"                         get_log_info(req.environ))"},{"line_number":1760,"context_line":"        self.assertEqual("}],"source_content_type":"text/x-python","patch_set":4,"id":"4ea0f99a_aff96105","line":1757,"updated":"2025-04-07 19:13:01.000000000","message":"this isn\u0027t the only place we use `get_increments` - so \"fixing\" this assertion different wouldn\u0027t have gotten rid of the pre-existing helper, and this diff seemed to make more sense than:\n\n```\ndiff --git a/test/unit/common/middleware/s3api/test_s3api.py b/test/unit/common/middleware/s3api/test_s3api.py\nindex 70d0d3627..02542a3c7 100644\n--- a/test/unit/common/middleware/s3api/test_s3api.py\n+++ b/test/unit/common/middleware/s3api/test_s3api.py\n@@ -1752,9 +1752,10 @@ class TestS3ApiMiddleware(S3ApiTestCase):\n         app \u003d ProxyLoggingMiddleware(self.s3api, log_conf, self.logger)\n         status, headers, body \u003d self.call_app(req, app\u003dapp)\n \n-        self.assertEqual(\n-            [\u0027403.AccessDenied.invalid_credential\u0027],\n-            self.s3api.logger.statsd_client.get_increments())\n+        self.assertEqual({\n+            \u0027403.AccessDenied.invalid_credential\u0027: 1,\n+            \u0027UNKNOWN.HEAD.403.xfer\u0027: 0,\n+        }, self.s3api.logger.statsd_client.counters)\n         self.assertEqual(\u0027s3:err:AccessDenied.invalid_credential\u0027,\n                          get_log_info(req.environ))\n         self.assertEqual(\n```\n\n... even tho I did confirm the xfer byte count was emitted by proxy-logging via an update_stats call I don\u0027t really know what to make of the UNKNOWN bit:\n\nhttps://github.com/openstack/swift/blob/master/swift/common/middleware/proxy_logging.py#L342\n\n*should* we expect to be lacking a metric_type_name ???  I thought I could just try to side-step it.","commit_id":"acfed0da0e1c1c6e2c38e54abf88b27a2fcee710"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"6b510e602704a9bf0960b062396ce35df29b09fc","unresolved":true,"context_lines":[{"line_number":1754,"context_line":""},{"line_number":1755,"context_line":"        self.assertEqual("},{"line_number":1756,"context_line":"            [\u0027403.AccessDenied.invalid_credential\u0027],"},{"line_number":1757,"context_line":"            self.s3api.logger.statsd_client.get_increments())"},{"line_number":1758,"context_line":"        self.assertEqual(\u0027s3:err:AccessDenied.invalid_credential\u0027,"},{"line_number":1759,"context_line":"                         get_log_info(req.environ))"},{"line_number":1760,"context_line":"        self.assertEqual("}],"source_content_type":"text/x-python","patch_set":4,"id":"5424bc1d_57604fe3","line":1757,"in_reply_to":"4ea0f99a_aff96105","updated":"2025-04-08 12:20:56.000000000","message":"I guess the UNKNOWN is because the request fails early in s3api so swift.backend_path is not set and proxy_logging cannot infer what \u0027type\u0027 pf request is is","commit_id":"acfed0da0e1c1c6e2c38e54abf88b27a2fcee710"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"b9bd4202357fee91789e13551cb37576e914111d","unresolved":false,"context_lines":[{"line_number":1754,"context_line":""},{"line_number":1755,"context_line":"        self.assertEqual("},{"line_number":1756,"context_line":"            [\u0027403.AccessDenied.invalid_credential\u0027],"},{"line_number":1757,"context_line":"            self.s3api.logger.statsd_client.get_increments())"},{"line_number":1758,"context_line":"        self.assertEqual(\u0027s3:err:AccessDenied.invalid_credential\u0027,"},{"line_number":1759,"context_line":"                         get_log_info(req.environ))"},{"line_number":1760,"context_line":"        self.assertEqual("}],"source_content_type":"text/x-python","patch_set":4,"id":"73e11423_c3102356","line":1757,"in_reply_to":"5424bc1d_57604fe3","updated":"2025-04-09 00:51:30.000000000","message":"\u003e proxy_logging cannot infer what \u0027type\u0027 pf request is is\n\nright, pre-existing - so ... I\u0027ll leave it the way I wrote it.","commit_id":"acfed0da0e1c1c6e2c38e54abf88b27a2fcee710"}],"test/unit/container/test_replicator.py":[{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"6b510e602704a9bf0960b062396ce35df29b09fc","unresolved":true,"context_lines":[{"line_number":890,"context_line":"            \u0027successes\u0027: 4,"},{"line_number":891,"context_line":"            \u0027rsyncs\u0027: 3,"},{"line_number":892,"context_line":"            \u0027reconciler_db_created\u0027: 1,"},{"line_number":893,"context_line":"        }, self.logger.statsd_client.counters)"},{"line_number":894,"context_line":""},{"line_number":895,"context_line":"        # grab the rsynced instance of remote_broker"},{"line_number":896,"context_line":"        remote_broker \u003d self._get_broker(\u0027a\u0027, \u0027c\u0027, node_index\u003d1)"}],"source_content_type":"text/x-python","patch_set":4,"id":"62e90783_a0418e7e","line":893,"updated":"2025-04-08 12:20:56.000000000","message":"ok, this looks like a more exact assertion\n\nnote to self: The call to get_stats_counts() on master no longer gave the expected value because it no longer counted called to increment().","commit_id":"acfed0da0e1c1c6e2c38e54abf88b27a2fcee710"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"b9bd4202357fee91789e13551cb37576e914111d","unresolved":false,"context_lines":[{"line_number":890,"context_line":"            \u0027successes\u0027: 4,"},{"line_number":891,"context_line":"            \u0027rsyncs\u0027: 3,"},{"line_number":892,"context_line":"            \u0027reconciler_db_created\u0027: 1,"},{"line_number":893,"context_line":"        }, self.logger.statsd_client.counters)"},{"line_number":894,"context_line":""},{"line_number":895,"context_line":"        # grab the rsynced instance of remote_broker"},{"line_number":896,"context_line":"        remote_broker \u003d self._get_broker(\u0027a\u0027, \u0027c\u0027, node_index\u003d1)"}],"source_content_type":"text/x-python","patch_set":4,"id":"6a3f9715_1e1c196d","line":893,"in_reply_to":"62e90783_a0418e7e","updated":"2025-04-09 00:51:30.000000000","message":"\u003e this looks like a more exact assertion\n\nyeah I hate `assert(1, dict.get(\u0027key\u0027)` just freaking `assert(1, dict[\u0027key\u0027])` or yeah, even better `assert(expected, dict)` and you can avoid the silly `assert(None, dict.get(\u0027key\u0027))` ... like, was it in the dict or not!?\n\n\u003e get_stats_counts() on master no longer gave the expected value because it no longer counted called to increment().\n\nwhat?  really!?","commit_id":"acfed0da0e1c1c6e2c38e54abf88b27a2fcee710"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"b8bce895dd89146bab1207e3fe3274b9a8fdae71","unresolved":false,"context_lines":[{"line_number":890,"context_line":"            \u0027successes\u0027: 4,"},{"line_number":891,"context_line":"            \u0027rsyncs\u0027: 3,"},{"line_number":892,"context_line":"            \u0027reconciler_db_created\u0027: 1,"},{"line_number":893,"context_line":"        }, self.logger.statsd_client.counters)"},{"line_number":894,"context_line":""},{"line_number":895,"context_line":"        # grab the rsynced instance of remote_broker"},{"line_number":896,"context_line":"        remote_broker \u003d self._get_broker(\u0027a\u0027, \u0027c\u0027, node_index\u003d1)"}],"source_content_type":"text/x-python","patch_set":4,"id":"81664040_0eebce57","line":893,"in_reply_to":"6a3f9715_1e1c196d","updated":"2025-04-09 09:26:39.000000000","message":"\u003e\u003e get_stats_counts() on master no longer gave the expected value because it \n\u003e\u003e no longer counted called to increment().\n\n\u003e what? really!?\n\nIIUC on master get_stats_counts() calls get_update_stats() which returned calls to update_stats(). On master increment called update_stats. With this change increment no longer calls update_stats so the implementation of get_stats_counts on master would have lost increments.\n\nAnyway, this is better 😊","commit_id":"acfed0da0e1c1c6e2c38e54abf88b27a2fcee710"}],"test/unit/obj/test_reconstructor.py":[{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"6b510e602704a9bf0960b062396ce35df29b09fc","unresolved":true,"context_lines":[{"line_number":3731,"context_line":"        }"},{"line_number":3732,"context_line":"        stub_hashes \u003d {}"},{"line_number":3733,"context_line":"        with mock.patch(\u0027swift.obj.diskfile.ECDiskFileManager._get_hashes\u0027,"},{"line_number":3734,"context_line":"                        return_value\u003d(0, stub_hashes)):"},{"line_number":3735,"context_line":"            jobs \u003d self.reconstructor.build_reconstruction_jobs(part_info)"},{"line_number":3736,"context_line":"        self.assertEqual(1, len(jobs))"},{"line_number":3737,"context_line":"        job \u003d jobs[0]"}],"source_content_type":"text/x-python","patch_set":4,"id":"b41cf113_55f5a752","line":3734,"updated":"2025-04-08 12:20:56.000000000","message":"so...was the problem that the FakeStatsdClient now actually increments an int counter which is not going to like having ``None`` added to it?","commit_id":"acfed0da0e1c1c6e2c38e54abf88b27a2fcee710"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"b9bd4202357fee91789e13551cb37576e914111d","unresolved":false,"context_lines":[{"line_number":3731,"context_line":"        }"},{"line_number":3732,"context_line":"        stub_hashes \u003d {}"},{"line_number":3733,"context_line":"        with mock.patch(\u0027swift.obj.diskfile.ECDiskFileManager._get_hashes\u0027,"},{"line_number":3734,"context_line":"                        return_value\u003d(0, stub_hashes)):"},{"line_number":3735,"context_line":"            jobs \u003d self.reconstructor.build_reconstruction_jobs(part_info)"},{"line_number":3736,"context_line":"        self.assertEqual(1, len(jobs))"},{"line_number":3737,"context_line":"        job \u003d jobs[0]"}],"source_content_type":"text/x-python","patch_set":4,"id":"4f0c45ae_a1bb1ac5","line":3734,"in_reply_to":"b41cf113_55f5a752","updated":"2025-04-09 00:51:30.000000000","message":"\u003e fix stub return value TypeError\n\n*exactly*","commit_id":"acfed0da0e1c1c6e2c38e54abf88b27a2fcee710"}],"test/unit/obj/test_updater.py":[{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"6b510e602704a9bf0960b062396ce35df29b09fc","unresolved":true,"context_lines":[{"line_number":2648,"context_line":"            \u0027unlinks\u0027: 5,"},{"line_number":2649,"context_line":"            \u0027deferrals\u0027: 4,"},{"line_number":2650,"context_line":"            \u0027drains\u0027: 2,"},{"line_number":2651,"context_line":"            \u0027skips\u0027: 2,"},{"line_number":2652,"context_line":"        }, self.logger.statsd_client.counters)"},{"line_number":2653,"context_line":"        self.assertEqual("},{"line_number":2654,"context_line":"            2, self.logger.statsd_client.get_stats_counts()[\u0027skips\u0027])"}],"source_content_type":"text/x-python","patch_set":4,"id":"978ecff5_faefe00d","line":2651,"updated":"2025-04-08 12:20:56.000000000","message":"ok, skips is inc\u0027d using using_stats, the others use increment, now we get to assert them all","commit_id":"acfed0da0e1c1c6e2c38e54abf88b27a2fcee710"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"b9bd4202357fee91789e13551cb37576e914111d","unresolved":false,"context_lines":[{"line_number":2648,"context_line":"            \u0027unlinks\u0027: 5,"},{"line_number":2649,"context_line":"            \u0027deferrals\u0027: 4,"},{"line_number":2650,"context_line":"            \u0027drains\u0027: 2,"},{"line_number":2651,"context_line":"            \u0027skips\u0027: 2,"},{"line_number":2652,"context_line":"        }, self.logger.statsd_client.counters)"},{"line_number":2653,"context_line":"        self.assertEqual("},{"line_number":2654,"context_line":"            2, self.logger.statsd_client.get_stats_counts()[\u0027skips\u0027])"}],"source_content_type":"text/x-python","patch_set":4,"id":"1c0d0fdf_b32ad6db","line":2651,"in_reply_to":"978ecff5_faefe00d","updated":"2025-04-09 00:51:30.000000000","message":"yup, this one got a chuckle from me because you can see `2 skips` right there in the `assertIn(log_lines)` but this assert made it *look* like we didn\u0027t get metrics for skips!","commit_id":"acfed0da0e1c1c6e2c38e54abf88b27a2fcee710"}]}