feat: added client-side instrumentation to all rpcs #925

daniel-sanche · 2024-01-26T23:26:48Z

This PR builds off of #924 to add instrumentation to each rpc in the Bigtable v3 data client

This change required some changes to the gapic generated client, which will need to be upstreamed to the gapic generator at some point: googleapis/gapic-generator-python#1856

TODO:

~~add end-to-end system tests~~
get a final benchmarking in place
improve _mutate_rows instrumentation
more tests?
- mutations batcher
- _read_rows and _mutate_rows operations
  - sepcifically: _MutateRowsIncomplete exception

…instrumentation

mutianf · 2024-02-08T22:10:47Z

google/cloud/bigtable/data/_async/_mutate_rows.py

+                        del active_request_indices[result.index]
+            finally:
+                # send trailing metadata to metrics
+                result_generator.cancel()


why do we need to call cancel in the end?

This is calling cancel on the grpc stream. I think this is in case we encounter a client error; we need to stop the stream so it'll give us the trailing metadata

What client error would happen? I'm just wondering if this will introduce some bugs (like not consuming all the results or cancelling the grpc stream twice)

google/cloud/bigtable/data/_async/_read_rows.py

mutianf · 2024-02-08T22:18:50Z

google/cloud/bigtable/data/_async/_read_rows.py

+                        # the value directly to avoid extra overhead
+                        operation.active_attempt.application_blocking_time_ms += (  # type: ignore
+                            time.monotonic() - block_time
+                        ) * 1000


Can we record everything in nanoseconds and convert them to milliseconds instead?

mutianf · 2024-02-08T22:23:46Z

google/cloud/bigtable/data/_async/client.py

@@ -593,11 +595,20 @@ async def read_rows_stream(
        )
        retryable_excs = _get_retryable_errors(retryable_errors, self)

+        # extract metric operation if passed down through kwargs
+        # used so that read_row can disable is_streaming flag
+        metric_operation = kwargs.pop("metric_operation", None)


Is this the API customer will interact with? This feels a bit weird 🤔 I think if someone calls readrows we can set is_streaming=True and OperationType to be ReadRows.

This is a hidden argument, users aren't supposed to interact with it

The problem is read_row is a small wrapper on top of read_rows. So read_rows can't assume the operation is streaming. This pattern is trying to allow read_row to pass down its own operation when it calls read_rows, so it can make sure streaming is False

We could also solve it using an entirely separate helper if this is too ugly though

users aren't supposed to interact with it

Is it possible to make sure users can't interact with it? The fact that someone could look at the source code and pass in a random string (which will increase the cardinality) is a little concerning.

mutianf · 2024-02-08T22:52:00Z

google/cloud/bigtable/data/_async/mutations_batcher.py

@@ -328,17 +331,26 @@ async def _flush_internal(self, new_entries: list[RowMutationEntry]):
        """
        # flush new entries
        in_process_requests: list[asyncio.Future[list[FailedMutationEntryError]]] = []
+        metric = self._table._metrics.create_operation(OperationType.BULK_MUTATE_ROWS)


We don't have this OperationType in java 😓 let's just use MUTATE_ROWS for this.

BULK_MUTATE_ROWS is just the name we've been using for mutate_rows, since it's easier to parse. The string value is still "MutateRows"

mutianf · 2024-02-08T22:55:17Z

google/cloud/bigtable/data/_metrics/data_model.py

+            timeout_val: float | None,
+        ) -> tuple[Exception, Exception | None]:
+            exc, source = _retry_exception_factory(exc_list, reason, timeout_val)
+            if reason != RetryFailureReason.NON_RETRYABLE_ERROR:


should this be if reason = RetryFailureReason.NON_RETRYABLE_ERROR?

No, the wrapped_predicate will report all exceptions encountered. But the predicate isn't called when the operation ends due to timeout. So this extra wrapper is needed for that

This part is a bit more complicated than I hoped, so I was considering trying to refactor some of the read_rows instrumentation when I have a chance. Let me know what you think

I see. It makes sense but a little hard to follow. Maybe add a comment for now? And also in _read_rows start_operation comment what metric_fns[0] and metric_fns[1] are?

…instrumentation

mutianf · 2024-02-09T21:16:53Z

google/cloud/bigtable/data/_async/_mutate_rows.py

+                        del active_request_indices[result.index]
+            finally:
+                # send trailing metadata to metrics
+                result_generator.cancel()


What client error would happen? I'm just wondering if this will introduce some bugs (like not consuming all the results or cancelling the grpc stream twice)

mutianf · 2024-02-09T21:19:13Z

google/cloud/bigtable/data/_async/_read_rows.py

        # For each row
        while True:
            try:
                c = await it.__anext__()
            except StopAsyncIteration:
                # stream complete
+                operation.end_with_success()


does this happen when customer cancels the read in the middle of a stream?

mutianf · 2024-02-09T21:25:19Z

google/cloud/bigtable/data/_metrics/data_model.py

+            timeout_val: float | None,
+        ) -> tuple[Exception, Exception | None]:
+            exc, source = _retry_exception_factory(exc_list, reason, timeout_val)
+            if reason != RetryFailureReason.NON_RETRYABLE_ERROR:


I see. It makes sense but a little hard to follow. Maybe add a comment for now? And also in _read_rows start_operation comment what metric_fns[0] and metric_fns[1] are?

feat: added instrumentation to all rpcs

2955411

daniel-sanche requested review from a team as code owners January 26, 2024 23:26

product-auto-label bot added size: xl Pull request size is extra large. api: bigtable Issues related to the googleapis/python-bigtable API. labels Jan 26, 2024

This was referenced Jan 26, 2024

feat: client side metrics data model #923

Open

feat: client side metrics handlers #924

Open

daniel-sanche added 18 commits January 30, 2024 17:22

added system test for attempt_latencies metrics

2ba1321

added tests for other latency types

87bd1db

added system tests for all latency metrics

6f79b9a

added tests for counts

d736503

Merge branch 'client_side_metrics_handlers' into client_side_metrics_…

20260dc

…instrumentation

Merge branch 'client_side_metrics_handlers' into client_side_metrics_…

1058fad

…instrumentation

catch timed out operations

67b545e

fixed bug in retry detection

67a6fcd

reworking metric system tests

ef0fdd8

got first test working

7ee253e

fixed test

28d162b

added system tests

6caba34

fixed bug in parsing bulk mutations errors

8ac92d0

fixed up test

7a95c4a

improved tests

c96b534

added missing end_op

86159a4

fixed test

89c5216

fixed lint

fcd3aaa

mutianf reviewed Feb 8, 2024

View reviewed changes

daniel-sanche added 4 commits February 8, 2024 15:18

combined wrapped predicate with wrapped exc factory

f2528ae

fixed blacken

eb3aae1

fixed failing test

1a08f1a

improved exception parsing

fdc2e3b

daniel-sanche added 3 commits February 8, 2024 16:57

removed export interval option

e428bf0

changed buckets

1bf566c

Merge branch 'client_side_metrics_handlers' into client_side_metrics_…

ca0963a

…instrumentation

daniel-sanche force-pushed the client_side_metrics_instrumentation branch from fcbdda6 to ca0963a Compare February 9, 2024 02:01

daniel-sanche requested a review from a team as a code owner February 9, 2024 02:01

mutianf reviewed Feb 9, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: added client-side instrumentation to all rpcs #925

feat: added client-side instrumentation to all rpcs #925

daniel-sanche commented Jan 26, 2024 •

edited

mutianf Feb 8, 2024

daniel-sanche Feb 9, 2024

mutianf Feb 9, 2024

mutianf Feb 8, 2024

mutianf Feb 8, 2024

daniel-sanche Feb 9, 2024

mutianf Feb 9, 2024 •

edited

mutianf Feb 8, 2024

daniel-sanche Feb 9, 2024

mutianf Feb 9, 2024

mutianf Feb 8, 2024

daniel-sanche Feb 9, 2024

mutianf Feb 9, 2024

mutianf Feb 9, 2024

mutianf Feb 9, 2024

mutianf Feb 9, 2024

feat: added client-side instrumentation to all rpcs #925

Are you sure you want to change the base?

feat: added client-side instrumentation to all rpcs #925

Conversation

daniel-sanche commented Jan 26, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mutianf Feb 9, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniel-sanche commented Jan 26, 2024 •

edited

mutianf Feb 9, 2024 •

edited