Skip to content

Commit 67c682e

Browse files
sinhasubhamsinhasubham
committed
fix(metrics): prevent thread leak by ensuring singleton initialization
1 parent 2c5eb96 commit 67c682e

File tree

3 files changed

+121
-35
lines changed

3 files changed

+121
-35
lines changed

‎google/cloud/spanner_v1/client.py‎

Lines changed: 30 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
import os
2828
import logging
2929
import warnings
30+
import threading
3031

3132
from google.api_core.gapic_v1 import client_info
3233
from google.auth.credentials import AnonymousCredentials
@@ -99,6 +100,9 @@ def _get_spanner_optimizer_statistics_package():
99100

100101
log = logging.getLogger(__name__)
101102

103+
_metrics_monitor_initialized = False
104+
_metrics_monitor_lock = threading.Lock()
105+
102106

103107
def _get_spanner_enable_builtin_metrics_env():
104108
return os.getenv(SPANNER_DISABLE_BUILTIN_METRICS_ENV_VAR) != "true"
@@ -252,30 +256,37 @@ def __init__(
252256
):
253257
warnings.warn(_EMULATOR_HOST_HTTP_SCHEME)
254258
# Check flag to enable Spanner builtin metrics
259+
global _metrics_monitor_initialized
255260
if (
256261
_get_spanner_enable_builtin_metrics_env()
257262
and not disable_builtin_metrics
258263
and HAS_GOOGLE_CLOUD_MONITORING_INSTALLED
259264
):
260-
meter_provider = metrics.NoOpMeterProvider()
261-
try:
262-
if not _get_spanner_emulator_host():
263-
meter_provider = MeterProvider(
264-
metric_readers=[
265-
PeriodicExportingMetricReader(
266-
CloudMonitoringMetricsExporter(
267-
project_id=project, credentials=credentials
268-
),
269-
export_interval_millis=METRIC_EXPORT_INTERVAL_MS,
270-
),
271-
]
272-
)
273-
metrics.set_meter_provider(meter_provider)
274-
SpannerMetricsTracerFactory()
275-
except Exception as e:
276-
log.warning(
277-
"Failed to initialize Spanner built-in metrics. Error: %s", e
278-
)
265+
if not _metrics_monitor_initialized:
266+
with _metrics_monitor_lock:
267+
if not _metrics_monitor_initialized:
268+
meter_provider = metrics.NoOpMeterProvider()
269+
try:
270+
if not _get_spanner_emulator_host():
271+
meter_provider = MeterProvider(
272+
metric_readers=[
273+
PeriodicExportingMetricReader(
274+
CloudMonitoringMetricsExporter(
275+
project_id=project,
276+
credentials=credentials,
277+
),
278+
export_interval_millis=METRIC_EXPORT_INTERVAL_MS,
279+
),
280+
]
281+
)
282+
metrics.set_meter_provider(meter_provider)
283+
SpannerMetricsTracerFactory()
284+
_metrics_monitor_initialized = True
285+
except Exception as e:
286+
log.warning(
287+
"Failed to initialize Spanner built-in metrics. Error: %s",
288+
e,
289+
)
279290
else:
280291
SpannerMetricsTracerFactory(enabled=False)
281292

‎tests/unit/test_client.py‎

Lines changed: 78 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -255,28 +255,44 @@ def test_constructor_w_directed_read_options(self):
255255
expected_scopes, creds, directed_read_options=self.DIRECTED_READ_OPTIONS
256256
)
257257

258+
@mock.patch("google.cloud.spanner_v1.client.metrics")
259+
@mock.patch("google.cloud.spanner_v1.client.CloudMonitoringMetricsExporter")
260+
@mock.patch("google.cloud.spanner_v1.client.PeriodicExportingMetricReader")
261+
@mock.patch("google.cloud.spanner_v1.client.MeterProvider")
258262
@mock.patch("google.cloud.spanner_v1.client.SpannerMetricsTracerFactory")
259263
@mock.patch.dict(os.environ, {"SPANNER_DISABLE_BUILTIN_METRICS": "false"})
260264
def test_constructor_w_metrics_initialization_error(
261-
self, mock_spanner_metrics_factory
265+
self,
266+
mock_spanner_metrics_factory,
267+
mock_meter_provider,
268+
mock_periodic_reader,
269+
mock_exporter,
270+
mock_metrics,
262271
):
263272
"""
264273
Test that Client constructor handles exceptions during metrics
265274
initialization and logs a warning.
266275
"""
267276
from google.cloud.spanner_v1.client import Client
277+
from google.cloud.spanner_v1 import client as MUT
268278

