feat(gapic): add OpenTelemetry T3 client method span wrapping in gapic_v1.method (D) - #18274
feat(gapic): add OpenTelemetry T3 client method span wrapping in gapic_v1.method (D)#18274chalmerlowe wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request adds OpenTelemetry tracing support to _GapicCallable in google/api_core/gapic_v1/method.py and includes unit tests to verify the tracing behavior. The review feedback identifies a critical reliability issue: catching only ImportError during OTel setup could allow other exceptions to crash the API call, violating the self-contained fallback rule. Additionally, broadening the exception handling introduces a double execution risk for the wrapped function if it fails. A robust solution using a state flag is proposed to safely handle setup errors while ensuring wrapped function exceptions propagate correctly.
| if _observability.is_otel_capabilities_enabled(): | ||
| try: | ||
| from opentelemetry import trace | ||
|
|
||
| tracer = trace.get_tracer("google.api_core") | ||
| raw_method = getattr(self._target, "_method", None) | ||
| if raw_method and isinstance(raw_method, (str, bytes)): | ||
| if isinstance(raw_method, bytes): | ||
| raw_method = raw_method.decode("utf-8") | ||
| method_str = raw_method.lstrip("/") | ||
| service, _, method = method_str.rpartition("/") | ||
| span_name = method_str | ||
| else: | ||
| service = "google.api_core" | ||
| method = getattr(self._target, "__name__", "call") | ||
| span_name = f"{service}/{method}" | ||
|
|
||
| with tracer.start_as_current_span( | ||
| span_name, | ||
| kind=trace.SpanKind.CLIENT, | ||
| attributes={ | ||
| "rpc.system": "grpc", | ||
| "rpc.service": service, | ||
| "rpc.method": method, | ||
| }, | ||
| ) as span: | ||
| try: | ||
| return wrapped_func(*args, **kwargs) | ||
| except Exception as exc: | ||
| span.record_exception(exc) | ||
| span.set_status(trace.StatusCode.ERROR, str(exc)) | ||
| raise | ||
| except ImportError: | ||
| pass |
There was a problem hiding this comment.
Critical Reliability & Correctness Issues
- Resilience of Fallback Logic: Currently, only
ImportErroris caught. If any other exception occurs during OpenTelemetry setup, tracer retrieval, or span creation (e.g.,AttributeError,TypeError,UnicodeDecodeErrorwhen decoding_method, or OTel initialization/configuration errors), it will propagate and crash the user's API call. Per the Repository Style Guide (Rule 3: Self-Contained Fallbacks), fallback logic must be resilient and self-contained, bypassing failures gracefully. - Double Execution Risk: If we simply broaden the exception handler to
except Exception:, any exception raised bywrapped_func(which is caught and re-raised by the innerexcept Exception as exc) would be caught by the outerexcept Exceptionand trigger a second execution ofwrapped_func(*args, **kwargs). This is a critical bug that could cause non-idempotent RPCs to be executed twice.
Solution
We can use a state flag (func_called) to track whether wrapped_func has been invoked. This allows us to catch all exceptions during OTel setup/span creation and fallback gracefully, while ensuring that any exception raised by wrapped_func itself is propagated immediately without triggering a double execution.
if _observability.is_otel_capabilities_enabled():
func_called = False
try:
from opentelemetry import trace
tracer = trace.get_tracer("google.api_core")
raw_method = getattr(self._target, "_method", None)
if raw_method and isinstance(raw_method, (str, bytes)):
if isinstance(raw_method, bytes):
raw_method = raw_method.decode("utf-8")
method_str = raw_method.lstrip("/")
service, _, method = method_str.rpartition("/")
span_name = method_str
else:
service = "google.api_core"
method = getattr(self._target, "__name__", "call")
span_name = f"{service}/{method}"
with tracer.start_as_current_span(
span_name,
kind=trace.SpanKind.CLIENT,
attributes={
"rpc.system": "grpc",
"rpc.service": service,
"rpc.method": method,
},
) as span:
try:
func_called = True
return wrapped_func(*args, **kwargs)
except Exception as exc:
span.record_exception(exc)
span.set_status(trace.StatusCode.ERROR, str(exc))
raise
except Exception:
if func_called:
raiseReferences
- Rule 3: Self-Contained Fallbacks - Fallback logic must be resilient and self-contained. Always wrap fallback configuration loading in try-except blocks to catch expected exceptions and bypass failures gracefully. (link)
| span.record_exception(exc) | ||
| span.set_status(trace.StatusCode.ERROR, str(exc)) | ||
| raise | ||
| except ImportError: |
There was a problem hiding this comment.
Note
This is purposeful minimal error handling to allow us to discuss the overall approach, without getting bogged down in the minutiae.
With approval of the approach, I will update the error handling and tests.
bf82825 to
bc94977
Compare
bc94977 to
67e3879
Compare
Warning
This PR is NOT intended to be merged. It is a proof-of-concept intended to inform the design and implementation of changes that need to be made in the GAPIC Generator templates.
Summary
This PR implements Tier 3 (T3) Client Method Spans in
google-api-core:google.api_core.gapic_v1.method._GapicCallable.__call__:SpanKind.CLIENTspan representing the high-level GAPIC SDK method call (e.g.google.cloud.secretmanager.v1.SecretManagerService/ListSecrets).rpc.system = "grpc",rpc.service,rpc.method).packages/google-api-core/tests/unit/gapic/test_method.py.Reference: