Skip to content

[DRAFT] 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

Draft
daniel-sanche wants to merge 2 commits into
base: bigtable_csm_3_handlers
Choose a base branch
from
+32 −28
Draft

[DRAFT] 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
b44895d
remove backoff time from application blocking latencies
daniel-sanche Jul 24, 2026
0b3600d
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 number Diff line number Diff 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 number Diff line number Diff 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-assist gemini-code-assist Bot Jul 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
and op.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
and op.flow_throttling_time_ns is not None
and op.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 number Diff line number Diff 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-assist gemini-code-assist Bot Jul 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