Skip to content

[DRAFT] fix(bigtable): standardize client side metrics#17899

Draft
daniel-sanche wants to merge 2 commits into
bigtable_csm_3_handlersfrom
bigtable_csm_fixes
Draft

[DRAFT] fix(bigtable): standardize client side metrics#17899
daniel-sanche wants to merge 2 commits into
bigtable_csm_3_handlersfrom
bigtable_csm_fixes

Conversation

@daniel-sanche

Copy link
Copy Markdown
Contributor

Made some adjustments to the client side metrics system after comparing against node and java implemenations (with gemini)

The diffs for each commit can be looked it in isolation

1 (0b3600d): throttling_latencies is now recorded once per operation

  • java records grpc time as part of throttling latencies. Python just tracks flow-control time
  • flow control happens once at the start of an operation: once the mutations have made it past the flow control gate, retries don't have to wait again, they have space reserved until they reach a terminal state
  • Python was previously recording throttling_latencieson each attempt, but recording the same per-operation value on each attempt. This would give misleading data
  • Now, we only record it once per operation

2 (b44895d): removing backoff time from application blocking latencies

  • previously, python was including time spent backing off between attempts as part of the application_latencies metric
  • other languages do not include this, so we can drop it from Python as well

@gemini-code-assist gemini-code-assist Bot left a comment

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.

Code Review

This pull request refactors how throttling and application latencies are recorded in OpenTelemetry. Specifically, throttling latencies are now recorded on operation completion rather than attempt completion, and only for bulk mutate rows operations. Additionally, application latencies no longer include backoff time. The review feedback highlights a potential TypeError if flow_throttling_time_ns is None during the operation completion check, recommending a defensive check and an accompanying unit test to handle this case safely.

Comment on lines +208 to +214
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
)

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
)

Comment on lines 325 to 332
@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),
],
)

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),
        ],
    )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant