feat(bigtable): client side metrics handlers - #16760
Conversation
…_2_instrumentation_advanced
There was a problem hiding this comment.
Code Review
This pull request implements client-side metrics for the Bigtable library using OpenTelemetry, including a custom exporter for Google Cloud Monitoring. The review feedback focuses on improving resource efficiency by moving the MeterProvider and client_uid generation to the client level to avoid thread leaks and inconsistent identifiers across tables. Additionally, recommendations were made to handle potential KeyError exceptions in the exporter, improve logging for background export failures, and ensure non-negative timeouts during batch writes.
| write_ind = 0 | ||
| while write_ind < len(series): | ||
| # find time left for next batch | ||
| timeout = deadline - time.time() if deadline else gapic_v1.method.DEFAULT |
There was a problem hiding this comment.
why do we need to calculate the remaining deadline for each batch? I think each batch should just use the default timeout.
There was a problem hiding this comment.
The intention here is to be consistent with the OpenTelemetry exporter spec, which passes in a timeout value for exporters to conform to.
I'm not entirely sure what happens if we ignore it, but I can look into it if needed. Does Java not have anything similar?
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces client-side metrics collection and exporting to Google Cloud Monitoring for the Bigtable Python client by adding GoogleCloudMetricsHandler and BigtableMetricsExporter. Feedback on the changes highlights several critical issues, including potential KeyError and TypeError exceptions during metric label processing and protobuf serialization, broad exception handling that silently masks export failures, and side-effects from instantiating OpenTelemetry instruments as default arguments. Additionally, corrections are needed in the system tests to resolve mismatches in metric names and type prefixes.
…rics/handlers/opentelemetry.py Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…rics/handlers/gcp_exporter.py Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
| self, | ||
| project_id: str, | ||
| series: list[TimeSeries], | ||
| deadline=None, |
There was a problem hiding this comment.
SHould we have a deadline for the write?
There was a problem hiding this comment.
My interpretation is yes, we should follow a deadline, and we pass it down here (the None in the code above your comment is just the function default value)
The MetricExporter base class has a timeout_millis argument, so our timeout policy is just making sure we are following the expectations of the base class
But let me know if you think we need to handle anything differently
Updates the client post-processing configuration for `google-cloud-bigtable` in `.librarian/generator-input/client-post-processing/bigtable-integration.yaml` to ensure code generation produces zero diff. Required as a result of the change in #16760
Migrate googleapis/python-bigtable#1189 to the monorepo
This PR builds off of googleapis/python-bigtable#1187 to add handlers to the client-side metrics system, which can subscribe to the metrics stream, and export the results into different collection systems
We add two handlers to the system:
GoogleCloudMetricsHandler: sends metrics to a private OpenTelemetry meter, and then periodically exports them to GCP. Built on top ofOpenTelemetryMetricsHandlerOpenTelemetryMetricsHandler: sends metrics to the root MeterProvider, so the user can access the exported metrics for their own systems. This will be off by default, but can be added alongsideGoogleCloudMetricsHandlerif needed