)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"be2aade3eed4e66b8d6eb213f05d8346b6075079","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"e5dc9816_1bb0113b","updated":"2023-04-21 14:27:31.000000000","message":"I think this is a good direction to go for more insight without revealing anything sensitive","commit_id":"4e5a7fbef0a9328c1fe70530bd83a6f0c8469a9c"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"9969010f4088c311e6a8003ce17ea52b2424a9e6","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":5,"id":"f73074e0_f7be8918","updated":"2023-04-24 14:49:17.000000000","message":"LGTM, just a query about the message format","commit_id":"7834edf3dd99b0b588a95975ed75cbd48b3fe1ee"},{"author":{"_account_id":34930,"name":"Jianjian Huo","email":"jhuo@nvidia.com","username":"jhuo"},"change_message_id":"5529418d607cd64bbeb69e13482a0e707dadeca3","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":6,"id":"a0c081fd_a9df24ad","updated":"2023-04-28 22:48:34.000000000","message":"recheck","commit_id":"365b208bc3d685028b21c41998a120b636a86ede"},{"author":{"_account_id":34930,"name":"Jianjian Huo","email":"jhuo@nvidia.com","username":"jhuo"},"change_message_id":"a93052ece553c88d335ca88f845f1b256696b017","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":6,"id":"a762b3ff_6aa479cd","updated":"2023-04-28 19:21:26.000000000","message":"recheck","commit_id":"365b208bc3d685028b21c41998a120b636a86ede"},{"author":{"_account_id":34930,"name":"Jianjian Huo","email":"jhuo@nvidia.com","username":"jhuo"},"change_message_id":"a09dda0e0e8a6c0040fee0f1b4af59bfc8d9baec","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":6,"id":"32f2f8d4_904a578b","updated":"2023-04-25 18:02:42.000000000","message":"recheck\nsporadic test issue, \"503 Service Unavailable\" during GET.","commit_id":"365b208bc3d685028b21c41998a120b636a86ede"},{"author":{"_account_id":7233,"name":"Matthew Oliver","email":"matt@oliver.net.au","username":"mattoliverau"},"change_message_id":"2bb2bd712ea816bab207fe703f4007adc96fc2f4","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":7,"id":"24224c14_6496d5f1","updated":"2023-05-10 06:56:40.000000000","message":"Looks good. The only thing I can think of is we need to be sure if any data can leak from this.. but it\u0027s just logging the key (not the value). This will link account names, but we already log those, so it\u0027s fine.\n\nThere is a bunch of repitition having to get the prefix and then passing that around.. I guess we \"could\" wrap the key up into some class that can have a prefix and a hash built in.. but that seems overkill and way outta scope. Maybe if we every pull or use more key related data it would be something to think about then.","commit_id":"9fb860880d755291377c28702679c4d5deb55a6f"},{"author":{"_account_id":1179,"name":"Clay Gerrard","email":"clay.gerrard@gmail.com","username":"clay-gerrard"},"change_message_id":"fbf585b5e8c7f9006515194c02004d059ac7a901","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":7,"id":"0d417c51_58ae8717","updated":"2023-05-17 16:31:38.000000000","message":"i\u0027m ok with the plumbing for key_prefix for logging - I like the idea of using a named tuple or something to pass around a \"key\" object that has key, key_prefix, key_hash attributes or something.","commit_id":"9fb860880d755291377c28702679c4d5deb55a6f"},{"author":{"_account_id":34930,"name":"Jianjian Huo","email":"jhuo@nvidia.com","username":"jhuo"},"change_message_id":"582eef576a8dc4700e4252cf506575bea1fcc8e6","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":7,"id":"bcbde004_dddc1a40","in_reply_to":"24224c14_6496d5f1","updated":"2023-05-10 20:35:49.000000000","message":"you are absolutely correct to think about any potential data leakages. As a matter of fact, the memcache key of auth token is something like \"auth_reseller_name/token/X58E34EL2SDFLEY3\" which has token in the end, that\u0027s why \"get_key_prefix\" is implemented to remove the string after the last \"/\".","commit_id":"9fb860880d755291377c28702679c4d5deb55a6f"}],"swift/common/memcached.py":[{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"b34fbd21725237595dd97a4bddd404d74e5c1042","unresolved":true,"context_lines":[{"line_number":226,"context_line":"            self.logger.error("},{"line_number":227,"context_line":"                \"Error %(action)s to memcached: %(server)s: %(err)s\""},{"line_number":228,"context_line":"                \": with key %(key)s\","},{"line_number":229,"context_line":"                {\u0027action\u0027: action, \u0027server\u0027: server, \u0027err\u0027: e, \u0027key\u0027: key})"},{"line_number":230,"context_line":"        else:"},{"line_number":231,"context_line":"            self.logger.exception(\"Error %(action)s to memcached: %(server)s\""},{"line_number":232,"context_line":"                                  \": with key %(key)s\","}],"source_content_type":"text/x-python","patch_set":2,"id":"cf0a4f1f_632e20ad","line":229,"updated":"2023-04-20 15:46:03.000000000","message":"we need to be careful when logging the key because it may contain a secret, for example an auth token:\n\nhttps://github.com/openstack/swift/blob/94226bdd39a64415e456057740add0ee6190d033/swift/common/middleware/tempauth.py#L731-L734\n\nI think it would be better to either: \n\n- obscure keys (for example https://github.com/openstack/swift/blob/1831658b338d628f18a2e3357e7b7f5d34ec79df/swift/common/middleware/proxy_logging.py#L202-L205)\n\nOR\n\n- leave it for callers to catch the MemcacheConnectionError and log.","commit_id":"2a46355f87e9be5c5b168c55818f64f8d18aafea"},{"author":{"_account_id":34930,"name":"Jianjian Huo","email":"jhuo@nvidia.com","username":"jhuo"},"change_message_id":"c9a7595b42f02a8c68f8dd7df30570ca75475e1b","unresolved":false,"context_lines":[{"line_number":226,"context_line":"            self.logger.error("},{"line_number":227,"context_line":"                \"Error %(action)s to memcached: %(server)s: %(err)s\""},{"line_number":228,"context_line":"                \": with key %(key)s\","},{"line_number":229,"context_line":"                {\u0027action\u0027: action, \u0027server\u0027: server, \u0027err\u0027: e, \u0027key\u0027: key})"},{"line_number":230,"context_line":"        else:"},{"line_number":231,"context_line":"            self.logger.exception(\"Error %(action)s to memcached: %(server)s\""},{"line_number":232,"context_line":"                                  \": with key %(key)s\","}],"source_content_type":"text/x-python","patch_set":2,"id":"747ba9e6_c80124a8","line":229,"in_reply_to":"9744acd8_f9ccd0a4","updated":"2023-04-21 02:30:11.000000000","message":"thanks for all your suggestions! I think it\u0027s great we can remove the sensitive part of the key without calling the middleware function, I would prefer \"key.rsplit(\u0027/\u0027, 1)[0]\".","commit_id":"2a46355f87e9be5c5b168c55818f64f8d18aafea"},{"author":{"_account_id":15343,"name":"Tim Burke","email":"tburke@nvidia.com","username":"tburke"},"change_message_id":"59fb28571d8d4fd57e4fdf21290b0ae414f12cdd","unresolved":true,"context_lines":[{"line_number":226,"context_line":"            self.logger.error("},{"line_number":227,"context_line":"                \"Error %(action)s to memcached: %(server)s: %(err)s\""},{"line_number":228,"context_line":"                \": with key %(key)s\","},{"line_number":229,"context_line":"                {\u0027action\u0027: action, \u0027server\u0027: server, \u0027err\u0027: e, \u0027key\u0027: key})"},{"line_number":230,"context_line":"        else:"},{"line_number":231,"context_line":"            self.logger.exception(\"Error %(action)s to memcached: %(server)s\""},{"line_number":232,"context_line":"                                  \": with key %(key)s\","}],"source_content_type":"text/x-python","patch_set":2,"id":"9744acd8_f9ccd0a4","line":229,"in_reply_to":"cf0a4f1f_632e20ad","updated":"2023-04-20 18:45:58.000000000","message":"Might be enough to just log something like\n\n    key_prefix \u003d key.rsplit(\u0027/\u0027, 1)[0]\n\nor even\n\n    key_prefix \u003d \u0027/\u0027.join(key.split(\u0027/\u0027, 2)[:2]\n\nI think most middlewares that touch memcache use a /-delimited format that would be at least somewhat amenable to that approach.\n\nHow important is it to know *exactly* which key had the error? Is it enough to just know the general bucket that the key falls into?\n\nI\u0027m fairly certain we **don\u0027t** need the middle-ground provided by something like `obscure_sensitive()`, where _given a particular key_ we can say with some confidence that the key was or wasn\u0027t tripping errors. If we _did_ want that, we could just use the hashed key.\n\n---\n\nBut maybe it would be better to update the get/set/etc APIs to take both a `key` and a `bucket` arg, so callers can be explicit about where that separation lies. Could also be a step towards using `{bucket}/{hashed(key)}` instead of `hashed({bucket}/{key})`, so we could use something like mcrouter\u0027s prefix-routing.","commit_id":"2a46355f87e9be5c5b168c55818f64f8d18aafea"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"be2aade3eed4e66b8d6eb213f05d8346b6075079","unresolved":true,"context_lines":[{"line_number":119,"context_line":""},{"line_number":120,"context_line":"# get the prefix of a user provided memcache key, all current usages within"},{"line_number":121,"context_line":"# swift are using prefix, such as \"shard-updating-v2\", \"nvratelimit\" and etc."},{"line_number":122,"context_line":"def get_key_prefix(key):"},{"line_number":123,"context_line":"    return key.rsplit(\u0027/\u0027, 1)[0]"},{"line_number":124,"context_line":""},{"line_number":125,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"de549c89_2a70a236","line":122,"updated":"2023-04-21 14:27:31.000000000","message":"a unit test for the function would be good","commit_id":"4e5a7fbef0a9328c1fe70530bd83a6f0c8469a9c"},{"author":{"_account_id":34930,"name":"Jianjian Huo","email":"jhuo@nvidia.com","username":"jhuo"},"change_message_id":"85a23e12692be9356f311169fea86ae08f9da842","unresolved":false,"context_lines":[{"line_number":119,"context_line":""},{"line_number":120,"context_line":"# get the prefix of a user provided memcache key, all current usages within"},{"line_number":121,"context_line":"# swift are using prefix, such as \"shard-updating-v2\", \"nvratelimit\" and etc."},{"line_number":122,"context_line":"def get_key_prefix(key):"},{"line_number":123,"context_line":"    return key.rsplit(\u0027/\u0027, 1)[0]"},{"line_number":124,"context_line":""},{"line_number":125,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"d16d2600_b40ac311","line":122,"in_reply_to":"de549c89_2a70a236","updated":"2023-04-21 23:38:48.000000000","message":"Done","commit_id":"4e5a7fbef0a9328c1fe70530bd83a6f0c8469a9c"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"be2aade3eed4e66b8d6eb213f05d8346b6075079","unresolved":true,"context_lines":[{"line_number":120,"context_line":"# get the prefix of a user provided memcache key, all current usages within"},{"line_number":121,"context_line":"# swift are using prefix, such as \"shard-updating-v2\", \"nvratelimit\" and etc."},{"line_number":122,"context_line":"def get_key_prefix(key):"},{"line_number":123,"context_line":"    return key.rsplit(\u0027/\u0027, 1)[0]"},{"line_number":124,"context_line":""},{"line_number":125,"context_line":""},{"line_number":126,"context_line":"class MemcacheConnectionError(Exception):"}],"source_content_type":"text/x-python","patch_set":4,"id":"11912a34_c1a97eb8","line":123,"range":{"start_line":123,"start_character":15,"end_line":123,"end_character":21},"updated":"2023-04-21 14:27:31.000000000","message":"did you intend to use rsplit?\n\n  \u003e\u003e\u003e \u0027a/b/c\u0027.rsplit(\u0027/\u0027, 1)[0]\n  \u0027a/b\u0027\n \nI think I\u0027d be more comfortable limiting the prefix to the first element only (i.e. using split), just so I don\u0027t have to worry that someone might be using keys like \u0027myauth/\u003csecret\u003e\u0027","commit_id":"4e5a7fbef0a9328c1fe70530bd83a6f0c8469a9c"},{"author":{"_account_id":34930,"name":"Jianjian Huo","email":"jhuo@nvidia.com","username":"jhuo"},"change_message_id":"85a23e12692be9356f311169fea86ae08f9da842","unresolved":false,"context_lines":[{"line_number":120,"context_line":"# get the prefix of a user provided memcache key, all current usages within"},{"line_number":121,"context_line":"# swift are using prefix, such as \"shard-updating-v2\", \"nvratelimit\" and etc."},{"line_number":122,"context_line":"def get_key_prefix(key):"},{"line_number":123,"context_line":"    return key.rsplit(\u0027/\u0027, 1)[0]"},{"line_number":124,"context_line":""},{"line_number":125,"context_line":""},{"line_number":126,"context_line":"class MemcacheConnectionError(Exception):"}],"source_content_type":"text/x-python","patch_set":4,"id":"dde62631_6bee9906","line":123,"range":{"start_line":123,"start_character":15,"end_line":123,"end_character":21},"in_reply_to":"0eae7540_090bffa5","updated":"2023-04-21 23:38:48.000000000","message":"Ack","commit_id":"4e5a7fbef0a9328c1fe70530bd83a6f0c8469a9c"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"b8731292249e65178055a83d9abc6b1afeae02dd","unresolved":true,"context_lines":[{"line_number":120,"context_line":"# get the prefix of a user provided memcache key, all current usages within"},{"line_number":121,"context_line":"# swift are using prefix, such as \"shard-updating-v2\", \"nvratelimit\" and etc."},{"line_number":122,"context_line":"def get_key_prefix(key):"},{"line_number":123,"context_line":"    return key.rsplit(\u0027/\u0027, 1)[0]"},{"line_number":124,"context_line":""},{"line_number":125,"context_line":""},{"line_number":126,"context_line":"class MemcacheConnectionError(Exception):"}],"source_content_type":"text/x-python","patch_set":4,"id":"0eae7540_090bffa5","line":123,"range":{"start_line":123,"start_character":15,"end_line":123,"end_character":21},"in_reply_to":"11912a34_c1a97eb8","updated":"2023-04-21 16:36:56.000000000","message":"oh but of course,  \u0027myauth/\u003csecret\u003e\u0027 would render as just \u0027myauth\u0027\n\n#ignoreme","commit_id":"4e5a7fbef0a9328c1fe70530bd83a6f0c8469a9c"}],"test/unit/common/test_memcached.py":[{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"be2aade3eed4e66b8d6eb213f05d8346b6075079","unresolved":true,"context_lines":[{"line_number":994,"context_line":"            # try to get connect and no connection found"},{"line_number":995,"context_line":"            # so it will result in StopIteration"},{"line_number":996,"context_line":"            conn_generator \u003d memcache_client._get_conns("},{"line_number":997,"context_line":"                \u0027key\u0027, md5hash(b\u0027key\u0027))"},{"line_number":998,"context_line":"            with self.assertRaises(StopIteration):"},{"line_number":999,"context_line":"                next(conn_generator)"},{"line_number":1000,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"a0164169_13e2692e","line":997,"updated":"2023-04-21 14:27:31.000000000","message":"it wasn\u0027t immediately obvious to me that any of these tests cover a key with a \u0027/\u0027 delimiter i.e. in the tests the prefix is always the same as the entire key.\n\nNor do there seem to be any tests covering the \u0027Error talking to memcached\u0027 scenarios - presumably no existing test coverage ? :/","commit_id":"4e5a7fbef0a9328c1fe70530bd83a6f0c8469a9c"},{"author":{"_account_id":34930,"name":"Jianjian Huo","email":"jhuo@nvidia.com","username":"jhuo"},"change_message_id":"85a23e12692be9356f311169fea86ae08f9da842","unresolved":false,"context_lines":[{"line_number":994,"context_line":"            # try to get connect and no connection found"},{"line_number":995,"context_line":"            # so it will result in StopIteration"},{"line_number":996,"context_line":"            conn_generator \u003d memcache_client._get_conns("},{"line_number":997,"context_line":"                \u0027key\u0027, md5hash(b\u0027key\u0027))"},{"line_number":998,"context_line":"            with self.assertRaises(StopIteration):"},{"line_number":999,"context_line":"                next(conn_generator)"},{"line_number":1000,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"75af43d6_d7cc7a1b","line":997,"in_reply_to":"a0164169_13e2692e","updated":"2023-04-21 23:38:48.000000000","message":"added more tests. \u0027Error talking to memcached\u0027 is covered in \u0027test_error_raising\u0027 of this patch, \u0027Timeout talking to memcached\u0027 is covered in the following metrics patch.","commit_id":"4e5a7fbef0a9328c1fe70530bd83a6f0c8469a9c"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"9969010f4088c311e6a8003ce17ea52b2424a9e6","unresolved":true,"context_lines":[{"line_number":210,"context_line":"            \"shard-listing-v2/accout\")"},{"line_number":211,"context_line":"        self.assertEqual("},{"line_number":212,"context_line":"            get_key_prefix(\"auth_reseller_name/token/X58E34EL2SDFLEY3\"),"},{"line_number":213,"context_line":"            \"auth_reseller_name/token\")"},{"line_number":214,"context_line":""},{"line_number":215,"context_line":"    def test_logger_kwarg(self):"},{"line_number":216,"context_line":"        server_socket \u003d \u0027%s:%s\u0027 % (\u0027[::1]\u0027, 11211)"}],"source_content_type":"text/x-python","patch_set":5,"id":"723bec2a_798c3609","line":213,"updated":"2023-04-24 14:49:17.000000000","message":"thanks\n\nnit: for completeness perhaps add a case with no \u0027/\u0027 delimiter i.e. prefix \u003d\u003d key","commit_id":"7834edf3dd99b0b588a95975ed75cbd48b3fe1ee"},{"author":{"_account_id":34930,"name":"Jianjian Huo","email":"jhuo@nvidia.com","username":"jhuo"},"change_message_id":"18a9e641473b0177f65f01115f2f12d14c63483f","unresolved":false,"context_lines":[{"line_number":210,"context_line":"            \"shard-listing-v2/accout\")"},{"line_number":211,"context_line":"        self.assertEqual("},{"line_number":212,"context_line":"            get_key_prefix(\"auth_reseller_name/token/X58E34EL2SDFLEY3\"),"},{"line_number":213,"context_line":"            \"auth_reseller_name/token\")"},{"line_number":214,"context_line":""},{"line_number":215,"context_line":"    def test_logger_kwarg(self):"},{"line_number":216,"context_line":"        server_socket \u003d \u0027%s:%s\u0027 % (\u0027[::1]\u0027, 11211)"}],"source_content_type":"text/x-python","patch_set":5,"id":"f5e6894c_5bc5ec4c","line":213,"in_reply_to":"723bec2a_798c3609","updated":"2023-04-25 04:02:32.000000000","message":"Done","commit_id":"7834edf3dd99b0b588a95975ed75cbd48b3fe1ee"},{"author":{"_account_id":7847,"name":"Alistair Coles","email":"alistairncoles@gmail.com","username":"acoles"},"change_message_id":"9969010f4088c311e6a8003ce17ea52b2424a9e6","unresolved":true,"context_lines":[{"line_number":665,"context_line":"        self.assertEqual(self.logger.get_lines_for_level(\u0027error\u0027), ["},{"line_number":666,"context_line":"            \u0027Error talking to memcached: 1.2.3.4:11211: \u0027"},{"line_number":667,"context_line":"            \u0027[Errno 32] Broken pipe: with key_prefix shard-updating-v2/acc\u0027,"},{"line_number":668,"context_line":"        ])"},{"line_number":669,"context_line":"        self.logger.clear()"},{"line_number":670,"context_line":""},{"line_number":671,"context_line":"        # ...but default is no exception"}],"source_content_type":"text/x-python","patch_set":5,"id":"4752e22e_5e85f511","line":668,"updated":"2023-04-24 14:49:17.000000000","message":"nit: I didn\u0027t notice this in last review, but the message format makes it look like the \u0027with key_prefix...\u0027 part is part of the exception message.\n\n  [Errno 32] Broken pipe: with key_prefix shard-updating-v2/acc\n\nWould it be better to keep the exception message as the final part of the log message, something like \n\n```\n\u0027Error talking to memcached: 1.2.3.4:11211: with key_prefix shard-updating-v2/acc: [Errno 32] Broken pipe:\u0027\n```","commit_id":"7834edf3dd99b0b588a95975ed75cbd48b3fe1ee"},{"author":{"_account_id":34930,"name":"Jianjian Huo","email":"jhuo@nvidia.com","username":"jhuo"},"change_message_id":"18a9e641473b0177f65f01115f2f12d14c63483f","unresolved":false,"context_lines":[{"line_number":665,"context_line":"        self.assertEqual(self.logger.get_lines_for_level(\u0027error\u0027), ["},{"line_number":666,"context_line":"            \u0027Error talking to memcached: 1.2.3.4:11211: \u0027"},{"line_number":667,"context_line":"            \u0027[Errno 32] Broken pipe: with key_prefix shard-updating-v2/acc\u0027,"},{"line_number":668,"context_line":"        ])"},{"line_number":669,"context_line":"        self.logger.clear()"},{"line_number":670,"context_line":""},{"line_number":671,"context_line":"        # ...but default is no exception"}],"source_content_type":"text/x-python","patch_set":5,"id":"2f7f7995_c804e144","line":668,"in_reply_to":"4752e22e_5e85f511","updated":"2023-04-25 04:02:32.000000000","message":"Done","commit_id":"7834edf3dd99b0b588a95975ed75cbd48b3fe1ee"}]}