From e456e0fbfc1731fb31aa65629bc574e12f3eccc5 Mon Sep 17 00:00:00 2001 From: Shuto Otaki Date: Fri, 23 Jan 2026 11:44:10 +0900 Subject: [PATCH 1/4] wip: add test for missing instance_id in SpannerMetricsTracerFactory --- google/cloud/spanner_v1/client.py | 1 + .../spanner_v1/metrics/metrics_exporter.py | 9 +++ test_instance_id_bug.py | 61 +++++++++++++++++++ 3 files changed, 71 insertions(+) create mode 100644 test_instance_id_bug.py diff --git a/google/cloud/spanner_v1/client.py b/google/cloud/spanner_v1/client.py index 5f72905616..dbc33214dc 100644 --- a/google/cloud/spanner_v1/client.py +++ b/google/cloud/spanner_v1/client.py @@ -23,6 +23,7 @@ * a :class:`~google.cloud.spanner_v1.instance.Instance` owns a :class:`~google.cloud.spanner_v1.database.Database` """ + import grpc import os import logging diff --git a/google/cloud/spanner_v1/metrics/metrics_exporter.py b/google/cloud/spanner_v1/metrics/metrics_exporter.py index 68da08b400..2c1a5a234f 100644 --- a/google/cloud/spanner_v1/metrics/metrics_exporter.py +++ b/google/cloud/spanner_v1/metrics/metrics_exporter.py @@ -143,6 +143,15 @@ def _batch_write(self, series: List["TimeSeries"], timeout_millis: float) -> Non :param series: ProtoBuf TimeSeries :return: """ + # DEBUG: Log TimeSeries labels before sending to Cloud Monitoring + logger.warning("=== DEBUG: TimeSeries labels ===") + for ts in series: + logger.warning(f" metric.type: {ts.metric.type}") + logger.warning(f" metric.labels: {dict(ts.metric.labels)}") + logger.warning(f" resource.type: {ts.resource.type}") + logger.warning(f" resource.labels: {dict(ts.resource.labels)}") + logger.warning("=== END DEBUG ===") + write_ind = 0 timeout = timeout_millis / MILLIS_PER_SECOND while write_ind < len(series): diff --git a/test_instance_id_bug.py b/test_instance_id_bug.py new file mode 100644 index 0000000000..30dd45d040 --- /dev/null +++ b/test_instance_id_bug.py @@ -0,0 +1,61 @@ +""" +instance_id 欠落バグの再現テスト + +このスクリプトは、SpannerMetricsTracerFactory が初期化された後、 +instance_id が client_attributes に含まれていないことを確認します。 +""" + +from google.cloud.spanner_v1.metrics.spanner_metrics_tracer_factory import ( + SpannerMetricsTracerFactory, +) +from google.cloud.spanner_v1.metrics.constants import MONITORED_RES_LABEL_KEY_INSTANCE + + +def test_instance_id_missing(): + """instance_id が設定されていないことを確認するテスト""" + + # シングルトンをリセット（テスト用） + SpannerMetricsTracerFactory._metrics_tracer_factory = None + + # ファクトリを初期化 + factory = SpannerMetricsTracerFactory(enabled=True) + + print("=== SpannerMetricsTracerFactory の client_attributes ===") + print(f"client_attributes: {factory.client_attributes}") + print() + + # instance_id が含まれているかチェック + has_instance_id = MONITORED_RES_LABEL_KEY_INSTANCE in factory.client_attributes + print( + f"instance_id キー ({MONITORED_RES_LABEL_KEY_INSTANCE}): {'✅ 存在' if has_instance_id else '❌ 欠落'}" + ) + + if has_instance_id: + print(f" 値: {factory.client_attributes[MONITORED_RES_LABEL_KEY_INSTANCE]}") + + print() + + # MetricsTracer を作成して確認 + tracer = factory.create_metrics_tracer() + if tracer: + print("=== MetricsTracer の client_attributes ===") + print(f"client_attributes: {tracer.client_attributes}") + + has_instance_id_in_tracer = ( + MONITORED_RES_LABEL_KEY_INSTANCE in tracer.client_attributes + ) + print(f"instance_id キー: {'✅ 存在' if has_instance_id_in_tracer else '❌ 欠落'}") + else: + print("MetricsTracer: None (OpenTelemetry 未インストール)") + + print() + print("=== 結論 ===") + if not has_instance_id: + print("❌ バグ確認: instance_id が設定されていません") + print(" -> Cloud Monitoring へのエクスポート時に InvalidArgument エラーが発生します") + else: + print("✅ instance_id は正しく設定されています") + + +if __name__ == "__main__": + test_instance_id_missing() From 5af62b59f97742adf52a8c0c103ef5fdca83d5a1 Mon Sep 17 00:00:00 2001 From: shutootaki Date: Sun, 8 Feb 2026 13:51:07 +0900 Subject: [PATCH 2/4] fix(metrics): propagate project/instance to tracer factory --- .../spanner_v1/metrics/metrics_exporter.py | 9 -- .../spanner_v1/metrics/metrics_interceptor.py | 11 ++ tests/unit/test_metrics_interceptor.py | 138 ++++++++++++++++++ 3 files changed, 149 insertions(+), 9 deletions(-) diff --git a/google/cloud/spanner_v1/metrics/metrics_exporter.py b/google/cloud/spanner_v1/metrics/metrics_exporter.py index 2c1a5a234f..68da08b400 100644 --- a/google/cloud/spanner_v1/metrics/metrics_exporter.py +++ b/google/cloud/spanner_v1/metrics/metrics_exporter.py @@ -143,15 +143,6 @@ def _batch_write(self, series: List["TimeSeries"], timeout_millis: float) -> Non :param series: ProtoBuf TimeSeries :return: """ - # DEBUG: Log TimeSeries labels before sending to Cloud Monitoring - logger.warning("=== DEBUG: TimeSeries labels ===") - for ts in series: - logger.warning(f" metric.type: {ts.metric.type}") - logger.warning(f" metric.labels: {dict(ts.metric.labels)}") - logger.warning(f" resource.type: {ts.resource.type}") - logger.warning(f" resource.labels: {dict(ts.resource.labels)}") - logger.warning("=== END DEBUG ===") - write_ind = 0 timeout = timeout_millis / MILLIS_PER_SECOND while write_ind < len(series): diff --git a/google/cloud/spanner_v1/metrics/metrics_interceptor.py b/google/cloud/spanner_v1/metrics/metrics_interceptor.py index 4b55056dab..f01c07596a 100644 --- a/google/cloud/spanner_v1/metrics/metrics_interceptor.py +++ b/google/cloud/spanner_v1/metrics/metrics_interceptor.py @@ -94,6 +94,8 @@ def _set_metrics_tracer_attributes(self, resources: Dict[str, str]) -> None: This method updates the current metric tracer's attributes with the project, instance, and database information extracted from the resources dictionary. If the current metric tracer is not set, the method does nothing. + Additionally, this method updates the factory's client attributes for project and instance to ensure these values are available for subsequent operations. + Args: resources (Dict[str, str]): A dictionary containing project, instance, and database information. """ @@ -114,6 +116,15 @@ def _set_metrics_tracer_attributes(self, resources: Dict[str, str]) -> None: resources["database"] ) + # Also update factory's client attributes for project and instance + # to ensure these values are available for subsequent operations. + # Database is not set here as it may vary per operation. + factory = SpannerMetricsTracerFactory() + if "project" in resources: + factory.set_project(resources["project"]) + if "instance" in resources: + factory.set_instance(resources["instance"]) + def intercept(self, invoked_method, request_or_iterator, call_details): """Intercept gRPC calls to collect metrics. diff --git a/tests/unit/test_metrics_interceptor.py b/tests/unit/test_metrics_interceptor.py index e32003537f..91eb4fbaab 100644 --- a/tests/unit/test_metrics_interceptor.py +++ b/tests/unit/test_metrics_interceptor.py @@ -71,6 +71,144 @@ def test_set_metrics_tracer_attributes(interceptor): assert SpannerMetricsTracerFactory.current_metrics_tracer.database == "my_database" +def test_set_metrics_tracer_attributes_updates_factory(interceptor): + """Verify that factory's _client_attributes are also updated.""" + SpannerMetricsTracerFactory.current_metrics_tracer = MockMetricTracer() + factory = SpannerMetricsTracerFactory() + + # Reset factory attributes for test isolation + factory._client_attributes.pop("instance_id", None) + factory._client_attributes.pop("project_id", None) + + resources = { + "project": "test_project", + "instance": "test_instance", + "database": "test_database", + } + + interceptor._set_metrics_tracer_attributes(resources) + + # Verify factory attributes are updated + assert factory.client_attributes.get("instance_id") == "test_instance" + assert factory.client_attributes.get("project_id") == "test_project" + # Database should NOT be set in factory (it may vary per operation) + assert "database_id" not in factory.client_attributes + + +def test_set_metrics_tracer_attributes_no_tracer(interceptor): + """Verify that nothing happens when current_metrics_tracer is None.""" + SpannerMetricsTracerFactory.current_metrics_tracer = None + factory = SpannerMetricsTracerFactory() + + # Reset factory attributes for test isolation + factory._client_attributes.pop("instance_id", None) + factory._client_attributes.pop("project_id", None) + + resources = { + "project": "test_project", + "instance": "test_instance", + } + + interceptor._set_metrics_tracer_attributes(resources) + + # Factory should NOT be updated when current_metrics_tracer is None + assert "instance_id" not in factory.client_attributes + assert "project_id" not in factory.client_attributes + + +def test_set_metrics_tracer_attributes_empty_resources(interceptor): + """Verify that nothing happens when resources is empty.""" + SpannerMetricsTracerFactory.current_metrics_tracer = MockMetricTracer() + factory = SpannerMetricsTracerFactory() + + # Reset factory attributes for test isolation + factory._client_attributes.pop("instance_id", None) + factory._client_attributes.pop("project_id", None) + + interceptor._set_metrics_tracer_attributes({}) + + # Factory should NOT be updated when resources is empty + assert "instance_id" not in factory.client_attributes + assert "project_id" not in factory.client_attributes + + +def test_set_metrics_tracer_attributes_partial_resources_project_only(interceptor): + """Verify that only project is set when instance is missing.""" + SpannerMetricsTracerFactory.current_metrics_tracer = MockMetricTracer() + factory = SpannerMetricsTracerFactory() + + # Reset factory attributes for test isolation + factory._client_attributes.pop("instance_id", None) + factory._client_attributes.pop("project_id", None) + + resources = {"project": "test_project"} + + interceptor._set_metrics_tracer_attributes(resources) + + # Only project should be set + assert factory.client_attributes.get("project_id") == "test_project" + assert "instance_id" not in factory.client_attributes + assert SpannerMetricsTracerFactory.current_metrics_tracer.project == "test_project" + assert SpannerMetricsTracerFactory.current_metrics_tracer.instance is None + + +def test_set_metrics_tracer_attributes_partial_resources_instance_only(interceptor): + """Verify that only instance is set when project is missing.""" + SpannerMetricsTracerFactory.current_metrics_tracer = MockMetricTracer() + factory = SpannerMetricsTracerFactory() + + # Reset factory attributes for test isolation + factory._client_attributes.pop("instance_id", None) + factory._client_attributes.pop("project_id", None) + + resources = {"instance": "test_instance"} + + interceptor._set_metrics_tracer_attributes(resources) + + # Only instance should be set + assert factory.client_attributes.get("instance_id") == "test_instance" + assert "project_id" not in factory.client_attributes + assert ( + SpannerMetricsTracerFactory.current_metrics_tracer.instance == "test_instance" + ) + assert SpannerMetricsTracerFactory.current_metrics_tracer.project is None + + +def test_new_tracer_inherits_factory_attributes(interceptor): + """ + Integration test: Verify that a new tracer created after + _set_metrics_tracer_attributes inherits project and instance from factory. + + This is the core test for the bug fix - ensuring that subsequent operations + get tracers with the correct attributes. + """ + factory = SpannerMetricsTracerFactory() + + # Reset factory attributes for test isolation + factory._client_attributes.pop("instance_id", None) + factory._client_attributes.pop("project_id", None) + + # Simulate first operation: set up current tracer and call interceptor + SpannerMetricsTracerFactory.current_metrics_tracer = MockMetricTracer() + resources = { + "project": "inherited_project", + "instance": "inherited_instance", + "database": "db1", + } + interceptor._set_metrics_tracer_attributes(resources) + + # Simulate second operation: create a new tracer from factory + new_tracer = factory.create_metrics_tracer() + + # The new tracer should have project and instance from factory + # (This is the bug fix: before, these would be missing) + if new_tracer is not None: # None if OpenTelemetry not installed + assert new_tracer.client_attributes.get("project_id") == "inherited_project" + assert new_tracer.client_attributes.get("instance_id") == "inherited_instance" + # Database should NOT be inherited (it's per-operation) + assert "database" not in new_tracer.client_attributes + + def test_intercept_with_tracer(interceptor): SpannerMetricsTracerFactory.current_metrics_tracer = MockMetricTracer() SpannerMetricsTracerFactory.current_metrics_tracer.record_attempt_start = ( From c49453dd28c24de4d055bca229522387d86d292a Mon Sep 17 00:00:00 2001 From: shutootaki Date: Sun, 8 Feb 2026 14:28:01 +0900 Subject: [PATCH 3/4] Fix stale metrics resource labels in interceptor --- .../spanner_v1/metrics/metrics_interceptor.py | 14 +++++++ tests/unit/test_metrics_interceptor.py | 39 +++++++++++++++++-- 2 files changed, 49 insertions(+), 4 deletions(-) diff --git a/google/cloud/spanner_v1/metrics/metrics_interceptor.py b/google/cloud/spanner_v1/metrics/metrics_interceptor.py index f01c07596a..9db429af9d 100644 --- a/google/cloud/spanner_v1/metrics/metrics_interceptor.py +++ b/google/cloud/spanner_v1/metrics/metrics_interceptor.py @@ -17,6 +17,9 @@ from grpc_interceptor import ClientInterceptor from .constants import ( GOOGLE_CLOUD_RESOURCE_KEY, + METRIC_LABEL_KEY_DATABASE, + MONITORED_RES_LABEL_KEY_INSTANCE, + MONITORED_RES_LABEL_KEY_PROJECT, SPANNER_METHOD_PREFIX, ) @@ -104,14 +107,25 @@ def _set_metrics_tracer_attributes(self, resources: Dict[str, str]) -> None: if resources: if "project" in resources: + # Allow request-scoped resource data to replace stale values + # inherited from the singleton factory. + SpannerMetricsTracerFactory.current_metrics_tracer.client_attributes.pop( + MONITORED_RES_LABEL_KEY_PROJECT, None + ) SpannerMetricsTracerFactory.current_metrics_tracer.set_project( resources["project"] ) if "instance" in resources: + SpannerMetricsTracerFactory.current_metrics_tracer.client_attributes.pop( + MONITORED_RES_LABEL_KEY_INSTANCE, None + ) SpannerMetricsTracerFactory.current_metrics_tracer.set_instance( resources["instance"] ) if "database" in resources: + SpannerMetricsTracerFactory.current_metrics_tracer.client_attributes.pop( + METRIC_LABEL_KEY_DATABASE, None + ) SpannerMetricsTracerFactory.current_metrics_tracer.set_database( resources["database"] ) diff --git a/tests/unit/test_metrics_interceptor.py b/tests/unit/test_metrics_interceptor.py index 91eb4fbaab..33bf92201b 100644 --- a/tests/unit/test_metrics_interceptor.py +++ b/tests/unit/test_metrics_interceptor.py @@ -92,7 +92,7 @@ def test_set_metrics_tracer_attributes_updates_factory(interceptor): assert factory.client_attributes.get("instance_id") == "test_instance" assert factory.client_attributes.get("project_id") == "test_project" # Database should NOT be set in factory (it may vary per operation) - assert "database_id" not in factory.client_attributes + assert "database" not in factory.client_attributes def test_set_metrics_tracer_attributes_no_tracer(interceptor): @@ -174,6 +174,30 @@ def test_set_metrics_tracer_attributes_partial_resources_instance_only(intercept assert SpannerMetricsTracerFactory.current_metrics_tracer.project is None +def test_set_metrics_tracer_attributes_overwrites_stale_tracer_values(interceptor): + """Verify request resource values replace stale tracer labels.""" + stale_tracer = MockMetricTracer() + stale_tracer.set_project("stale_project") + stale_tracer.set_instance("stale_instance") + stale_tracer.set_database("stale_database") + + SpannerMetricsTracerFactory.current_metrics_tracer = stale_tracer + resources = { + "project": "fresh_project", + "instance": "fresh_instance", + "database": "fresh_database", + } + + interceptor._set_metrics_tracer_attributes(resources) + + assert stale_tracer.project == "fresh_project" + assert stale_tracer.instance == "fresh_instance" + assert stale_tracer.database == "fresh_database" + assert stale_tracer.client_attributes.get("project_id") == "fresh_project" + assert stale_tracer.client_attributes.get("instance_id") == "fresh_instance" + assert stale_tracer.client_attributes.get("database") == "fresh_database" + + def test_new_tracer_inherits_factory_attributes(interceptor): """ Integration test: Verify that a new tracer created after @@ -246,15 +270,22 @@ def __init__(self): self.instance = None self.database = None self.method = None + self.client_attributes = {} def set_project(self, project): - self.project = project + if "project_id" not in self.client_attributes: + self.project = project + self.client_attributes["project_id"] = project def set_instance(self, instance): - self.instance = instance + if "instance_id" not in self.client_attributes: + self.instance = instance + self.client_attributes["instance_id"] = instance def set_database(self, database): - self.database = database + if "database" not in self.client_attributes: + self.database = database + self.client_attributes["database"] = database def set_method(self, method): self.method = method From 6bfcc5e9933e60c423f3ec73dade60a8b1333b9a Mon Sep 17 00:00:00 2001 From: shutootaki Date: Sun, 8 Feb 2026 15:13:29 +0900 Subject: [PATCH 4/4] Fix metrics tracer updates and interceptor tests --- google/cloud/spanner_v1/client.py | 1 - .../spanner_v1/metrics/metrics_interceptor.py | 106 ++++---- .../metrics/spanner_metrics_tracer_factory.py | 2 +- test_instance_id_bug.py | 61 ----- tests/unit/test_metrics_interceptor.py | 230 +++++++++++------- 5 files changed, 200 insertions(+), 200 deletions(-) delete mode 100644 test_instance_id_bug.py diff --git a/google/cloud/spanner_v1/client.py b/google/cloud/spanner_v1/client.py index dbc33214dc..5f72905616 100644 --- a/google/cloud/spanner_v1/client.py +++ b/google/cloud/spanner_v1/client.py @@ -23,7 +23,6 @@ * a :class:`~google.cloud.spanner_v1.instance.Instance` owns a :class:`~google.cloud.spanner_v1.database.Database` """ - import grpc import os import logging diff --git a/google/cloud/spanner_v1/metrics/metrics_interceptor.py b/google/cloud/spanner_v1/metrics/metrics_interceptor.py index 9db429af9d..f6d048ac98 100644 --- a/google/cloud/spanner_v1/metrics/metrics_interceptor.py +++ b/google/cloud/spanner_v1/metrics/metrics_interceptor.py @@ -14,6 +14,8 @@ """Interceptor for collecting Cloud Spanner metrics.""" +import logging + from grpc_interceptor import ClientInterceptor from .constants import ( GOOGLE_CLOUD_RESOURCE_KEY, @@ -27,6 +29,8 @@ from .spanner_metrics_tracer_factory import SpannerMetricsTracerFactory import re +_logger = logging.getLogger(__name__) + class MetricsInterceptor(ClientInterceptor): """Interceptor that collects metrics for Cloud Spanner operations.""" @@ -95,50 +99,49 @@ def _set_metrics_tracer_attributes(self, resources: Dict[str, str]) -> None: """ Sets the metric tracer attributes based on the provided resources. - This method updates the current metric tracer's attributes with the project, instance, and database information extracted from the resources dictionary. If the current metric tracer is not set, the method does nothing. + This method updates the current metric tracer's attributes with the + project, instance, and database information extracted from the resources + dictionary. If the current metric tracer is not set, the method does + nothing. + + Before setting each attribute, any existing value for that key is removed + from the current tracer's client_attributes to ensure the set_* methods + (which use set-once semantics) can overwrite stale values from previous + operations. - Additionally, this method updates the factory's client attributes for project and instance to ensure these values are available for subsequent operations. + Additionally, this method updates the factory's client attributes for + project and instance to ensure these values are available for subsequent + operations. Database is not propagated to the factory because each + Spanner RPC may target a different database within the same instance. Args: - resources (Dict[str, str]): A dictionary containing project, instance, and database information. + resources (Dict[str, str]): A dictionary containing project, + instance, and database information. """ if SpannerMetricsTracerFactory.current_metrics_tracer is None: return if resources: - if "project" in resources: - # Allow request-scoped resource data to replace stale values - # inherited from the singleton factory. - SpannerMetricsTracerFactory.current_metrics_tracer.client_attributes.pop( - MONITORED_RES_LABEL_KEY_PROJECT, None - ) - SpannerMetricsTracerFactory.current_metrics_tracer.set_project( - resources["project"] - ) - if "instance" in resources: - SpannerMetricsTracerFactory.current_metrics_tracer.client_attributes.pop( - MONITORED_RES_LABEL_KEY_INSTANCE, None - ) - SpannerMetricsTracerFactory.current_metrics_tracer.set_instance( - resources["instance"] - ) - if "database" in resources: - SpannerMetricsTracerFactory.current_metrics_tracer.client_attributes.pop( - METRIC_LABEL_KEY_DATABASE, None - ) - SpannerMetricsTracerFactory.current_metrics_tracer.set_database( - resources["database"] - ) - - # Also update factory's client attributes for project and instance - # to ensure these values are available for subsequent operations. - # Database is not set here as it may vary per operation. + tracer = SpannerMetricsTracerFactory.current_metrics_tracer factory = SpannerMetricsTracerFactory() + + # For each resource key, remove the existing value from + # client_attributes so the tracer's set_* method (which only + # writes if the key is absent) will accept the fresh value. if "project" in resources: + tracer.client_attributes.pop(MONITORED_RES_LABEL_KEY_PROJECT, None) + tracer.set_project(resources["project"]) factory.set_project(resources["project"]) + if "instance" in resources: + tracer.client_attributes.pop(MONITORED_RES_LABEL_KEY_INSTANCE, None) + tracer.set_instance(resources["instance"]) factory.set_instance(resources["instance"]) + if "database" in resources: + tracer.client_attributes.pop(METRIC_LABEL_KEY_DATABASE, None) + tracer.set_database(resources["database"]) + def intercept(self, invoked_method, request_or_iterator, call_details): """Intercept gRPC calls to collect metrics. @@ -158,24 +161,33 @@ def intercept(self, invoked_method, request_or_iterator, call_details): return invoked_method(request_or_iterator, call_details) # Setup Metric Tracer attributes from call details - ## Extract Project / Instance / Databse from header information - resources = self._extract_resource_from_path(call_details.metadata) - self._set_metrics_tracer_attributes(resources) + try: + ## Extract Project / Instance / Database from header information + resources = self._extract_resource_from_path(call_details.metadata) + self._set_metrics_tracer_attributes(resources) - ## Format method to be be spanner. - method_name = self._remove_prefix( - call_details.method, SPANNER_METHOD_PREFIX - ).replace("/", ".") + ## Format method to be spanner. + method_name = self._remove_prefix( + call_details.method, SPANNER_METHOD_PREFIX + ).replace("/", ".") + + SpannerMetricsTracerFactory.current_metrics_tracer.set_method(method_name) + SpannerMetricsTracerFactory.current_metrics_tracer.record_attempt_start() + except Exception: + _logger.warning("Failed to set up metrics tracer attributes", exc_info=True) - SpannerMetricsTracerFactory.current_metrics_tracer.set_method(method_name) - SpannerMetricsTracerFactory.current_metrics_tracer.record_attempt_start() response = invoked_method(request_or_iterator, call_details) - SpannerMetricsTracerFactory.current_metrics_tracer.record_attempt_completion() - - # Process and send GFE metrics if enabled - if SpannerMetricsTracerFactory.current_metrics_tracer.gfe_enabled: - metadata = response.initial_metadata() - SpannerMetricsTracerFactory.current_metrics_trace.record_gfe_metrics( - metadata - ) + + try: + SpannerMetricsTracerFactory.current_metrics_tracer.record_attempt_completion() + + # Process and send GFE metrics if enabled + if SpannerMetricsTracerFactory.current_metrics_tracer.gfe_enabled: + metadata = response.initial_metadata() + SpannerMetricsTracerFactory.current_metrics_tracer.record_gfe_metrics( + metadata + ) + except Exception: + _logger.warning("Failed to record metrics", exc_info=True) + return response diff --git a/google/cloud/spanner_v1/metrics/spanner_metrics_tracer_factory.py b/google/cloud/spanner_v1/metrics/spanner_metrics_tracer_factory.py index 9566e61a28..89dae1fd0c 100644 --- a/google/cloud/spanner_v1/metrics/spanner_metrics_tracer_factory.py +++ b/google/cloud/spanner_v1/metrics/spanner_metrics_tracer_factory.py @@ -80,7 +80,7 @@ def __new__( cls._metrics_tracer_factory.gfe_enabled = gfe_enabled if cls._metrics_tracer_factory.enabled != enabled: - cls._metrics_tracer_factory.enabeld = enabled + cls._metrics_tracer_factory.enabled = enabled return cls._metrics_tracer_factory diff --git a/test_instance_id_bug.py b/test_instance_id_bug.py deleted file mode 100644 index 30dd45d040..0000000000 --- a/test_instance_id_bug.py +++ /dev/null @@ -1,61 +0,0 @@ -""" -instance_id 欠落バグの再現テスト - -このスクリプトは、SpannerMetricsTracerFactory が初期化された後、 -instance_id が client_attributes に含まれていないことを確認します。 -""" - -from google.cloud.spanner_v1.metrics.spanner_metrics_tracer_factory import ( - SpannerMetricsTracerFactory, -) -from google.cloud.spanner_v1.metrics.constants import MONITORED_RES_LABEL_KEY_INSTANCE - - -def test_instance_id_missing(): - """instance_id が設定されていないことを確認するテスト""" - - # シングルトンをリセット（テスト用） - SpannerMetricsTracerFactory._metrics_tracer_factory = None - - # ファクトリを初期化 - factory = SpannerMetricsTracerFactory(enabled=True) - - print("=== SpannerMetricsTracerFactory の client_attributes ===") - print(f"client_attributes: {factory.client_attributes}") - print() - - # instance_id が含まれているかチェック - has_instance_id = MONITORED_RES_LABEL_KEY_INSTANCE in factory.client_attributes - print( - f"instance_id キー ({MONITORED_RES_LABEL_KEY_INSTANCE}): {'✅ 存在' if has_instance_id else '❌ 欠落'}" - ) - - if has_instance_id: - print(f" 値: {factory.client_attributes[MONITORED_RES_LABEL_KEY_INSTANCE]}") - - print() - - # MetricsTracer を作成して確認 - tracer = factory.create_metrics_tracer() - if tracer: - print("=== MetricsTracer の client_attributes ===") - print(f"client_attributes: {tracer.client_attributes}") - - has_instance_id_in_tracer = ( - MONITORED_RES_LABEL_KEY_INSTANCE in tracer.client_attributes - ) - print(f"instance_id キー: {'✅ 存在' if has_instance_id_in_tracer else '❌ 欠落'}") - else: - print("MetricsTracer: None (OpenTelemetry 未インストール)") - - print() - print("=== 結論 ===") - if not has_instance_id: - print("❌ バグ確認: instance_id が設定されていません") - print(" -> Cloud Monitoring へのエクスポート時に InvalidArgument エラーが発生します") - else: - print("✅ instance_id は正しく設定されています") - - -if __name__ == "__main__": - test_instance_id_missing() diff --git a/tests/unit/test_metrics_interceptor.py b/tests/unit/test_metrics_interceptor.py index 33bf92201b..285ad0f9cf 100644 --- a/tests/unit/test_metrics_interceptor.py +++ b/tests/unit/test_metrics_interceptor.py @@ -26,6 +26,25 @@ def interceptor(): return MetricsInterceptor() +@pytest.fixture +def clean_factory(): + """Return a factory with project_id and instance_id cleared for test isolation.""" + factory = SpannerMetricsTracerFactory() + factory._client_attributes.pop("instance_id", None) + factory._client_attributes.pop("project_id", None) + return factory + + +@pytest.fixture +def mock_tracer(clean_factory): + """Set up a clean MockMetricTracer and return the factory.""" + SpannerMetricsTracerFactory.current_metrics_tracer = MockMetricTracer() + return clean_factory + + +# --- Parsing tests --- + + def test_parse_resource_path_valid(interceptor): path = "projects/my_project/instances/my_instance/databases/my_database" expected = { @@ -57,8 +76,11 @@ def test_extract_resource_from_path(interceptor): assert interceptor._extract_resource_from_path(metadata) == expected -def test_set_metrics_tracer_attributes(interceptor): - SpannerMetricsTracerFactory.current_metrics_tracer = MockMetricTracer() +# --- _set_metrics_tracer_attributes tests --- + + +def test_set_metrics_tracer_attributes(interceptor, mock_tracer): + """Verify tracer receives project, instance, and database from resources.""" resources = { "project": "my_project", "instance": "my_instance", @@ -66,20 +88,15 @@ def test_set_metrics_tracer_attributes(interceptor): } interceptor._set_metrics_tracer_attributes(resources) - assert SpannerMetricsTracerFactory.current_metrics_tracer.project == "my_project" - assert SpannerMetricsTracerFactory.current_metrics_tracer.instance == "my_instance" - assert SpannerMetricsTracerFactory.current_metrics_tracer.database == "my_database" + tracer = SpannerMetricsTracerFactory.current_metrics_tracer + assert tracer.client_attributes.get("project_id") == "my_project" + assert tracer.client_attributes.get("instance_id") == "my_instance" + assert tracer.client_attributes.get("database") == "my_database" -def test_set_metrics_tracer_attributes_updates_factory(interceptor): - """Verify that factory's _client_attributes are also updated.""" - SpannerMetricsTracerFactory.current_metrics_tracer = MockMetricTracer() - factory = SpannerMetricsTracerFactory() - - # Reset factory attributes for test isolation - factory._client_attributes.pop("instance_id", None) - factory._client_attributes.pop("project_id", None) +def test_set_metrics_tracer_attributes_updates_factory(interceptor, mock_tracer): + """Verify that the factory's client_attributes are updated with project and instance.""" resources = { "project": "test_project", "instance": "test_instance", @@ -89,20 +106,15 @@ def test_set_metrics_tracer_attributes_updates_factory(interceptor): interceptor._set_metrics_tracer_attributes(resources) # Verify factory attributes are updated - assert factory.client_attributes.get("instance_id") == "test_instance" - assert factory.client_attributes.get("project_id") == "test_project" + assert mock_tracer.client_attributes.get("instance_id") == "test_instance" + assert mock_tracer.client_attributes.get("project_id") == "test_project" # Database should NOT be set in factory (it may vary per operation) - assert "database" not in factory.client_attributes + assert "database" not in mock_tracer.client_attributes -def test_set_metrics_tracer_attributes_no_tracer(interceptor): +def test_set_metrics_tracer_attributes_no_tracer(interceptor, clean_factory): """Verify that nothing happens when current_metrics_tracer is None.""" SpannerMetricsTracerFactory.current_metrics_tracer = None - factory = SpannerMetricsTracerFactory() - - # Reset factory attributes for test isolation - factory._client_attributes.pop("instance_id", None) - factory._client_attributes.pop("project_id", None) resources = { "project": "test_project", @@ -112,74 +124,81 @@ def test_set_metrics_tracer_attributes_no_tracer(interceptor): interceptor._set_metrics_tracer_attributes(resources) # Factory should NOT be updated when current_metrics_tracer is None - assert "instance_id" not in factory.client_attributes - assert "project_id" not in factory.client_attributes + assert "instance_id" not in clean_factory.client_attributes + assert "project_id" not in clean_factory.client_attributes -def test_set_metrics_tracer_attributes_empty_resources(interceptor): +def test_set_metrics_tracer_attributes_empty_resources(interceptor, mock_tracer): """Verify that nothing happens when resources is empty.""" - SpannerMetricsTracerFactory.current_metrics_tracer = MockMetricTracer() - factory = SpannerMetricsTracerFactory() - - # Reset factory attributes for test isolation - factory._client_attributes.pop("instance_id", None) - factory._client_attributes.pop("project_id", None) - interceptor._set_metrics_tracer_attributes({}) - # Factory should NOT be updated when resources is empty - assert "instance_id" not in factory.client_attributes - assert "project_id" not in factory.client_attributes + assert "instance_id" not in mock_tracer.client_attributes + assert "project_id" not in mock_tracer.client_attributes -def test_set_metrics_tracer_attributes_partial_resources_project_only(interceptor): - """Verify that only project is set when instance is missing.""" - SpannerMetricsTracerFactory.current_metrics_tracer = MockMetricTracer() - factory = SpannerMetricsTracerFactory() +def test_set_metrics_tracer_attributes_none_resources(interceptor, mock_tracer): + """Verify that nothing happens when resources is None.""" + interceptor._set_metrics_tracer_attributes(None) - # Reset factory attributes for test isolation - factory._client_attributes.pop("instance_id", None) - factory._client_attributes.pop("project_id", None) + assert "instance_id" not in mock_tracer.client_attributes + assert "project_id" not in mock_tracer.client_attributes + +def test_set_metrics_tracer_attributes_partial_resources_project_only( + interceptor, mock_tracer +): + """Verify that only project is set when instance is missing.""" resources = {"project": "test_project"} interceptor._set_metrics_tracer_attributes(resources) # Only project should be set - assert factory.client_attributes.get("project_id") == "test_project" - assert "instance_id" not in factory.client_attributes - assert SpannerMetricsTracerFactory.current_metrics_tracer.project == "test_project" - assert SpannerMetricsTracerFactory.current_metrics_tracer.instance is None + assert mock_tracer.client_attributes.get("project_id") == "test_project" + assert "instance_id" not in mock_tracer.client_attributes + tracer = SpannerMetricsTracerFactory.current_metrics_tracer + assert tracer.client_attributes.get("project_id") == "test_project" + assert "instance_id" not in tracer.client_attributes -def test_set_metrics_tracer_attributes_partial_resources_instance_only(interceptor): +def test_set_metrics_tracer_attributes_partial_resources_instance_only( + interceptor, mock_tracer +): """Verify that only instance is set when project is missing.""" - SpannerMetricsTracerFactory.current_metrics_tracer = MockMetricTracer() - factory = SpannerMetricsTracerFactory() - - # Reset factory attributes for test isolation - factory._client_attributes.pop("instance_id", None) - factory._client_attributes.pop("project_id", None) - resources = {"instance": "test_instance"} interceptor._set_metrics_tracer_attributes(resources) # Only instance should be set - assert factory.client_attributes.get("instance_id") == "test_instance" - assert "project_id" not in factory.client_attributes - assert ( - SpannerMetricsTracerFactory.current_metrics_tracer.instance == "test_instance" - ) - assert SpannerMetricsTracerFactory.current_metrics_tracer.project is None + assert mock_tracer.client_attributes.get("instance_id") == "test_instance" + assert "project_id" not in mock_tracer.client_attributes + tracer = SpannerMetricsTracerFactory.current_metrics_tracer + assert tracer.client_attributes.get("instance_id") == "test_instance" + assert "project_id" not in tracer.client_attributes + + +def test_set_metrics_tracer_attributes_partial_resources_database_only( + interceptor, mock_tracer +): + """Verify that database is set on tracer but NOT propagated to factory.""" + resources = {"database": "test_database"} + + interceptor._set_metrics_tracer_attributes(resources) + + tracer = SpannerMetricsTracerFactory.current_metrics_tracer + assert tracer.client_attributes.get("database") == "test_database" + # Factory should NOT have database, project, or instance + assert "database" not in mock_tracer.client_attributes + assert "project_id" not in mock_tracer.client_attributes + assert "instance_id" not in mock_tracer.client_attributes def test_set_metrics_tracer_attributes_overwrites_stale_tracer_values(interceptor): - """Verify request resource values replace stale tracer labels.""" + """Verify that request resource values replace stale tracer client_attributes.""" stale_tracer = MockMetricTracer() - stale_tracer.set_project("stale_project") - stale_tracer.set_instance("stale_instance") - stale_tracer.set_database("stale_database") + # Directly populate to simulate stale values from factory copy + stale_tracer.client_attributes["project_id"] = "stale_project" + stale_tracer.client_attributes["instance_id"] = "stale_instance" + stale_tracer.client_attributes["database"] = "stale_database" SpannerMetricsTracerFactory.current_metrics_tracer = stale_tracer resources = { @@ -190,15 +209,12 @@ def test_set_metrics_tracer_attributes_overwrites_stale_tracer_values(intercepto interceptor._set_metrics_tracer_attributes(resources) - assert stale_tracer.project == "fresh_project" - assert stale_tracer.instance == "fresh_instance" - assert stale_tracer.database == "fresh_database" assert stale_tracer.client_attributes.get("project_id") == "fresh_project" assert stale_tracer.client_attributes.get("instance_id") == "fresh_instance" assert stale_tracer.client_attributes.get("database") == "fresh_database" -def test_new_tracer_inherits_factory_attributes(interceptor): +def test_new_tracer_inherits_factory_attributes(interceptor, mock_tracer): """ Integration test: Verify that a new tracer created after _set_metrics_tracer_attributes inherits project and instance from factory. @@ -206,14 +222,6 @@ def test_new_tracer_inherits_factory_attributes(interceptor): This is the core test for the bug fix - ensuring that subsequent operations get tracers with the correct attributes. """ - factory = SpannerMetricsTracerFactory() - - # Reset factory attributes for test isolation - factory._client_attributes.pop("instance_id", None) - factory._client_attributes.pop("project_id", None) - - # Simulate first operation: set up current tracer and call interceptor - SpannerMetricsTracerFactory.current_metrics_tracer = MockMetricTracer() resources = { "project": "inherited_project", "instance": "inherited_instance", @@ -222,18 +230,22 @@ def test_new_tracer_inherits_factory_attributes(interceptor): interceptor._set_metrics_tracer_attributes(resources) # Simulate second operation: create a new tracer from factory - new_tracer = factory.create_metrics_tracer() + new_tracer = mock_tracer.create_metrics_tracer() - # The new tracer should have project and instance from factory - # (This is the bug fix: before, these would be missing) - if new_tracer is not None: # None if OpenTelemetry not installed - assert new_tracer.client_attributes.get("project_id") == "inherited_project" - assert new_tracer.client_attributes.get("instance_id") == "inherited_instance" - # Database should NOT be inherited (it's per-operation) - assert "database" not in new_tracer.client_attributes + if new_tracer is None: + pytest.skip("OpenTelemetry not installed; cannot verify tracer inheritance") + + assert new_tracer.client_attributes.get("project_id") == "inherited_project" + assert new_tracer.client_attributes.get("instance_id") == "inherited_instance" + # Database should NOT be inherited (it's per-operation) + assert "database" not in new_tracer.client_attributes + + +# --- intercept tests --- def test_intercept_with_tracer(interceptor): + """Verify intercept records metrics and invokes the gRPC method.""" SpannerMetricsTracerFactory.current_metrics_tracer = MockMetricTracer() SpannerMetricsTracerFactory.current_metrics_tracer.record_attempt_start = ( MagicMock() @@ -264,27 +276,65 @@ def test_intercept_with_tracer(interceptor): mock_invoked_method.assert_called_once_with("request", call_details) +def test_intercept_no_tracer(interceptor): + """Verify that intercept returns directly when current_metrics_tracer is None.""" + SpannerMetricsTracerFactory.current_metrics_tracer = None + + invoked_response = MagicMock() + mock_invoked_method = MagicMock(return_value=invoked_response) + call_details = MagicMock(method="spanner.someMethod", metadata=[]) + + response = interceptor.intercept(mock_invoked_method, "request", call_details) + assert response == invoked_response + mock_invoked_method.assert_called_once_with("request", call_details) + + +def test_intercept_factory_disabled(interceptor): + """Verify that intercept returns directly when factory is disabled.""" + SpannerMetricsTracerFactory.current_metrics_tracer = MockMetricTracer() + factory = SpannerMetricsTracerFactory() + original_enabled = factory.enabled + + try: + factory.enabled = False + + invoked_response = MagicMock() + mock_invoked_method = MagicMock(return_value=invoked_response) + call_details = MagicMock(method="spanner.someMethod", metadata=[]) + + response = interceptor.intercept(mock_invoked_method, "request", call_details) + assert response == invoked_response + mock_invoked_method.assert_called_once_with("request", call_details) + finally: + factory.enabled = original_enabled + + +# --- MockMetricTracer --- + + class MockMetricTracer: + """Mock that mirrors MetricsTracer's set-once semantics. + + The real MetricsTracer.set_project/set_instance/set_database only write + if the key is absent from _client_attributes. This guard is critical for + testing the pop-then-set pattern in _set_metrics_tracer_attributes. + """ + def __init__(self): - self.project = None - self.instance = None - self.database = None self.method = None self.client_attributes = {} + self.gfe_enabled = False def set_project(self, project): if "project_id" not in self.client_attributes: - self.project = project self.client_attributes["project_id"] = project def set_instance(self, instance): if "instance_id" not in self.client_attributes: - self.instance = instance self.client_attributes["instance_id"] = instance def set_database(self, database): if "database" not in self.client_attributes: - self.database = database self.client_attributes["database"] = database def set_method(self, method):