Conversation
There was a problem hiding this comment.
Code Review
This pull request removes the background cleanup interval and the global tracking of active operation tracers within MetricsTracerFactory. Instead, the MetricsTracer is now carried directly on the call options/context, simplifying tracer resolution and improving context isolation. Additionally, error handling has been improved by wrapping request executions in try-catch blocks to ensure metrics are recorded even when synchronous errors occur. Feedback was provided regarding MetricsTracerFactory.getInstance(), where if the singleton is initially created with an empty projectId, subsequent calls with a valid projectId will not update it, leading to empty project ID attributes in future metrics.
6790179 to
24ffb1f
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request simplifies the OpenTelemetry metrics tracking in the Spanner client by removing the background cleanup timer and passing the MetricsTracer directly on the call options. It also adds error handling around request functions and safely handles missing request options. The code review feedback highlights opportunities to prevent configuration mutation by deep copying headers, avoid double-callback execution using a local guard, defensively check for valid streams, and address a singleton limitation in MetricsTracerFactory that could affect multi-project environments.
24ffb1f to
00ae672
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the metrics tracking implementation in the Spanner client by eliminating the background interval-based cleanup of MetricsTracer instances. Instead, the tracer is carried directly on the call options/context, removing the need for background timers and caching. Additionally, error handling and stream initialization are improved with try-catch blocks. The review feedback suggests using a deep copy for config.headers to prevent accidental mutation of the user's original configuration object.
00ae672 to
2da7278
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the metrics tracking implementation in the Spanner client by eliminating the background tracer cleanup interval and the associated caching maps in MetricsTracerFactory. Instead, MetricsTracer instances are now passed directly via call options and interceptors. Additionally, the changes ensure that caller-provided headers are not mutated, and improve error handling and robustness in both synchronous and streaming requests. Regarding the review feedback, a potential issue was identified in interceptor.ts where truthiness checks on latency values (e.g., metricsTracer?.gfeLatency) could incorrectly treat a latency of 0 as falsy, leading to incorrect metric reporting. It is recommended to explicitly check if the latency is a number.
…registry Pass the MetricsTracer instance directly through gRPC call options rather than registering and retrieving it via a global map keyed by Spanner request ID. - Attach MetricsTracer to call options in request() and requestStream(), resolving it directly in MetricInterceptor. - Eliminate the global _currentOperationTracers map and the background setInterval cleanup timer from MetricsTracerFactory. - Remove per-attempt metadata regex parsing for project ID and request ID in MetricInterceptor. - Record attempt completion before forwarding status to downstream listeners to prevent race conditions during operation completion. - Guard synchronous requestFn invocations to guarantee operation completion metrics on early failures. - Retain getCurrentTracer, clearCurrentTracer, and cleanup interval constants as @deprecated stubs for backwards compatibility.
2da7278 to
bab2b60
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the metrics tracing implementation in the Spanner client by eliminating the background cleanup timer in MetricsTracerFactory and passing the MetricsTracer directly via call options. It also improves robustness by preventing double callback execution, avoiding mutation of caller-provided headers, and correctly handling 0ms latency. There are no review comments, and I have no additional feedback to provide.
Pass the MetricsTracer instance directly through gRPC call options rather than registering and retrieving it via a global map keyed by Spanner request ID.