)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":31083,"name":"Benjamin Schanzel","email":"benjamin.schanzel@bmw.de","username":"benjamin.schanzel"},"change_message_id":"6f873c776fe92f62f4900583bc53a94eb1314173","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":4,"id":"79b3c4d0_310613cf","updated":"2024-02-29 16:03:44.000000000","message":"A provider can have multiple pools, each with potentially overlapping sets of supported_labels. In such a case, I think we would be counting the same request(s) multiple times per provider.\nSo I think we should sum them up not per pool.provider_name but a tuple (pool.id, pool.provider_name) and have the metric name include the pool.id. So any consumer of the metric can do the desired reduction (max, min, sum, ... by provider_name, pool_id, ...) .\n\nThis would look something like so\n\n```\nfor pool in zk_conn.getRegisteredProviders():\n  key \u003d (pool.id, pool.provider_name)\n  if key not in provider_requests:\n    provider_requests[key] \u003d 0\n  ...\n  for (pool_id, provider_name), requests_count in provider_requests.items():\n    metric_name \u003d (\"nodepool.\"\n                   \"provider.\"\n                   f\"{provider_name}.\"\n                   f\"{pool_id}.\"\n                   \"handleable_requests\")\n    pipeline.gauge(metric_name, requests_count)\n```\n\nOr, to keep it simpler, we could also skip the provider_name here and sum only by the pool.id, as this is directly relatable to the provider anyway. But this would offload parsing provider_name to the metric consumer (so this would be simpler here but more complex on the consumer side).","commit_id":"dd2f4d02f222e6b7602dd5f4562b4ccec78c444d"},{"author":{"_account_id":36785,"name":"Samuel Jan Surovka","display_name":"Samuel Surovka","email":"samuel.surovka@bmw.de","username":"samuelsurovka"},"change_message_id":"1acea76f7c025f6bf6b04489f70cd8c1961f4f99","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"a4e07da1_182fb28b","updated":"2024-02-13 11:43:15.000000000","message":"recheck","commit_id":"dd2f4d02f222e6b7602dd5f4562b4ccec78c444d"},{"author":{"_account_id":36785,"name":"Samuel Jan Surovka","display_name":"Samuel Surovka","email":"samuel.surovka@bmw.de","username":"samuelsurovka"},"change_message_id":"d9cf3398535a3ae7170f0f788a62c906c1461e8e","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":4,"id":"baaac704_074f89db","updated":"2024-02-12 11:37:52.000000000","message":"recheck","commit_id":"dd2f4d02f222e6b7602dd5f4562b4ccec78c444d"},{"author":{"_account_id":36785,"name":"Samuel Jan Surovka","display_name":"Samuel Surovka","email":"samuel.surovka@bmw.de","username":"samuelsurovka"},"change_message_id":"debd35eb13c9bcd21d902e52a81eed551b0887da","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":5,"id":"52a4ecaf_778a36f4","updated":"2024-03-01 13:12:08.000000000","message":"recheck","commit_id":"50fbf8f7ac3fb5d27823d69aa85d59e8a8f7a23c"},{"author":{"_account_id":31083,"name":"Benjamin Schanzel","email":"benjamin.schanzel@bmw.de","username":"benjamin.schanzel"},"change_message_id":"0724c6b057c48ded7a09da3bd7632b0444dd8352","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":6,"id":"2847edfc_1bdb7dc5","updated":"2024-03-05 09:53:20.000000000","message":"I must revise my previous review comments, since the `pool.id` is quite a complicated string, containing periods in its hostname/fqdn part [1]. This makes it a rather bad qualifier for metrics. Sorry for missing that part.\nInstead, I\u0027d use the configured `pool.name` as a separate part of the metric. We don\u0027t have the `pool.name` available in the zk PoolComponent as of now, but adding this field there is rather trivial [2].\nI\u0027d rebase this change atop of [2] and substitute `pool.id` with `pool.name`.\n\n[1] https://opendev.org/zuul/nodepool/src/branch/master/nodepool/launcher.py#L79\n[2] https://review.opendev.org/c/zuul/nodepool/+/911052","commit_id":"b455e8201d141331cc42239ed11332e7c5de87cf"},{"author":{"_account_id":36785,"name":"Samuel Jan Surovka","display_name":"Samuel Surovka","email":"samuel.surovka@bmw.de","username":"samuelsurovka"},"change_message_id":"1b8385251031c5edef457a8685173b5b10e795b6","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":6,"id":"82f4e0ec_b0ef1e29","updated":"2024-03-05 09:06:43.000000000","message":"recheck","commit_id":"b455e8201d141331cc42239ed11332e7c5de87cf"},{"author":{"_account_id":36785,"name":"Samuel Jan Surovka","display_name":"Samuel Surovka","email":"samuel.surovka@bmw.de","username":"samuelsurovka"},"change_message_id":"bfada879f02d0af720d01e2592109f7697c994c1","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":8,"id":"aaacec8b_79e110ec","updated":"2024-03-05 13:45:33.000000000","message":"recheck","commit_id":"5a9e3644bfb5b738b26b8571223134fd1ed6fcc4"},{"author":{"_account_id":36785,"name":"Samuel Jan Surovka","display_name":"Samuel Surovka","email":"samuel.surovka@bmw.de","username":"samuelsurovka"},"change_message_id":"27de6d749ab903c0b2949af2f92985d7c4f117e4","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":10,"id":"723d1d1e_e1055c6c","updated":"2024-03-07 15:12:53.000000000","message":"Sorry, that was a mistake, should be fixed now.","commit_id":"77d91b8014089dda51bcf8f7e0a5475f720669e7"},{"author":{"_account_id":27582,"name":"Simon Westphahl","email":"simon.westphahl@bmw.de","username":"simon.westphahl"},"change_message_id":"923e0a0b305653970db6d0ee85886f3024a9eee5","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":11,"id":"be6323d1_d978053e","updated":"2024-07-09 05:07:23.000000000","message":"recheck","commit_id":"18c8e43c2a770a51afcbf69ad6da7f04834fd55e"}],"doc/source/operation.rst":[{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"ce4c763ed3e34e2d4bdbfb407da2969fe165e97a","unresolved":true,"context_lines":[{"line_number":633,"context_line":"   Number of leaked volumes removed automatically by Nodepool."},{"line_number":634,"context_line":""},{"line_number":635,"context_line":".. zuul:stat:: nodepool.provider.\u003cprovider_name\u003e.\u003cpool_name\u003e.handleable_requests"},{"line_number":636,"context_line":"   :type: gauge"},{"line_number":637,"context_line":""},{"line_number":638,"context_line":"   Number of open node requests a provider pool can handle."},{"line_number":639,"context_line":""}],"source_content_type":"text/x-rst","patch_set":8,"id":"b3c2e0d0_14d9b2aa","line":636,"updated":"2024-03-05 21:13:19.000000000","message":"The name seems a little ambiguous -- after all, if the provider could handle it, it would have done so already.  Providers don\u0027t delay handling requests, they fulfill them as quickly as possible.  But they can be constrained by the cloud quota, tenant quota, max_servers, or max_concurrency.\n\nI think a better term might be \"addressible\", though I\u0027m open to others.\n\nI\u0027m also a bit curious about how this could be used.  It seems it might be tempting to use it to gauge backlog for a given provider, but it needs to be considered in terms of the other data points (quota, max*, etc), not all of which are emitted as stats or readily comparable to this one.\n\nFinally, we should insert another element in the name:\n\n  nodepool.provider.\u003cprovider\u003e.pool.\u003cpool\u003e.x\n\nBecause otherwise we could have collisions with other metrics.  We\u0027ve learned that while that usually doesn\u0027t matter, there are some metrics combinations that don\u0027t work (specifically, subkeys under a guage).  So adding the extra \".pool\" ensures we don\u0027t collide in the future.  (Plus, even if it\u0027s technically okay, there\u0027s already confusion with the metric above.)\n\nPlease use \"provider\", and \"pool\" for the metasyntactic names for consistency with the other metric documentation.","commit_id":"5a9e3644bfb5b738b26b8571223134fd1ed6fcc4"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"88517329cf5bdbf1d65d0c9e2a138f29bcfae8ca","unresolved":false,"context_lines":[{"line_number":633,"context_line":"   Number of leaked volumes removed automatically by Nodepool."},{"line_number":634,"context_line":""},{"line_number":635,"context_line":".. zuul:stat:: nodepool.provider.\u003cprovider_name\u003e.\u003cpool_name\u003e.handleable_requests"},{"line_number":636,"context_line":"   :type: gauge"},{"line_number":637,"context_line":""},{"line_number":638,"context_line":"   Number of open node requests a provider pool can handle."},{"line_number":639,"context_line":""}],"source_content_type":"text/x-rst","patch_set":8,"id":"0e272df5_06b2d54b","line":636,"in_reply_to":"b3c2e0d0_14d9b2aa","updated":"2024-06-03 17:04:50.000000000","message":"Done","commit_id":"5a9e3644bfb5b738b26b8571223134fd1ed6fcc4"}],"nodepool/stats.py":[{"author":{"_account_id":16068,"name":"Tobias Henkel","email":"tobias.henkel@bmw.de","username":"tobias.henkel"},"change_message_id":"81a0780f3f25dda6b6ff50dca952a833cf39f519","unresolved":true,"context_lines":[{"line_number":188,"context_line":"        pipeline \u003d self._statsd.pipeline()"},{"line_number":189,"context_line":""},{"line_number":190,"context_line":"        pools \u003d zk_conn.getRegisteredPools()"},{"line_number":191,"context_line":"        node_requests \u003d zk_conn.getNodeRequests()"},{"line_number":192,"context_line":""},{"line_number":193,"context_line":"        provider_requests \u003d {}"},{"line_number":194,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"e4cda794_ace85dbb","line":191,"updated":"2024-02-09 14:19:45.000000000","message":"This fetches the node requests directly from zk which I think is not really needed for a gauge statistic. I think using nodeRequestIterator with cached ids would be good enough for the statistics and not hit zk at all.","commit_id":"888561532e32d4e4022ad47cf9c21bc9ab3a24bf"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"49718954e285afb5f1b45d66252b81bf6021d1db","unresolved":false,"context_lines":[{"line_number":188,"context_line":"        pipeline \u003d self._statsd.pipeline()"},{"line_number":189,"context_line":""},{"line_number":190,"context_line":"        pools \u003d zk_conn.getRegisteredPools()"},{"line_number":191,"context_line":"        node_requests \u003d zk_conn.getNodeRequests()"},{"line_number":192,"context_line":""},{"line_number":193,"context_line":"        provider_requests \u003d {}"},{"line_number":194,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"4314353f_ca3f6c45","line":191,"in_reply_to":"e4cda794_ace85dbb","updated":"2024-06-03 17:06:04.000000000","message":"Done","commit_id":"888561532e32d4e4022ad47cf9c21bc9ab3a24bf"},{"author":{"_account_id":16068,"name":"Tobias Henkel","email":"tobias.henkel@bmw.de","username":"tobias.henkel"},"change_message_id":"81a0780f3f25dda6b6ff50dca952a833cf39f519","unresolved":true,"context_lines":[{"line_number":208,"context_line":"                             \"provider.\""},{"line_number":209,"context_line":"                             f\"{provider_name}.\""},{"line_number":210,"context_line":"                             \"handleable_requests\")"},{"line_number":211,"context_line":"            print(provider_name)"},{"line_number":212,"context_line":"            pipeline.gauge(provider_name, requests_count)"},{"line_number":213,"context_line":"        pipeline.send()"}],"source_content_type":"text/x-python","patch_set":3,"id":"ab76e550_4ebb9d97","line":211,"updated":"2024-02-09 14:19:45.000000000","message":"The print should be removed.","commit_id":"888561532e32d4e4022ad47cf9c21bc9ab3a24bf"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"49718954e285afb5f1b45d66252b81bf6021d1db","unresolved":false,"context_lines":[{"line_number":208,"context_line":"                             \"provider.\""},{"line_number":209,"context_line":"                             f\"{provider_name}.\""},{"line_number":210,"context_line":"                             \"handleable_requests\")"},{"line_number":211,"context_line":"            print(provider_name)"},{"line_number":212,"context_line":"            pipeline.gauge(provider_name, requests_count)"},{"line_number":213,"context_line":"        pipeline.send()"}],"source_content_type":"text/x-python","patch_set":3,"id":"189cea6f_42f307ca","line":211,"in_reply_to":"ab76e550_4ebb9d97","updated":"2024-06-03 17:06:04.000000000","message":"Done","commit_id":"888561532e32d4e4022ad47cf9c21bc9ab3a24bf"},{"author":{"_account_id":31083,"name":"Benjamin Schanzel","email":"benjamin.schanzel@bmw.de","username":"benjamin.schanzel"},"change_message_id":"6f873c776fe92f62f4900583bc53a94eb1314173","unresolved":true,"context_lines":[{"line_number":181,"context_line":"                pipeline.gauge(key, lim)"},{"line_number":182,"context_line":"        pipeline.send()"},{"line_number":183,"context_line":""},{"line_number":184,"context_line":"    def updateRequestsByProvider(self, pools, node_iterator):"},{"line_number":185,"context_line":"        if not self._statsd:"},{"line_number":186,"context_line":"            return"},{"line_number":187,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"8c6e348e_2b55a3fb","line":184,"updated":"2024-02-29 16:03:44.000000000","message":"- To align this with the other method(s) here, I\u0027d call it `updateNodeRequestStats`\n- I\u0027d just pass in a `zk_conn` parameter (like in `updateNodeStats`) and get the required objects from zk here (`zk_conn.getRegisteredPools`, `zk_conn.nodeRequestIterator`)","commit_id":"dd2f4d02f222e6b7602dd5f4562b4ccec78c444d"},{"author":{"_account_id":31083,"name":"Benjamin Schanzel","email":"benjamin.schanzel@bmw.de","username":"benjamin.schanzel"},"change_message_id":"6f873c776fe92f62f4900583bc53a94eb1314173","unresolved":true,"context_lines":[{"line_number":187,"context_line":""},{"line_number":188,"context_line":"        pipeline \u003d self._statsd.pipeline()"},{"line_number":189,"context_line":""},{"line_number":190,"context_line":"        node_requests \u003d list(node_iterator)"},{"line_number":191,"context_line":""},{"line_number":192,"context_line":"        provider_requests \u003d {}"},{"line_number":193,"context_line":"        for pool in pools:"}],"source_content_type":"text/x-python","patch_set":4,"id":"9bfb5287_839767e8","line":190,"updated":"2024-02-29 16:03:44.000000000","message":"Converting this to a list is not necessary. You can iterate directly over the iterator (in the for loop below) and save one round of iteration.\n(`list(x)` iterates over `x`)","commit_id":"dd2f4d02f222e6b7602dd5f4562b4ccec78c444d"},{"author":{"_account_id":31083,"name":"Benjamin Schanzel","email":"benjamin.schanzel@bmw.de","username":"benjamin.schanzel"},"change_message_id":"6f873c776fe92f62f4900583bc53a94eb1314173","unresolved":true,"context_lines":[{"line_number":190,"context_line":"        node_requests \u003d list(node_iterator)"},{"line_number":191,"context_line":""},{"line_number":192,"context_line":"        provider_requests \u003d {}"},{"line_number":193,"context_line":"        for pool in pools:"},{"line_number":194,"context_line":"            provider_requests[provider_name :\u003d pool.provider_name] \u003d 0"},{"line_number":195,"context_line":"            for node_request in node_requests:"},{"line_number":196,"context_line":"                if all("}],"source_content_type":"text/x-python","patch_set":4,"id":"344ae0ab_22ec8315","line":193,"updated":"2024-02-29 16:03:44.000000000","message":"`for pool in zk_conn.getRegisteredPools()` (see above)","commit_id":"dd2f4d02f222e6b7602dd5f4562b4ccec78c444d"},{"author":{"_account_id":31083,"name":"Benjamin Schanzel","email":"benjamin.schanzel@bmw.de","username":"benjamin.schanzel"},"change_message_id":"6f873c776fe92f62f4900583bc53a94eb1314173","unresolved":true,"context_lines":[{"line_number":192,"context_line":"        provider_requests \u003d {}"},{"line_number":193,"context_line":"        for pool in pools:"},{"line_number":194,"context_line":"            provider_requests[provider_name :\u003d pool.provider_name] \u003d 0"},{"line_number":195,"context_line":"            for node_request in node_requests:"},{"line_number":196,"context_line":"                if all("},{"line_number":197,"context_line":"                        label in pool.supported_labels"},{"line_number":198,"context_line":"                        for label in node_request.node_types"}],"source_content_type":"text/x-python","patch_set":4,"id":"822398f5_d5ef65b5","line":195,"updated":"2024-02-29 16:03:44.000000000","message":"`for node_request in zk.nodeRequestIterator(cached_ids\u003dTrue)` (see above)","commit_id":"dd2f4d02f222e6b7602dd5f4562b4ccec78c444d"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"ce4c763ed3e34e2d4bdbfb407da2969fe165e97a","unresolved":true,"context_lines":[{"line_number":189,"context_line":""},{"line_number":190,"context_line":"        provider_requests \u003d {}"},{"line_number":191,"context_line":"        for pool in zk_conn.getRegisteredPools():"},{"line_number":192,"context_line":"            if not hasattr(pool, \"name\") or pool.name is None:"},{"line_number":193,"context_line":"                # skip pools without name attribute for backward compatibility"},{"line_number":194,"context_line":"                continue"},{"line_number":195,"context_line":"            provider_requests[(pool.provider_name, pool.name)] \u003d 0"}],"source_content_type":"text/x-python","patch_set":8,"id":"52f4b63e_305011a0","line":192,"updated":"2024-03-05 21:13:19.000000000","message":"Maybe just:\n  if not getattr(pool, \u0027name\u0027):\n\nThat way we handdle the case of an empty name and do so succinctly.","commit_id":"5a9e3644bfb5b738b26b8571223134fd1ed6fcc4"},{"author":{"_account_id":31083,"name":"Benjamin Schanzel","email":"benjamin.schanzel@bmw.de","username":"benjamin.schanzel"},"change_message_id":"ae978f6a259b68705ec0f40914e5e08a5109a3fb","unresolved":true,"context_lines":[{"line_number":189,"context_line":""},{"line_number":190,"context_line":"        provider_requests \u003d {}"},{"line_number":191,"context_line":"        for pool in zk_conn.getRegisteredPools():"},{"line_number":192,"context_line":"            if not hasattr(pool, \"name\") or pool.name is None:"},{"line_number":193,"context_line":"                # skip pools without name attribute for backward compatibility"},{"line_number":194,"context_line":"                continue"},{"line_number":195,"context_line":"            provider_requests[(pool.provider_name, pool.name)] \u003d 0"}],"source_content_type":"text/x-python","patch_set":8,"id":"e0f13a9f_cb79eb2a","line":192,"in_reply_to":"52f4b63e_305011a0","updated":"2024-04-30 08:19:12.000000000","message":"A thought on that: `getattr` throws an error if we don\u0027t supply a third default argument, so we\u0027d need to have `getattr(pool, \u0027name\u0027, None)` here. I think the current form is more readable.","commit_id":"5a9e3644bfb5b738b26b8571223134fd1ed6fcc4"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"88517329cf5bdbf1d65d0c9e2a138f29bcfae8ca","unresolved":false,"context_lines":[{"line_number":189,"context_line":""},{"line_number":190,"context_line":"        provider_requests \u003d {}"},{"line_number":191,"context_line":"        for pool in zk_conn.getRegisteredPools():"},{"line_number":192,"context_line":"            if not hasattr(pool, \"name\") or pool.name is None:"},{"line_number":193,"context_line":"                # skip pools without name attribute for backward compatibility"},{"line_number":194,"context_line":"                continue"},{"line_number":195,"context_line":"            provider_requests[(pool.provider_name, pool.name)] \u003d 0"}],"source_content_type":"text/x-python","patch_set":8,"id":"1641070d_39c0aa77","line":192,"in_reply_to":"e0f13a9f_cb79eb2a","updated":"2024-06-03 17:04:50.000000000","message":"Yes, `getattr(pool, \u0027name\u0027, None)` would be correct.  I\u0027m not going to insist on this change, but I do think it would be better and I recommend becoming accustomed to using more succinct and efficient pythonic constructs like that, as we do elsewhere.","commit_id":"5a9e3644bfb5b738b26b8571223134fd1ed6fcc4"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"ce4c763ed3e34e2d4bdbfb407da2969fe165e97a","unresolved":true,"context_lines":[{"line_number":208,"context_line":"                      f\"{provider_name}.\""},{"line_number":209,"context_line":"                      f\"{pool_name}.\""},{"line_number":210,"context_line":"                      \"handleable_requests\")"},{"line_number":211,"context_line":"            pipeline.gauge(metric, requests_count)"},{"line_number":212,"context_line":""},{"line_number":213,"context_line":"        pipeline.send()"}],"source_content_type":"text/x-python","patch_set":8,"id":"c9497517_0d0d77ed","line":211,"updated":"2024-03-05 21:13:19.000000000","message":"This will loop through the full set of node requests once for every provider-pool in the system.  That means this method doesn\u0027t just scale with the size of the request load, it compounds that with the size of the system.  It\u0027s still linear, but with a higher multiple than necessary.\n\nIf, instead, you did something like this, we would only iterate through the provider-pool list once, and then only iterate through the request list once:\n\n  for every registered pool:\n    initialize pool_counters[(provider, pool)] \u003d Counter(\u0027requests\u0027)\n    for every label supported by the pool:\n      append counter to counters_by_label[label]\n  for every node request:\n    for every label in the request:\n      for every counter in counters_by_label[label]:\n        increment counter(\u0027requests\u0027)\n  for every counter in pool_counters:\n    emit stat","commit_id":"5a9e3644bfb5b738b26b8571223134fd1ed6fcc4"},{"author":{"_account_id":1,"name":"James E. Blair","email":"jim@acmegating.com","username":"corvus"},"change_message_id":"88517329cf5bdbf1d65d0c9e2a138f29bcfae8ca","unresolved":false,"context_lines":[{"line_number":208,"context_line":"                      f\"{provider_name}.\""},{"line_number":209,"context_line":"                      f\"{pool_name}.\""},{"line_number":210,"context_line":"                      \"handleable_requests\")"},{"line_number":211,"context_line":"            pipeline.gauge(metric, requests_count)"},{"line_number":212,"context_line":""},{"line_number":213,"context_line":"        pipeline.send()"}],"source_content_type":"text/x-python","patch_set":8,"id":"18b6a186_721bd6da","line":211,"in_reply_to":"c9497517_0d0d77ed","updated":"2024-06-03 17:04:50.000000000","message":"Done","commit_id":"5a9e3644bfb5b738b26b8571223134fd1ed6fcc4"}]}