)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"746222e60c4ea2faf8eac1c23c6c0cda8bb53fba","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"8fa901d0_949b9d3c","updated":"2023-07-21 17:04:14.000000000","message":"I\u0027ve only just started looking at this, getting a few comments off my laptop","commit_id":"78b986e98e6856a898a1449538860e2de4a4d644"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"f4852deeb01f23e70d1c20f5fa08e39fd746d25d","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":6,"id":"193d2754_e81c345a","updated":"2023-10-12 21:54:51.000000000","message":"Worth responding here, but I think this all-at-once approach may not be sustainable for reviewers. https://review.opendev.org/c/openstack/swift/+/896968 is probably the better patch to watch; maybe I should copy all these comments there?","commit_id":"c55aecefd7c971dc94647cee732a16958e0abb40"}],"swift/common/utils/__init__.py":[{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"746222e60c4ea2faf8eac1c23c6c0cda8bb53fba","unresolved":true,"context_lines":[{"line_number":1221,"context_line":"        return self"},{"line_number":1222,"context_line":""},{"line_number":1223,"context_line":""},{"line_number":1224,"context_line":"LabelModes \u003d ["},{"line_number":1225,"context_line":"    \u0027disabled\u0027,"},{"line_number":1226,"context_line":"    \u0027graphite\u0027,"},{"line_number":1227,"context_line":"    \u0027influxdb\u0027,"}],"source_content_type":"text/x-python","patch_set":2,"id":"dcc0a855_80359a37","line":1224,"updated":"2023-07-21 17:04:14.000000000","message":"other places where we define globals we tend to use SNAKE_CASE_CAPS e.g. TRUE_VALUES in this module, SHARD_*_STATES in container/backend.py\n\nAlso, this might be better as a StatsdClient class attribute: seems like it  is part of the StatsdClient implementation/interface.","commit_id":"78b986e98e6856a898a1449538860e2de4a4d644"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"f4852deeb01f23e70d1c20f5fa08e39fd746d25d","unresolved":true,"context_lines":[{"line_number":1221,"context_line":"        return self"},{"line_number":1222,"context_line":""},{"line_number":1223,"context_line":""},{"line_number":1224,"context_line":"LabelModes \u003d ["},{"line_number":1225,"context_line":"    \u0027disabled\u0027,"},{"line_number":1226,"context_line":"    \u0027graphite\u0027,"},{"line_number":1227,"context_line":"    \u0027influxdb\u0027,"}],"source_content_type":"text/x-python","patch_set":2,"id":"75e24fbf_734ce2cb","line":1224,"in_reply_to":"dcc0a855_80359a37","updated":"2023-10-12 21:54:51.000000000","message":"Well, what I *really* wanted to reach for was an `enum`, which seems to generally by PascalCase like classes; see `http.server.HTTPStatus` for example. Seemed silly to pull in [the extra dep](https://pypi.org/project/enum/) just for py2, though.","commit_id":"78b986e98e6856a898a1449538860e2de4a4d644"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"746222e60c4ea2faf8eac1c23c6c0cda8bb53fba","unresolved":true,"context_lines":[{"line_number":1246,"context_line":"        self.random \u003d random"},{"line_number":1247,"context_line":"        self.logger \u003d logger"},{"line_number":1248,"context_line":"        self.label_mode \u003d label_mode"},{"line_number":1249,"context_line":"        self.build_line \u003d getattr(self, \u0027build_line_\u0027 + self.label_mode, None)"},{"line_number":1250,"context_line":"        self.default_disaggregate_prefix \u003d default_disaggregate_prefix or ()"},{"line_number":1251,"context_line":""},{"line_number":1252,"context_line":"        # Determine if host is IPv4 or IPv6"}],"source_content_type":"text/x-python","patch_set":2,"id":"2efa6568_c05fe20f","line":1249,"updated":"2023-07-21 17:04:14.000000000","message":"or, could LabelModes be a class attribute dict that maps modes to functions?","commit_id":"78b986e98e6856a898a1449538860e2de4a4d644"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"746222e60c4ea2faf8eac1c23c6c0cda8bb53fba","unresolved":true,"context_lines":[{"line_number":1285,"context_line":"            self._target \u003d (host, port)"},{"line_number":1286,"context_line":""},{"line_number":1287,"context_line":"    @property"},{"line_number":1288,"context_line":"    def statsd_labeling_enabled(self):"},{"line_number":1289,"context_line":"        return self.label_mode in LabelModes and \\"},{"line_number":1290,"context_line":"            self.label_mode !\u003d \u0027disabled\u0027"},{"line_number":1291,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"30bd2631_31ef7473","line":1288,"updated":"2023-07-21 17:04:14.000000000","message":"seems odd that this StatsdClient method name needs to start with `statsd_` - perhaps because it gets proxied by a LogAdapter and so needs to sound like its to do with statsd?\n\nSee my comment below about the opportunity to move away from the LogAdapter acting as a proxy for the StatsdClient.","commit_id":"78b986e98e6856a898a1449538860e2de4a4d644"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"746222e60c4ea2faf8eac1c23c6c0cda8bb53fba","unresolved":true,"context_lines":[{"line_number":1286,"context_line":""},{"line_number":1287,"context_line":"    @property"},{"line_number":1288,"context_line":"    def statsd_labeling_enabled(self):"},{"line_number":1289,"context_line":"        return self.label_mode in LabelModes and \\"},{"line_number":1290,"context_line":"            self.label_mode !\u003d \u0027disabled\u0027"},{"line_number":1291,"context_line":""},{"line_number":1292,"context_line":"    def _set_prefix(self, tail_prefix):"}],"source_content_type":"text/x-python","patch_set":2,"id":"952e0a43_03ffac36","line":1289,"range":{"start_line":1289,"start_character":15,"end_line":1289,"end_character":44},"updated":"2023-07-21 17:04:14.000000000","message":"I think I saw that label_mode had already been defaulted to \u0027disabled\u0027 if not in LabelModes","commit_id":"78b986e98e6856a898a1449538860e2de4a4d644"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"746222e60c4ea2faf8eac1c23c6c0cda8bb53fba","unresolved":true,"context_lines":[{"line_number":1384,"context_line":"        if sample_rate is None:"},{"line_number":1385,"context_line":"            sample_rate \u003d self._default_sample_rate"},{"line_number":1386,"context_line":"        sample_rate \u003d sample_rate * self._sample_rate_factor"},{"line_number":1387,"context_line":"        sample_rate \u003d 1"},{"line_number":1388,"context_line":"        if sample_rate \u003c 1 and self.random() \u003e\u003d sample_rate:"},{"line_number":1389,"context_line":"            return"},{"line_number":1390,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"9ca61053_29bd9d59","line":1387,"range":{"start_line":1387,"start_character":8,"end_line":1387,"end_character":23},"updated":"2023-07-21 17:04:14.000000000","message":"looks like maybe left over from dev/debug","commit_id":"78b986e98e6856a898a1449538860e2de4a4d644"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"f4852deeb01f23e70d1c20f5fa08e39fd746d25d","unresolved":true,"context_lines":[{"line_number":1384,"context_line":"        if sample_rate is None:"},{"line_number":1385,"context_line":"            sample_rate \u003d self._default_sample_rate"},{"line_number":1386,"context_line":"        sample_rate \u003d sample_rate * self._sample_rate_factor"},{"line_number":1387,"context_line":"        sample_rate \u003d 1"},{"line_number":1388,"context_line":"        if sample_rate \u003c 1 and self.random() \u003e\u003d sample_rate:"},{"line_number":1389,"context_line":"            return"},{"line_number":1390,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"d56507ea_619c7b11","line":1387,"range":{"start_line":1387,"start_character":8,"end_line":1387,"end_character":23},"in_reply_to":"9ca61053_29bd9d59","updated":"2023-10-12 21:54:51.000000000","message":"Most assuredly. There are a few things I did here to make this intentionally emit as many stats as possible and ensure they always got labeled. They would, of course, have to be backed out before this could possibly merge.","commit_id":"78b986e98e6856a898a1449538860e2de4a4d644"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"746222e60c4ea2faf8eac1c23c6c0cda8bb53fba","unresolved":true,"context_lines":[{"line_number":1390,"context_line":""},{"line_number":1391,"context_line":"        if legacy_format is None:"},{"line_number":1392,"context_line":"            #if labels:"},{"line_number":1393,"context_line":"            raise RuntimeError(\u0027All labeled metrics should have a legacy format\u0027)"},{"line_number":1394,"context_line":"            # else:"},{"line_number":1395,"context_line":"            # legacy_format \u003d m_name"},{"line_number":1396,"context_line":"        elif not legacy_format:"}],"source_content_type":"text/x-python","patch_set":2,"id":"480d4e8a_b588a445","line":1393,"range":{"start_line":1393,"start_character":12,"end_line":1393,"end_character":30},"updated":"2023-07-21 17:04:14.000000000","message":"this feels too brittle: arguably, the \"system\" should be able to continue despite a bad call into metrics","commit_id":"78b986e98e6856a898a1449538860e2de4a4d644"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"f4852deeb01f23e70d1c20f5fa08e39fd746d25d","unresolved":true,"context_lines":[{"line_number":1390,"context_line":""},{"line_number":1391,"context_line":"        if legacy_format is None:"},{"line_number":1392,"context_line":"            #if labels:"},{"line_number":1393,"context_line":"            raise RuntimeError(\u0027All labeled metrics should have a legacy format\u0027)"},{"line_number":1394,"context_line":"            # else:"},{"line_number":1395,"context_line":"            # legacy_format \u003d m_name"},{"line_number":1396,"context_line":"        elif not legacy_format:"}],"source_content_type":"text/x-python","patch_set":2,"id":"9cdef05e_076da433","line":1393,"range":{"start_line":1393,"start_character":12,"end_line":1393,"end_character":30},"in_reply_to":"480d4e8a_b588a445","updated":"2023-10-12 21:54:51.000000000","message":"See above; the brittleness was intentional for finding missed labeling opportunities -- you can even see the skeleton of what I was ultimately planning to do in the commented-out code.","commit_id":"78b986e98e6856a898a1449538860e2de4a4d644"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"746222e60c4ea2faf8eac1c23c6c0cda8bb53fba","unresolved":true,"context_lines":[{"line_number":1785,"context_line":"                return func(self.logger.statsd_client, *a, **kw)"},{"line_number":1786,"context_line":"        return wrapped"},{"line_number":1787,"context_line":""},{"line_number":1788,"context_line":"    statsd_labeling_enabled \u003d statsd_delegate(\u0027statsd_labeling_enabled\u0027)"},{"line_number":1789,"context_line":"    update_stats \u003d statsd_delegate(\u0027update_stats\u0027)"},{"line_number":1790,"context_line":"    increment \u003d statsd_delegate(\u0027increment\u0027)"},{"line_number":1791,"context_line":"    decrement \u003d statsd_delegate(\u0027decrement\u0027)"}],"source_content_type":"text/x-python","patch_set":2,"id":"142c9212_2a974c4c","line":1788,"updated":"2023-07-21 17:04:14.000000000","message":"since we\u0027re touching every call site for these methods (I think?), what do you think about moving away from the method forwarding and just have a `statsd` attribute on the logger interface , so we\u0027d call:\n\n```\nlogger.statsd.labeling_enabled\nlogger.statsd.increment\netc.\n```","commit_id":"78b986e98e6856a898a1449538860e2de4a4d644"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"f4852deeb01f23e70d1c20f5fa08e39fd746d25d","unresolved":true,"context_lines":[{"line_number":1785,"context_line":"                return func(self.logger.statsd_client, *a, **kw)"},{"line_number":1786,"context_line":"        return wrapped"},{"line_number":1787,"context_line":""},{"line_number":1788,"context_line":"    statsd_labeling_enabled \u003d statsd_delegate(\u0027statsd_labeling_enabled\u0027)"},{"line_number":1789,"context_line":"    update_stats \u003d statsd_delegate(\u0027update_stats\u0027)"},{"line_number":1790,"context_line":"    increment \u003d statsd_delegate(\u0027increment\u0027)"},{"line_number":1791,"context_line":"    decrement \u003d statsd_delegate(\u0027decrement\u0027)"}],"source_content_type":"text/x-python","patch_set":2,"id":"52ac2880_c4dfa0f4","line":1788,"in_reply_to":"142c9212_2a974c4c","updated":"2023-10-12 21:54:51.000000000","message":"I mean, we can do that today, no? It\u0027d be `statsd_client`, but totally doable.\n\nIf we want to make that the way we do it, I might prefer we rename it, though -- longer term, I could see us wanting to swap out transport for [OTLP-over-gRPC](https://opentelemetry.io/docs/specs/otlp/) or something -- maybe even have WSGI manager processes spin off their own thread/process to expose a prom endpoint that collects metrics from all the workers.\n\nI\u0027m still not sure I see the benefit of it, though, unless it\u0027s part of some longer-term plan to stop bundling the metrics client from the logger; even then, I\u0027m not sure I understand the appeal of replacing the\n```\nself.logger.increment(...)\n```\ncalls with\n```\nself.logger.statsd_client.increment(...)\n```\nnow, just so we can replace them again with\n```\nself.statsd_client.increment(...)\n```\nin the end...","commit_id":"78b986e98e6856a898a1449538860e2de4a4d644"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"746222e60c4ea2faf8eac1c23c6c0cda8bb53fba","unresolved":true,"context_lines":[{"line_number":1979,"context_line":"        if statsd_tail_prefix is None:"},{"line_number":1980,"context_line":"            statsd_tail_prefix \u003d name"},{"line_number":1981,"context_line":"        label_mode \u003d conf.get(\u0027log_statsd_label_mode\u0027, \u0027\u0027).lower()"},{"line_number":1982,"context_line":"        if label_mode not in LabelModes:"},{"line_number":1983,"context_line":"            label_mode \u003d \u0027disabled\u0027"},{"line_number":1984,"context_line":"        statsd_client \u003d StatsdClient(statsd_host, statsd_port, base_prefix,"},{"line_number":1985,"context_line":"                                     statsd_tail_prefix, default_sample_rate,"},{"line_number":1986,"context_line":"                                     sample_rate_factor, logger\u003dlogger,"}],"source_content_type":"text/x-python","patch_set":2,"id":"f7b96f6d_24d8378d","line":1983,"range":{"start_line":1982,"start_character":8,"end_line":1983,"end_character":35},"updated":"2023-07-21 17:04:14.000000000","message":"This defaulting feels like it should be encapsulated in the `StatsdClient.__init__`","commit_id":"78b986e98e6856a898a1449538860e2de4a4d644"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"f4852deeb01f23e70d1c20f5fa08e39fd746d25d","unresolved":true,"context_lines":[{"line_number":1979,"context_line":"        if statsd_tail_prefix is None:"},{"line_number":1980,"context_line":"            statsd_tail_prefix \u003d name"},{"line_number":1981,"context_line":"        label_mode \u003d conf.get(\u0027log_statsd_label_mode\u0027, \u0027\u0027).lower()"},{"line_number":1982,"context_line":"        if label_mode not in LabelModes:"},{"line_number":1983,"context_line":"            label_mode \u003d \u0027disabled\u0027"},{"line_number":1984,"context_line":"        statsd_client \u003d StatsdClient(statsd_host, statsd_port, base_prefix,"},{"line_number":1985,"context_line":"                                     statsd_tail_prefix, default_sample_rate,"},{"line_number":1986,"context_line":"                                     sample_rate_factor, logger\u003dlogger,"}],"source_content_type":"text/x-python","patch_set":2,"id":"283f17ec_fea71000","line":1983,"range":{"start_line":1982,"start_character":8,"end_line":1983,"end_character":35},"in_reply_to":"f7b96f6d_24d8378d","updated":"2023-10-12 21:54:51.000000000","message":"IDK -- at that point it seems like it might be better to `raise ValueError`","commit_id":"78b986e98e6856a898a1449538860e2de4a4d644"}]}