fix(metrics): prevent thread leak by ensuring singleton initialization

[sinhasubham]

Summary:
This PR fixes a critical memory and thread leak in the google-cloud-spanner client when built-in metrics are enabled (default behavior).
Previously, the Client constructor unconditionally initialized a new OpenTelemetry MeterProvider and PeriodicExportingMetricReader on every instantiation. Each reader spawned a new background thread for metric exporting that was never cleaned up or reused. In environments where Client objects are frequently created (e.g., Cloud Functions, web servers, or data pipelines), this caused a linear accumulation of threads, leading to RuntimeError: can't start new thread and OOM crashes.

Fix Implementation:
Refactored Metrics Initialization (Thread Safety & Memory Leak Fix):
Implemented a Singleton pattern for the OpenTelemetry MeterProvider using threading.Lock to prevent infinite background thread creation (memory leak).
Moved metrics initialization logic to a cleaner helper function _initialize_metrics in client.py.
Replaced global mutable state in SpannerMetricsTracerFactory with contextvars.ContextVar to ensure thread-safe, isolated metric tracing across concurrent requests.
Updated MetricsInterceptor and MetricsCapture to correctly use the thread-local tracer.
Fixed Batch.commit Idempotency (AlreadyExists Regression):
Modified Batch.commit to initialize nth_request and the attempt counter outside the retry loop.
This ensures that retries (e.g., on ABORTED) reuse the same Request ID, allowing Cloud Spanner to correctly deduplicate requests and preventing spurious AlreadyExists (409) errors.
Verification:
Added tests/unit/test_metrics_concurrency.py to verify tracer isolation and thread safety.
Cleaned up tests/unit/test_metrics.py and consolidated mocks in conftest.py.

[gemini-code-assist]

Summary of Changes

Hello @sinhasubham, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request resolves a critical issue in the google-cloud-spanner client where enabling built-in metrics caused a linear accumulation of background threads and memory, leading to RuntimeError and out-of-memory crashes in environments with frequent Client instantiations. The fix ensures that the OpenTelemetry metrics components are initialized using a singleton pattern, guaranteeing they are set up only once per process. This change significantly improves resource management and stability for applications utilizing the Spanner client with metrics enabled.

Highlights

• Thread and Memory Leak Fix: Implemented a singleton pattern for OpenTelemetry metrics initialization to prevent the creation of multiple PeriodicExportingMetricReader instances, which previously led to an accumulation of background threads and potential memory leaks.
• Singleton Implementation: Introduced a global boolean flag _metrics_monitor_initialized in client.py to ensure that the metrics subsystem is initialized only once per process, regardless of how many Client objects are instantiated.
• Enhanced Test Coverage: Added a new unit test (test_constructor_metrics_singleton_behavior) to explicitly verify that the metrics components are initialized only once. Existing tests were also updated to properly manage the global state of the metrics initialization flag.
• Robust Error Handling: Ensured that exceptions during metrics initialization are caught and logged as warnings, preventing crashes while still informing developers of potential issues.

Changelog

• google/cloud/spanner_v1/client.py
  • Added a global boolean variable _metrics_monitor_initialized to track the initialization status of the metrics monitor.
  • Wrapped the OpenTelemetry MeterProvider and PeriodicExportingMetricReader initialization logic within a conditional block, executing only if _metrics_monitor_initialized is False.
  • Set _metrics_monitor_initialized to True after successful metrics setup to prevent re-initialization.
  • Declared _metrics_monitor_initialized as global within the Client.__init__ method to allow modification.
• tests/unit/test_client.py
  • Introduced a new test case test_constructor_metrics_singleton_behavior to verify that metrics initialization occurs exactly once across multiple Client instantiations.
  • Modified test_constructor_w_metrics_initialization_error to include additional mocks for metrics-related components and to reset the global _metrics_monitor_initialized flag for proper test isolation.
  • Ensured that mock_metrics.set_meter_provider is asserted as called once in the error handling test.
• tests/unit/test_metrics.py
  • Updated the patched_client fixture to explicitly reset the client_module._metrics_monitor_initialized flag to False before and after tests. This ensures a clean state for metrics initialization in each test run.
  • Added a patch for CloudMonitoringMetricsExporter within the patched_client fixture to prevent actual external calls during tests.

Activity

• The author sinhasubham initiated this pull request to address a critical thread and memory leak in the Spanner client's metrics subsystem.
• The core changes involve modifying the client.py file to implement a singleton pattern for metrics initialization.
• New unit tests were added in test_client.py to validate the correct singleton behavior of the metrics setup.
• Existing test fixtures in test_metrics.py were updated to ensure proper isolation and state management for tests involving metrics initialization.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request addresses a critical thread and memory leak by ensuring the metrics subsystem is initialized only once. The approach uses a global flag to track initialization, which is a good start. However, the current implementation is not thread-safe and could still lead to multiple initializations under concurrent Client instantiations. I've provided a suggestion to add a threading.Lock to make the initialization truly a singleton. Additionally, I found a minor code duplication in one of the tests.

[olavloite]

nit: could this potentially be moved to a separate function to keep the init function a bit shorter/cleaner?

[olavloite]

nit: remove

[olavloite]

/gemini review

[gemini-code-assist]

Code Review

This pull request provides a crucial fix for a thread and memory leak issue caused by repeated initialization of OpenTelemetry providers. The implementation correctly uses a singleton pattern with double-checked locking for the metrics provider initialization, ensuring it only runs once. The switch from a global mutable tracer to a thread-safe contextvars.ContextVar is well-executed and effectively isolates metric tracing across concurrent operations, as demonstrated by the new concurrency tests. The accompanying fix for batch commit idempotency is also a valuable improvement. The test suite has been significantly enhanced with new concurrency tests and refactoring of existing ones, providing strong confidence in the correctness of these changes. I have one suggestion for improving the API clarity in the SpannerMetricsTracerFactory. Overall, this is an excellent and well-tested contribution that addresses a critical issue.

[gemini-code-assist]

You've introduced both a static method get_current_tracer() and a property current_metrics_tracer that do the same thing: retrieve the tracer from the context variable.

The property current_metrics_tracer is problematic because it replaces a class attribute with an instance property. Any code that previously accessed SpannerMetricsTracerFactory.current_metrics_tracer will now get a property object instead of the tracer, which is a breaking change and could lead to subtle bugs.

Since all new code in this PR uses the clear and unambiguous static method get_current_tracer(), I recommend removing the redundant and potentially confusing current_metrics_tracer property. This will make the API cleaner and prevent accidental misuse.