fix(metrics): correct GFE metrics extraction and enable by default by sinhasubham · Pull Request #17561 · googleapis/google-cloud-python

sinhasubham · 2026-06-24T07:14:28Z

Description

This PR enables and properly extracts Spanner Google Front End (GFE) and Application Front End (AFE) latency metrics.

Key Changes:

Fix Metadata Extraction Logic: Modified MetricsInterceptor and AsyncMetricsInterceptor to strictly extract server-timing data from initial_metadata().
Add AFE Metrics Publishing: Extended MetricsTracer to parse AFE latency (afe;\s*dur=) from the server-timing header alongside GFE metrics. Two new OpenTelemetry instruments (afe_latency and afe_missing_header_count) have been introduced to publish these.
Support for Streaming RPCs: Removed the manual toggle in SpannerMetricsTracerFactory. GFE and AFE metrics capture is now always-on whenever OpenTelemetry tracing is enabled.
Enable Frontend Metrics by Default: Removed the gfe_enabled toggle in SpannerMetricsTracerFactory. GFE metrics capture is now always-on whenever OpenTelemetry tracing is enabled.
Testing: Added unit tests and a new mockserver test (test_frontend_metrics.py) to ensure both GFE and AFE metrics are correctly published end-to-end.
Local Testing Screenshot: https://screenshot.googleplex.com/image/4tmjwuPjzdttcjW.png

gemini-code-assist

Code Review

This pull request introduces deferred metrics recording for streaming and async streaming RPC responses in Google Cloud Spanner by wrapping responses in specialized wrappers. It also implements GFE latency extraction from response metadata and adds corresponding tests. The review feedback highlights several critical improvements for robustness: refining streaming response detection to check for iterator methods (next and anext) rather than iterable methods to prevent incorrect wrapping of standard iterables; wrapping telemetry and metrics recording blocks in try-except blocks to ensure telemetry failures do not disrupt the main application flow; defensively validating metadata elements before unpacking to avoid unpacking errors; and properly decoding bytes metadata values before regex matching.

parthea · 2026-06-26T15:16:41Z

@sinhasubham, Please merge main into this branch. The presubmit failures were addressed in #17578

surbhigarg92 · 2026-07-16T04:49:05Z

+        instrument_gfe_latency: Optional["Histogram"] = None,
+        instrument_gfe_missing_header_count: Optional["Counter"] = None,
+        instrument_afe_latency: Optional["Histogram"] = None,
+        instrument_afe_missing_header_count: Optional["Counter"] = None,


Why Optional ?

surbhigarg92 · 2026-07-16T04:51:56Z

+            self._tracer.record_gfe_metrics(metadata)
+            self._tracer.record_afe_metrics(metadata)


Both methods are extracting latency values from metadata seprarately, this can be optimized to fetch both afe and gfe latency values from the metadata once, and pass it in these recorder methods

surbhigarg92 · 2026-07-16T04:56:28Z

+    if hasattr(response, "__next__"):
+        return _StreamingResponseWrapper(response, tracer)


Instead of checking this, cant we have interceptor listening to different events.

For eg: refer Node code, similar thing is used in Java and Go as well https://github.com/googleapis/google-cloud-node/blob/main/handwritten/spanner/src/metrics/interceptor.ts#L65

We have events like onReceiveStatus onReceiveMetadata ,

onReceiveStatus will only be called after the requests has finished execution which should trigger record_attempt_completion

onReceiveMetadata should be the place where we capture the metadata and use the value while calling record_gfe_metrics from onReceiveStatus

Unlike Node.js, Java or Go: Python grpc and grpc.aio client interceptor APIs do not natively expose event-based lifecycle hooks (like onReceiveMetadata or onReceiveStatus).

surbhigarg92 · 2026-07-16T05:16:31Z

/gemini review

gemini-code-assist

Code Review

This pull request adds support for collecting and exporting Application Front End (AFE) metrics alongside Google Front End (GFE) metrics, and introduces an AsyncMetricsInterceptor to handle asynchronous gRPC calls. The review feedback highlights critical issues: first, passing call_details.metadata directly to _extract_resource_from_path in the async interceptor will raise an AttributeError because it is a grpc.aio.Metadata object rather than a dictionary; second, metadata keys can be bytes in some gRPC environments, meaning str(key) will produce "b'server-timing'" and fail string comparisons in both extract_gfe_latency and extract_afe_latency.

surbhigarg92 · 2026-07-16T05:19:37Z

+METRIC_NAME_AFE_LATENCY = "afe_latency"
+METRIC_NAME_AFE_MISSING_HEADER_COUNT = "afe_missing_header_count"


Metric names are "gfe_latencies" and "afe_latencies" .

Not sure how were you able to export metrics to Cloud Monarch and Monitoring ? The screenshot attached shows metrics in "Generic Task" , it should be under Spanner -> Client with full name as spanner.googleapis.com/client/afe_latencies

Addressed. updated test screenshot

… specs

surbhigarg92

If possible also add a integration test similar to https://github.com/googleapis/google-cloud-java/blob/main/java-spanner/google-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITBuiltInMetricsTest.java#L87

surbhigarg92 · 2026-07-27T06:05:05Z

-        )
+        method_str = call_details.method
+        if isinstance(method_str, bytes):
+            method_str = method_str.decode("utf-8")


Why do we need this ? Why would method name have the encoding ? It was not there earlier, which case was failing because of it ?

