Skip to content

perf(spanner): carry MetricsTracer on call context instead of global registry - #9310

Open
olavloite wants to merge 1 commit into
mainfrom
spanner-metrics-tracer-on-call-context
Open

olavloite wants to merge 1 commit into
mainfrom
spanner-metrics-tracer-on-call-context

Conversation

@olavloite

Copy link
Copy Markdown
Contributor

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.

@olavloite
olavloite requested a review from a team as a code owner September 13, 2026 10:24
@product-auto-label product-auto-label Bot added the api: spanner Issues related to the Spanner API. label Sep 13, 2026

@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 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.

Comment thread handwritten/spanner/src/metrics/metrics-tracer-factory.ts
@olavloite
olavloite force-pushed the spanner-metrics-tracer-on-call-context branch from 6790179 to 24ffb1f Compare September 13, 2026 10:38
@olavloite

Copy link
Copy Markdown
Contributor Author

/gemini review

@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 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.

Comment thread handwritten/spanner/src/index.ts Outdated
Comment thread handwritten/spanner/src/index.ts
Comment thread handwritten/spanner/src/index.ts
Comment thread handwritten/spanner/src/metrics/metrics-tracer-factory.ts
@olavloite
olavloite force-pushed the spanner-metrics-tracer-on-call-context branch from 24ffb1f to 00ae672 Compare September 13, 2026 10:58
@olavloite

Copy link
Copy Markdown
Contributor Author

/gemini review

@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 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.

Comment thread handwritten/spanner/src/index.ts Outdated
@olavloite
olavloite force-pushed the spanner-metrics-tracer-on-call-context branch from 00ae672 to 2da7278 Compare September 13, 2026 11:26
@olavloite

Copy link
Copy Markdown
Contributor Author

/gemini review

@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 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.

Comment thread handwritten/spanner/src/metrics/interceptor.ts Outdated
…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.
@olavloite
olavloite force-pushed the spanner-metrics-tracer-on-call-context branch from 2da7278 to bab2b60 Compare September 13, 2026 12:12
@olavloite

Copy link
Copy Markdown
Contributor Author

/gemini review

@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 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.

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

Labels

api: spanner Issues related to the Spanner API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant