[Python] Open Telemetry tracing (part 2)#42190
Conversation
Co-authored-by: Sergii Tkachenko <sergiitk@google.com>
Co-authored-by: Sergii Tkachenko <sergiitk@google.com>
2f1e5f4 to
aad0d5e
Compare
aad0d5e to
ca8cd00
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds OpenTelemetry tracing support to gRPC Python for both sync and async (AIO) calls, enabling span context propagation and recording of span events. The implementation involves updates to Cython bindings, context management, and the addition of extensive tests. Feedback highlights a critical issue where the OpenTelemetry Tracer's internal ID generator is incorrectly accessed, which would prevent ID propagation, and recommends defensive programming when accessing the plugin list to avoid index errors.
| # to prevent concurrent RPCs from overwriting shared state | ||
| with self._tracer_lock: | ||
| # this step is needed to propagate span ID correctly | ||
| self._tracer.id_generator = _GrpcIdGenerator( |
There was a problem hiding this comment.
The opentelemetry.sdk.trace.Tracer class in the OpenTelemetry Python SDK uses _id_generator (with a leading underscore) as its internal attribute for the ID generator. Using id_generator without the underscore will create a new attribute that the SDK's start_span method will ignore, causing the custom ID propagation to fail.
| self._tracer.id_generator = _GrpcIdGenerator( | |
| self._tracer._id_generator = _GrpcIdGenerator( |
| current_span = trace.get_current_span( | ||
| context=self._plugins[0].get_trace_context() | ||
| ).get_span_context() |
There was a problem hiding this comment.
Accessing self._plugins[0] directly can lead to an IndexError if the _plugins list is empty. While observability might be initialized, it's safer to handle cases where no plugins are registered or configured for tracing.
context = self._plugins[0].get_trace_context() if self._plugins else None
current_span = trace.get_current_span(
context=context
).get_span_context()
Implementing gRFC A72 grpc/proposal/pull/474