279+
MUT._metrics_monitor_initialized = False
269280
mock_spanner_metrics_factory.side_effect = Exception("Metrics init failed")
270281
creds = build_scoped_credentials()
271-
272-
with self.assertLogs("google.cloud.spanner_v1.client", level="WARNING") as log:
273-
client = Client(project=self.PROJECT, credentials=creds)
274-
self.assertIsNotNone(client)
275-
self.assertIn(
276-
"Failed to initialize Spanner built-in metrics. Error: Metrics init failed",
277-
log.output[0],
278-
)
279-
mock_spanner_metrics_factory.assert_called_once()
282+
try:
283+
with self.assertLogs(
284+
"google.cloud.spanner_v1.client", level="WARNING"
285+
) as log:
286+
client = Client(project=self.PROJECT, credentials=creds)
287+
self.assertIsNotNone(client)
288+
self.assertIn(
289+
"Failed to initialize Spanner built-in metrics. Error: Metrics init failed",
290+
log.output[0],
291+
)
292+
mock_spanner_metrics_factory.assert_called_once()
293+
mock_metrics.set_meter_provider.assert_called_once()
294+
finally:
295+
MUT._metrics_monitor_initialized = False
280296

281297
@mock.patch("google.cloud.spanner_v1.client.SpannerMetricsTracerFactory")
282298
@mock.patch.dict(os.environ, {"SPANNER_DISABLE_BUILTIN_METRICS": "true"})
@@ -293,6 +309,58 @@ def test_constructor_w_disable_builtin_metrics_using_env(
293309
self.assertIsNotNone(client)
294310
mock_spanner_metrics_factory.assert_called_once_with(enabled=False)
295311

312+
@mock.patch("google.cloud.spanner_v1.client.metrics")
313+
@mock.patch("google.cloud.spanner_v1.client.CloudMonitoringMetricsExporter")
314+
@mock.patch("google.cloud.spanner_v1.client.PeriodicExportingMetricReader")
315+
@mock.patch("google.cloud.spanner_v1.client.MeterProvider")
316+
@mock.patch("google.cloud.spanner_v1.client.SpannerMetricsTracerFactory")
317+
@mock.patch.dict(os.environ, {"SPANNER_DISABLE_BUILTIN_METRICS": "false"})
318+
def test_constructor_metrics_singleton_behavior(
319+
self,
320+
mock_spanner_metrics_factory,
321+
mock_meter_provider,
322+
mock_periodic_reader,
323+
mock_exporter,
324+
mock_metrics,
325+
):
326+
"""
327+
Test that metrics are only initialized once.
328+
"""
329+
from google.cloud.spanner_v1 import client as MUT
330+
331+
# Reset global state for this test
332+
MUT._metrics_monitor_initialized = False
333+
try:
334+
creds = build_scoped_credentials()
335+
336+
# First client initialization
337+
client1 = MUT.Client(project=self.PROJECT, credentials=creds)
338+
self.assertIsNotNone(client1)
339+
mock_metrics.set_meter_provider.assert_called_once()
340+
mock_spanner_metrics_factory.assert_called_once()
341+
342+
# Verify MeterProvider chain was created
343+
mock_meter_provider.assert_called_once()
344+
mock_periodic_reader.assert_called_once()
345+
mock_exporter.assert_called_once()
346+
347+
self.assertTrue(MUT._metrics_monitor_initialized)
348+
349+
# Reset mocks to verify they are NOT called again
350+
mock_metrics.set_meter_provider.reset_mock()
351+
mock_spanner_metrics_factory.reset_mock()
352+
mock_meter_provider.reset_mock()
353+
354+
# Second client initialization
355+
client2 = MUT.Client(project=self.PROJECT, credentials=creds)
356+
self.assertIsNotNone(client2)
357+
mock_metrics.set_meter_provider.assert_not_called()
358+
mock_spanner_metrics_factory.assert_not_called()
359+
mock_meter_provider.assert_not_called()
360+
self.assertTrue(MUT._metrics_monitor_initialized)
361+
finally:
362+
MUT._metrics_monitor_initialized = False
363+
296364
@mock.patch("google.cloud.spanner_v1.client.SpannerMetricsTracerFactory")
297365
def test_constructor_w_disable_builtin_metrics_using_option(
298366
self, mock_spanner_metrics_factory

‎tests/unit/test_metrics.py‎

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -60,17 +60,24 @@ def patched_client(monkeypatch):
6060
if SpannerMetricsTracerFactory._metrics_tracer_factory is not None:
6161
SpannerMetricsTracerFactory._metrics_tracer_factory = None
6262

63-
client = Client(
64-
project="test",
65-
credentials=TestCredentials(),
66-
# client_options={"api_endpoint": "none"}
67-
)
68-
yield client
63+
# Reset the global flag to ensure metrics initialization runs
64+
from google.cloud.spanner_v1 import client as client_module
65+
66+
client_module._metrics_monitor_initialized = False
67+
68+
with patch("google.cloud.spanner_v1.client.CloudMonitoringMetricsExporter"):
69+
client = Client(
70+
project="test",
71+
credentials=TestCredentials(),
72+
# client_options={"api_endpoint": "none"}
73+
)
74+
yield client
6975

7076
# Resetting
7177
metrics.set_meter_provider(metrics.NoOpMeterProvider())
7278
SpannerMetricsTracerFactory._metrics_tracer_factory = None
7379
SpannerMetricsTracerFactory.current_metrics_tracer = None
80+
client_module._metrics_monitor_initialized = False
7481

7582

7683
def test_metrics_emission_with_failure_attempt(patched_client):

0 commit comments

Comments
 (0)