)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"53ae68de9b6ece9b3c668b0cee01c4500712c2e1","unresolved":true,"context_lines":[{"line_number":4,"context_line":"Commit:     Alistair Coles \u003calistairncoles@gmail.com\u003e"},{"line_number":5,"context_line":"CommitDate: 2025-04-08 22:04:12 +0000"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"tests: Remove BaseFakeStatsdClient.get_increment_counts"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"The method is redundant and confusingly named. Since the"},{"line_number":10,"context_line":"Related-Change it actually returns the aggregate value of counters,"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"67d080ce_da1d3048","line":7,"updated":"2025-04-09 01:35:41.000000000","message":"this seems like an unnecessary half-measure in the wrong direction\n\nmy preference would be:\n\n1) abandon this change - sure we could make additional technical investment but is THIS the most problematic inconsistency in the test suite?\n2) remove the aliases, it\u0027s way more obvious to use a single pattern consistently and the least confusing thing is to assert on statsd_client.counters *directly*\n\nI don\u0027t think getting rid of either _just_ `get_increment_counts` nor `get_stats_counts` does much to improve the test suite; but with nod towards existing adoption and future authors - I may have preferred keeping `get_increment_counts` despite it torrid legacy.\n\n\u003e There should be two-- and preferably only two --obvious way(s) to do it.","commit_id":"37a25b340d67df8e6a1aca42e45922e45ddf51e9"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"21971642c15605b577f5a0281dceba2f463eec33","unresolved":true,"context_lines":[{"line_number":4,"context_line":"Commit:     Alistair Coles \u003calistairncoles@gmail.com\u003e"},{"line_number":5,"context_line":"CommitDate: 2025-04-08 22:04:12 +0000"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"tests: Remove BaseFakeStatsdClient.get_increment_counts"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"The method is redundant and confusingly named. Since the"},{"line_number":10,"context_line":"Related-Change it actually returns the aggregate value of counters,"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"8239a2bd_8396f540","line":7,"in_reply_to":"67d080ce_da1d3048","updated":"2025-04-09 10:07:08.000000000","message":"\u003e this seems like an unnecessary half-measure in the wrong direction\n\nGoing from 3 way to 2 ways must be the *right* direction, but I agree this is a half-measure. I tackled the method that IMHO has a confusing name and whose behaviour has changed. I don\u0027t want to see that method used in new code.\n\n\u003e is THIS the most problematic inconsistency in the test suite?\n\nI wouldn\u0027t want to consume too much of anyone\u0027s time on this, but I  don\u0027t think I want to be restricted to only work on the single most problematic issue when there\u0027s something topical staring me in the face. The degree of each problem is clearly a matter for opinion.\n\nIt really was an insignificant investment on my part to make the global replacement, but I concede it is churn and requires review bandwidth too, which is why I did not change get_stats_counts because IMHO it has no semantic confusion: `\"get_stats_counts()` returns the stats counters\". It does what it says, it\u0027s just unnecessary.","commit_id":"37a25b340d67df8e6a1aca42e45922e45ddf51e9"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"3b6771727d2d36eaf6380bdb43c19dc4f6771ca9","unresolved":true,"context_lines":[{"line_number":4,"context_line":"Commit:     Alistair Coles \u003calistairncoles@gmail.com\u003e"},{"line_number":5,"context_line":"CommitDate: 2025-04-08 22:04:12 +0000"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"tests: Remove BaseFakeStatsdClient.get_increment_counts"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"The method is redundant and confusingly named. Since the"},{"line_number":10,"context_line":"Related-Change it actually returns the aggregate value of counters,"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"8a572172_1a6da8eb","line":7,"in_reply_to":"8239a2bd_8396f540","updated":"2025-04-09 14:35:51.000000000","message":"\u003e It really was an insignificant investment on my part to make the global replacement\n\nthis was what I was really worried about, I don\u0027t know enough about my tools to \"trivially\" do a global find and replace all \".get_stats_counts()\" with \".counters\" very quickly.  It would take me a good hot minute to whip up a 150 line diff so if I was going to do it I\u0027d want to do it once and for all.","commit_id":"37a25b340d67df8e6a1aca42e45922e45ddf51e9"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"53ae68de9b6ece9b3c668b0cee01c4500712c2e1","unresolved":true,"context_lines":[{"line_number":6,"context_line":""},{"line_number":7,"context_line":"tests: Remove BaseFakeStatsdClient.get_increment_counts"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"The method is redundant and confusingly named. Since the"},{"line_number":10,"context_line":"Related-Change it actually returns the aggregate value of counters,"},{"line_number":11,"context_line":"not just the count of calls made to the increment method alone."},{"line_number":12,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"aaef9c9a_28c9b653","line":9,"updated":"2025-04-09 01:35:41.000000000","message":"redundant sure, I think \"confusingly named\" depends on your familiarity with the history of this particular wart in the test code base - I wouldn\u0027t imagine any new test author would be \"confused\" that `get_increment_counts` returned all the statsd counters current values - including the rare cases where changes to statsd counters are made via `update_stats` and `decrement` - if they even noticed and thought about it for a second that\u0027s exactly what they should expect `get_increment_counts` to do as anything else is asking for trouble, i.e.\n\nIt think what WAS confusing was how a metric counter we know exists doesn\u0027t show up in an assert on metric counters because `get_increment_counts` was trying to be a fancy alias over `statsd_client.calls`... which are _still available_ BTW should anyone ever need to assert something more specific about HOW the statsd counters got to their current values as opposed to \"just\" what the value is after running the test regardless of what statsd methods it used to get there.","commit_id":"37a25b340d67df8e6a1aca42e45922e45ddf51e9"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"21971642c15605b577f5a0281dceba2f463eec33","unresolved":true,"context_lines":[{"line_number":6,"context_line":""},{"line_number":7,"context_line":"tests: Remove BaseFakeStatsdClient.get_increment_counts"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"The method is redundant and confusingly named. Since the"},{"line_number":10,"context_line":"Related-Change it actually returns the aggregate value of counters,"},{"line_number":11,"context_line":"not just the count of calls made to the increment method alone."},{"line_number":12,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"da742fd7_fac74fea","line":9,"in_reply_to":"aaef9c9a_28c9b653","updated":"2025-04-09 10:07:08.000000000","message":"For me, given an interface with a method ``increment``, and a Fake that has historically counted method calls, I expect ``get_increment_counts`` to tell me how many times ``increment`` had been called, regardless of the usefulness or not of that semantic. Perhaps, apart from the naming, my expectation is based on my familiarity with what the method used to return.\n\nBut, let me be clear, I in no way want to preserve that semantic. I fully support removing the incomplete picture of the stats that get_increment_counts returned. I like that ``counters`` will now give me the complete picture of the values of the counters. I\u0027d just rather see the back of what I still consider to be an inappropriately named helper.","commit_id":"37a25b340d67df8e6a1aca42e45922e45ddf51e9"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"3b6771727d2d36eaf6380bdb43c19dc4f6771ca9","unresolved":true,"context_lines":[{"line_number":6,"context_line":""},{"line_number":7,"context_line":"tests: Remove BaseFakeStatsdClient.get_increment_counts"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"The method is redundant and confusingly named. Since the"},{"line_number":10,"context_line":"Related-Change it actually returns the aggregate value of counters,"},{"line_number":11,"context_line":"not just the count of calls made to the increment method alone."},{"line_number":12,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"7f38d684_b1210082","line":9,"in_reply_to":"da742fd7_fac74fea","updated":"2025-04-09 14:35:51.000000000","message":"\u003e  a Fake that has historically counted method calls\n\nit *still* counts method calls, when you want  the *value* of a metric counter stat it\u0027s a fundamentally question (that happens to \u003d\u003d increments in ~85% of the code base)\n\n\u003e I expect get_increment_counts to tell me how many times increment had been called\n\nand while most of the time there is no difference, when it matters it\u0027s obvious to me that a method named `get_increment_counts` should very much NOT \"just\" return calls to increment - there\u0027s method tracking if that\u0027s what you really need.\n\n\u003e  I in no way want to preserve that semantic.\n\nfantastic!  it seems to me like the difference between \"get_increment_counts\" does what we both want and \"get_increment_counts\" doesn\u0027t exist is not very big.","commit_id":"37a25b340d67df8e6a1aca42e45922e45ddf51e9"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"53ae68de9b6ece9b3c668b0cee01c4500712c2e1","unresolved":true,"context_lines":[{"line_number":8,"context_line":""},{"line_number":9,"context_line":"The method is redundant and confusingly named. Since the"},{"line_number":10,"context_line":"Related-Change it actually returns the aggregate value of counters,"},{"line_number":11,"context_line":"not just the count of calls made to the increment method alone."},{"line_number":12,"context_line":""},{"line_number":13,"context_line":"Related-Change: I4d8a4b530465b587caced4362eb9178507701cef"},{"line_number":14,"context_line":"Change-Id: I52585567a177f58e0f110785b44f8d238b2ac54d"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"ac465b01_e4985732","line":11,"updated":"2025-04-09 01:35:41.000000000","message":"... and removed a confusing wart in those helpers to make their behavior reasonable and consistent and improved assertions along the way.","commit_id":"37a25b340d67df8e6a1aca42e45922e45ddf51e9"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"3b6771727d2d36eaf6380bdb43c19dc4f6771ca9","unresolved":true,"context_lines":[{"line_number":8,"context_line":""},{"line_number":9,"context_line":"The method is redundant and confusingly named. Since the"},{"line_number":10,"context_line":"Related-Change it actually returns the aggregate value of counters,"},{"line_number":11,"context_line":"not just the count of calls made to the increment method alone."},{"line_number":12,"context_line":""},{"line_number":13,"context_line":"Related-Change: I4d8a4b530465b587caced4362eb9178507701cef"},{"line_number":14,"context_line":"Change-Id: I52585567a177f58e0f110785b44f8d238b2ac54d"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"a80427d6_efefd70e","line":11,"in_reply_to":"2ed595a9_9f9277b9","updated":"2025-04-09 14:35:51.000000000","message":"I recognize my tone towards this change was negative but it came from a place of concern that your valuable time/effort here was tedious or distracting.  \n\nI really wasn\u0027t feeling touchy, I assumed this was saying:\n\nIf get_increment_counts continues to exist it should return only:\n\n    Counter(self.get_increments())\n\n... which I fundamentally disagree was a \"correct\" implementation for that method, it was merely \"so close enough\" that it almost never matters.\n\nAlso I don\u0027t have any disparagement for `get_stats_counts` (other than the double plural, i had trouble even *remembering* the spelling, I think I\u0027ve got it now tho).  Had I encountered a pre-existing `get_increment_counts` with the wrong semantic used 180+ times across the test suite I would have written the correct implementation with a new name and used that to get my test done and moved on same as you:\n\n917711: Add labeled metrics to proxy-logging | https://review.opendev.org/c/openstack/swift/+/917711\n\n^ I don\u0027t think I appreciated the history you\u0027ve had with this mess, for me it was all new and just a distraction from the labeled metrics follow-up.","commit_id":"37a25b340d67df8e6a1aca42e45922e45ddf51e9"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"21971642c15605b577f5a0281dceba2f463eec33","unresolved":true,"context_lines":[{"line_number":8,"context_line":""},{"line_number":9,"context_line":"The method is redundant and confusingly named. Since the"},{"line_number":10,"context_line":"Related-Change it actually returns the aggregate value of counters,"},{"line_number":11,"context_line":"not just the count of calls made to the increment method alone."},{"line_number":12,"context_line":""},{"line_number":13,"context_line":"Related-Change: I4d8a4b530465b587caced4362eb9178507701cef"},{"line_number":14,"context_line":"Change-Id: I52585567a177f58e0f110785b44f8d238b2ac54d"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"2ed595a9_9f9277b9","line":11,"in_reply_to":"ac465b01_e4985732","updated":"2025-04-09 10:07:08.000000000","message":"There was no criticism implied. The Related-Change is a huge improvement.","commit_id":"37a25b340d67df8e6a1aca42e45922e45ddf51e9"}],"/PATCHSET_LEVEL":[{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"3b6771727d2d36eaf6380bdb43c19dc4f6771ca9","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"3f30403c_f96f7cf6","updated":"2025-04-09 14:35:51.000000000","message":"\u003e Going from 3 way to 2 ways must be the right direction\n\nYou\u0027re right.  This is what progress looks like.  :shipit:","commit_id":"37a25b340d67df8e6a1aca42e45922e45ddf51e9"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"53ae68de9b6ece9b3c668b0cee01c4500712c2e1","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"0cab9acb_0e790f36","updated":"2025-04-09 01:35:41.000000000","message":"I can\u0027t bring myself to care enough to -1 but would be much more enthusiastic about change that removed *all* the aliases of `statsd_client.counters`\n\nI also wouldn\u0027t blame anyone for not finding the round tuits to respin this nor would I expect any future maintainers to lament us not getting back to clearing out the test helper aliases.\n\nIf this merges as is, AFAIK - that\u0027s fine too.","commit_id":"37a25b340d67df8e6a1aca42e45922e45ddf51e9"}],"test/debug_logger.py":[{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"53ae68de9b6ece9b3c668b0cee01c4500712c2e1","unresolved":true,"context_lines":[{"line_number":98,"context_line":""},{"line_number":99,"context_line":"    # getter for backwards compat"},{"line_number":100,"context_line":"    def get_stats_counts(self):"},{"line_number":101,"context_line":"        return self.counters"},{"line_number":102,"context_line":""},{"line_number":103,"context_line":""},{"line_number":104,"context_line":"class FakeStatsdClient(BaseFakeStatsdClient, statsd_client.StatsdClient):"}],"source_content_type":"text/x-python","patch_set":2,"id":"756045ac_218d7f87","line":101,"updated":"2025-04-09 01:35:41.000000000","message":"FWIW I don\u0027t much care for this method name, it\u0027s hard to say \"get statS countS\"\n\nThe only strong argument for keeping `get_stats_counts` and not `get_increment_counts` is because: \"`get_increment_counts` used to mean something other than what you\u0027d expect, and if you remembered that for some reason you might be surprised to find out it\u0027s just another way to spell `get_current_values_of_all_statsd_counters`\"\n\nThe counter argument is \"85% of the time whatever you may have thought it meant or means: it\u0027s equivalent since your code only calls `increment` anyway\"\n\n```\nvagrant-swift-all-in-onecgerrard@NVStation:~/Workspace/vagrant-swift-all-in-one/swift$ grep -R --include \\*.py \u0027increment(\u0027 swift/ | wc -l\n126\nvagrant-swift-all-in-onecgerrard@NVStation:~/Workspace/vagrant-swift-all-in-one/swift$ grep -R --include \\*.py \u0027update_stats(\u0027 swift/ | wc -l\n15\nvagrant-swift-all-in-onecgerrard@NVStation:~/Workspace/vagrant-swift-all-in-one/swift$ grep -R --include \\*.py \u0027decrement(\u0027 swift/ \nswift/common/statsd_client.py:    def decrement(self, metric, sample_rate\u003dNone):\nswift/common/statsd_client.py:    def decrement(self, metric, labels\u003dNone, sample_rate\u003dNone):\n```\n\ni.e. \"MOST of the time people understand their stats use increment and so naturally want to `assert(increment_counts)`\"\n\n```\nvagrant-swift-all-in-onecgerrard@NVStation:~/Workspace/vagrant-swift-all-in-one/swift$ grep -R --include \\*.py get_increment_counts test/ | wc -l\n138\nvagrant-swift-all-in-onecgerrard@NVStation:~/Workspace/vagrant-swift-all-in-one/swift$ grep -R --include \\*.py get_stats_counts test/ | wc -l\n85\n```\n\nif we *have* to touch all the code anyway why not just dodge the issue of which alias is best and use `statsd_client.counters` exclusively.","commit_id":"37a25b340d67df8e6a1aca42e45922e45ddf51e9"}],"test/unit/common/middleware/test_tempurl.py":[{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"53ae68de9b6ece9b3c668b0cee01c4500712c2e1","unresolved":true,"context_lines":[{"line_number":175,"context_line":"        self.assertEqual(self.logger.statsd_client.get_increment_counts(), {"},{"line_number":176,"context_line":"            \u0027tempurl.digests.sha1\u0027: 1,"},{"line_number":177,"context_line":"            \u0027tempurl.digests.sha256\u0027: 2,"},{"line_number":178,"context_line":"            \u0027tempurl.digests.sha512\u0027: 1"},{"line_number":179,"context_line":"        })"},{"line_number":180,"context_line":""},{"line_number":181,"context_line":"    def test_get_valid_key2(self):"}],"source_content_type":"text/x-python","patch_set":2,"id":"7c632c00_5e55984b","side":"PARENT","line":178,"updated":"2025-04-09 01:35:41.000000000","message":"I can understand why this test was written this way:\n\n```\nvagrant-swift-all-in-onecgerrard@NVStation:~/Workspace/vagrant-swift-all-in-one/swift$ grep -R --include \\*.py \u0027logger\\.\u0027 swift/common/middleware/tempurl.py \n        self.logger.increment(\u0027tempurl.digests.%s\u0027 % hash_algorithm)\n```\n\nyou call `increment`, you assert `increment_counts`","commit_id":"563c1ca6564ac94c1fe85ceb45bf7b8585fe3fbb"}]}