Skip to content

fix(bigtable): standardize client side metrics#17899

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
daniel-sanche merged 2 commits into from
Aug 12, 2026
+32 −28
Merged

fix(bigtable): standardize client side metrics #17899

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c2c8daa
remove backoff time from application blocking latencies
daniel-sanche Jul 24, 2026
64acfa1
avoid duplicate attribution of flow vontrol blocking time
daniel-sanche Jul 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion packages/google-cloud-bigtable/google/cloud/bigtable/data/_metrics/data_model.py
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -140,7 +140,6 @@ class ActiveAttemptMetric:
# time waiting on user to process the response, in nanoseconds
# currently only relevant for ReadRows
application_blocking_time_ns: int = 0
# backoff time is added to application_blocking_time_ns
backoff_before_attempt_ns: int = 0


Expand Down
16 changes: 9 additions & 7 deletions packages/google-cloud-bigtable/google/cloud/bigtable/data/_metrics/handlers/opentelemetry.py
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -181,6 +181,7 @@ def on_operation_complete(self, op: CompletedOperationMetric) -> None:
- operation_latencies
- retry_count
- first_response_latencies
- throttling_latencies
"""
labels = {
"method": op.op_type.value,
Expand All@@ -204,6 +205,13 @@ def on_operation_complete(self, op: CompletedOperationMetric) -> None:
# only record completed attempts if there were retries
if op.completed_attempts:
self.otel.retry_count.add(len(op.completed_attempts) - 1, labels)
if (
op.op_type == OperationType.BULK_MUTATE_ROWS
and op.flow_throttling_time_ns > 0
):
self.otel.throttling_latencies.record(
op.flow_throttling_time_ns / NS_TO_MS, labels
)
Comment on lines +208 to +214

@gemini-code-assistgemini-code-assistBotJul 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If op.flow_throttling_time_ns is None, evaluating op.flow_throttling_time_ns > 0 will raise a TypeError at runtime. Adding a defensive check to ensure it is not None before comparison prevents potential crashes.

Suggested change
if (
op.op_type==OperationType.BULK_MUTATE_ROWS
andop.flow_throttling_time_ns>0
):
self.otel.throttling_latencies.record(
op.flow_throttling_time_ns/NS_TO_MS, labels
)
if (
op.op_type==OperationType.BULK_MUTATE_ROWS
andop.flow_throttling_time_nsisnotNone
andop.flow_throttling_time_ns>0
):
self.otel.throttling_latencies.record(
op.flow_throttling_time_ns/NS_TO_MS, labels
)


def on_attempt_complete(
self, attempt: CompletedAttemptMetric, op: ActiveOperationMetric
Expand All@@ -214,7 +222,6 @@ def on_attempt_complete(
- server_latencies
- connectivity_error_count
- application_latencies
- throttling_latencies
"""
labels = {
"method": op.op_type.value,
Expand All@@ -229,13 +236,8 @@ def on_attempt_complete(
attempt.duration_ns / NS_TO_MS,
{"streaming": is_streaming, "status": status, **labels},
)
flow_throttling = (
op.flow_throttling_time_ns / NS_TO_MS if op.flow_throttling_time_ns else 0
)
self.otel.throttling_latencies.record(flow_throttling, labels)
self.otel.application_latencies.record(
(attempt.application_blocking_time_ns + attempt.backoff_before_attempt_ns)
/ NS_TO_MS,
attempt.application_blocking_time_ns / NS_TO_MS,
labels,
)
if attempt.gfe_latency_ns is not None:
Expand Down
43 changes: 23 additions & 20 deletions packages/google-cloud-bigtable/tests/unit/data/_metrics/test_opentelemetry_handler.py
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -323,31 +323,35 @@ def test_on_attempt_complete_attempt_latencies(self):
)

@pytest.mark.parametrize(
"is_first_attempt,flow_throttling_ns",
[(True, 54321), (False, 0), (True, 0)],
"op_type,flow_throttling_ns,should_record",
[
(OperationType.BULK_MUTATE_ROWS, 54321, True),
(OperationType.BULK_MUTATE_ROWS, 0, False),
(OperationType.READ_ROWS, 54321, False),
],
)
Comment on lines 325 to 332

@gemini-code-assistgemini-code-assistBotJul 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Add a test case where flow_throttling_ns is None to verify that the handler safely handles None values without raising a TypeError.

@pytest.mark.parametrize("op_type,flow_throttling_ns,should_record", [ (OperationType.BULK_MUTATE_ROWS, 54321, True), (OperationType.BULK_MUTATE_ROWS, 0, False), (OperationType.BULK_MUTATE_ROWS, None, False), (OperationType.READ_ROWS, 54321, False), ], )

def test_on_attempt_complete_throttling_latencies(
self, is_first_attempt, flow_throttling_ns
def test_on_operation_complete_throttling_latencies(
self, op_type, flow_throttling_ns, should_record
):
mock_instruments = mock.Mock(throttling_latencies=mock.Mock())
handler = self._make_one(instruments=mock_instruments)
attempt = CompletedAttemptMetric(
op = CompletedOperationMetric(
op_type=op_type,
duration_ns=1234567,
end_status=StatusCode.OK,
)
op = ActiveOperationMetric(
op_type=OperationType.READ_ROWS,
completed_attempts=[],
final_status=StatusCode.OK,
cluster_id="cluster",
zone="zone",
is_streaming=False,
flow_throttling_time_ns=flow_throttling_ns,
)
if not is_first_attempt:
op.completed_attempts.append(mock.Mock())
handler.on_attempt_complete(attempt, op)
expected_throttling = 0
if is_first_attempt:
expected_throttling += flow_throttling_ns / 1e6
mock_instruments.throttling_latencies.record.assert_called_once_with(
pytest.approx(expected_throttling), mock.ANY
)
handler.on_operation_complete(op)
if should_record:
mock_instruments.throttling_latencies.record.assert_called_once_with(
pytest.approx(flow_throttling_ns / 1e6), mock.ANY
)
else:
mock_instruments.throttling_latencies.record.assert_not_called()

def test_on_attempt_complete_application_latencies(self):
mock_instruments = mock.Mock(application_latencies=mock.Mock())
Expand All@@ -361,8 +365,7 @@ def test_on_attempt_complete_application_latencies(self):
op = ActiveOperationMetric(op_type=OperationType.READ_ROWS)
handler.on_attempt_complete(attempt, op)
mock_instruments.application_latencies.record.assert_called_once_with(
(attempt.application_blocking_time_ns + attempt.backoff_before_attempt_ns)
/ 1e6,
attempt.application_blocking_time_ns / 1e6,
mock.ANY,
)

Expand Down
Loading