feat(bigtable): add client side metric instrumentation to basic rpcs#16712
feat(bigtable): add client side metric instrumentation to basic rpcs#16712daniel-sanche wants to merge 4 commits intomainfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request integrates client-side metrics tracking into the Bigtable data client for both asynchronous and synchronous implementations. It wraps key operations—including sample_row_keys, mutate_row, check_and_mutate_row, and read_modify_write_row—with metrics collection logic and introduces a tracked_retry wrapper to monitor retry attempts. Additionally, the PR refactors system tests by consolidating fixtures into a shared SystemTestRunner class and adds new system tests specifically for metrics. Feedback focuses on regressions in retry logic where critical arguments like sleep_generator and exception_factory were omitted in the transition to tracked_retry. There are also suggestions to improve resource cleanup in test fixtures and to relax restrictive timing assertions in tests to prevent flakiness.
| ), | ||
| clusters=cluster_config, | ||
| ) | ||
| operation.result(timeout=240) |
There was a problem hiding this comment.
If operation.result(timeout=240) raises an exception (e.g., a TimeoutError), the fixture will stop execution and the delete_instance call in the teardown phase will never be reached. This can lead to leaked Bigtable instances in the test project. Consider wrapping the creation and yield in a try...finally block or ensuring cleanup happens even on creation failure.
| operation.zone | ||
| == cluster_config[operation.cluster_id].location.split("/")[-1] | ||
| ) | ||
| assert operation.duration_ns > 0 and operation.duration_ns < 1e9 |
There was a problem hiding this comment.
The assertion operation.duration_ns < 1e9 (1 second) might be too restrictive for system tests running against a live backend. Network latency or backend load could easily cause an RPC to exceed 1 second, leading to flaky tests. It is recommended to remove this upper bound or increase it significantly.
| assert operation.duration_ns > 0 and operation.duration_ns < 1e9 | |
| assert operation.duration_ns > 0 |
1 participant
Migration of googleapis/python-bigtable#1188 to the monorepo
This PR builds off of googleapis/python-bigtable#1187 to add instrumentation to basic data client rpcs (check_and_mutate, read_modify_write, sample_row_keys, mutate_row)
Metrics are not currently being exported anywhere, just collected and dropped. A future PR will add a GCP exporter to the system