surbhigarg92 · 2026-07-27T06:06:05Z

        tracer.set_method(method_name)
        tracer.record_attempt_start()
+
+        if os.environ.get("SPANNER_DISABLE_AFE_SERVER_TIMING", "").lower() != "true":


Instead of getting this from ENV for every call, can we fetch it once during client init and pass it to the interceptor ?

surbhigarg92 · 2026-07-27T06:14:59Z

+        if os.environ.get("SPANNER_DISABLE_AFE_SERVER_TIMING", "").lower() != "true":
+            metadata = list(call_details.metadata or [])
+            metadata.append(("x-goog-spanner-enable-afe-server-timing", "true"))
+            call_details = call_details._replace(metadata=metadata)
+


nit: Good to create a method in helpers file for appending metadata like we have for all other headers

https://github.com/googleapis/google-cloud-python/blob/main/packages/google-cloud-spanner/google/cloud/spanner_v1/_helpers.py#L803

surbhigarg92 · 2026-07-27T06:18:48Z

+        if os.environ.get("SPANNER_DISABLE_AFE_SERVER_TIMING", "").lower() != "true":
+            metadata = list(call_details.metadata or [])
+            metadata.append(("x-goog-spanner-enable-afe-server-timing", "true"))
+            call_details = call_details._replace(metadata=metadata)


Instead of doing call_details._replace which might be a little expensive operation , this can be done with all other metadata.

Eg: We could do this where we are appending prefix metadata which is already called for every operation

https://github.com/googleapis/google-cloud-python/blob/main/packages/google-cloud-spanner/google/cloud/spanner_v1/_helpers.py#L710C5-L710C26

surbhigarg92 · 2026-07-27T06:21:08Z

+        )
        self.enabled = enabled
-        self.gfe_enabled = gfe_enabled
+        self.gfe_enabled = True


nit: We could remove this

surbhigarg92 · 2026-07-27T06:23:49Z

+            not self.enabled
+            or not HAS_OPENTELEMETRY_INSTALLED
+            or not getattr(self, "_instrument_afe_latency", None)
+            or os.environ.get("SPANNER_DISABLE_AFE_SERVER_TIMING", "").lower() == "true"


nit: Same here, dont fetch from ENV for every call, have a static property and use it.

surbhigarg92 · 2026-07-27T06:24:53Z

+        header_vals = []
+        for key, val in items:
+            key_str = key.decode("utf-8") if isinstance(key, bytes) else str(key)
+            if key_str and key_str.lower() in ("server-timing", "server_timing"):


We are only capturing "server-timing" in all other clients not "server_timing"

surbhigarg92 · 2026-07-27T06:26:05Z

+                        pass
+
+            if afe_latency is None:
+                match = re.search(r"afe(?:t4t7)?;\s*dur=([0-9.]+)", header_val)


nit: Optional "t4t7" is not required, it will always be like "/afe; dur=([0-9]+).*/"

…logic

brunoprado-ux approved these changes Jun 24, 2026

View reviewed changes

gemini-code-assist Bot reviewed Jun 24, 2026

View reviewed changes

parthea assigned sinhasubham Jun 26, 2026

sinhasubham force-pushed the gfe-metrics-fix branch 6 times, most recently from a495bbb to 82f08c2 Compare July 1, 2026 09:38

sinhasubham marked this pull request as ready for review July 6, 2026 06:11

sinhasubham requested a review from a team as a code owner July 6, 2026 06:11

sinhasubham requested a review from surbhigarg92 July 6, 2026 06:11

sinhasubham force-pushed the gfe-metrics-fix branch from 6c8800b to dd8522b Compare July 14, 2026 09:15

surbhigarg92 reviewed Jul 16, 2026

View reviewed changes

gemini-code-assist Bot reviewed Jul 16, 2026

View reviewed changes

surbhigarg92 reviewed Jul 16, 2026

View reviewed changes

sinhasubham force-pushed the gfe-metrics-fix branch 4 times, most recently from ceaa773 to 644fe1c Compare July 26, 2026 05:14

sinhasubham and others added 5 commits July 26, 2026 10:49

fix(metrics): correct GFE metrics extraction and enable by default

bc4ca60

feat(metrics): add AFE latency metrics and simplify metadata extraction

ee696db

fix(spanner): align GFE/AFE metric names and instruments with backend…

30e3122

… specs

feat(metrics): add support for AFE server timings

d938f30

fix(metrics): fix OpenTelemetry resource detector import path

df80428

sinhasubham force-pushed the gfe-metrics-fix branch from 644fe1c to df80428 Compare July 26, 2026 05:29

surbhigarg92 reviewed Jul 27, 2026

View reviewed changes

fix(metrics): remove obsolete gfe_enabled flag and refine AFE timing …

a1532ce

…logic

		self._tracer.record_gfe_metrics(metadata)
		self._tracer.record_afe_metrics(metadata)

		if hasattr(response, "__next__"):
		return _StreamingResponseWrapper(response, tracer)

		METRIC_NAME_AFE_LATENCY = "afe_latency"
		METRIC_NAME_AFE_MISSING_HEADER_COUNT = "afe_missing_header_count"

Uh oh!

Conversation

sinhasubham commented Jun 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Key Changes:

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

parthea commented Jun 26, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

surbhigarg92 commented Jul 16, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

surbhigarg92 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sinhasubham commented Jun 24, 2026 •

edited

Loading