impl(gax-internal): integrate RequestRecorder with gRPC transport for Bidi Stream#5373
impl(gax-internal): integrate RequestRecorder with gRPC transport for Bidi Stream#5373haphungw wants to merge 1 commit intogoogleapis:mainfrom
RequestRecorder with gRPC transport for Bidi Stream#5373Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5373 +/- ##
==========================================
- Coverage 97.58% 97.58% -0.01%
==========================================
Files 222 222
Lines 46609 46623 +14
==========================================
+ Hits 45485 45498 +13
- Misses 1124 1125 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
haphungw
force-pushed
the
o11y-bidi-request-recorder
branch
from
881e278 to
239c1de
Compare
suzmue
left a comment
suzmue
left a comment
There was a problem hiding this comment.
Did a pass but leaving approval to someone more familiar with the observability portion of this CL
| let extensions = { | ||
| let mut e = tonic::Extensions::new(); | ||
| e.insert(tonic::GrpcMethod::new( | ||
| "google.test.v1.EchoServices", |
There was a problem hiding this comment.
No action required: EchoServices is used in all of our tests for the service name for GrpcMetho, but I think this maybe should actually be google.test.v1.EchoService (no s) which is the name we use in the paths, but it just doesn't have any affect on the test (I replaced all instances and reran the tests and it doesn't break anything).
| ) | ||
| } else { | ||
| (None, None, None, None) | ||
| }; |
There was a problem hiding this comment.
nit: this can alternatively be written as
| }; | |
| let (service, version, repo, artifact) = attrs.instrumentation | |
| .map(|info| ( | |
| Some(info.service_name), | |
| Some(info.client_version), | |
| Some("googleapis/google-cloud-rust"), | |
| Some(info.client_artifact), | |
| )) | |
| .unwrap_or_default(); |
| tracing::info_span!( | ||
| "grpc.request", | ||
| { OTEL_NAME } = rpc_method, | ||
| { RPC_SYSTEM_NAME } = attributes::RPC_SYSTEM_GRPC, | ||
| { OTEL_KIND } = attributes::OTEL_KIND_CLIENT, | ||
| { otel_trace::RPC_METHOD } = rpc_method, | ||
| { otel_trace::SERVER_ADDRESS } = attrs.server_address, | ||
| { otel_trace::SERVER_PORT } = attrs.server_port, | ||
| { otel_attr::URL_DOMAIN } = attrs.url_domain, | ||
| { RPC_RESPONSE_STATUS_CODE } = tracing::field::Empty, | ||
| { OTEL_STATUS_CODE } = otel_status_codes::UNSET, | ||
| { otel_trace::ERROR_TYPE } = tracing::field::Empty, | ||
| { GCP_CLIENT_SERVICE } = service, | ||
| { GCP_CLIENT_VERSION } = version, | ||
| { GCP_CLIENT_REPO } = repo, | ||
| { GCP_CLIENT_ARTIFACT } = artifact, | ||
| { GCP_GRPC_RESEND_COUNT } = None::<i64>, | ||
| { GCP_RESOURCE_DESTINATION_ID } = tracing::field::Empty, |
There was a problem hiding this comment.
possible code clean up for later: This seems like it could share code with #5375
|
I think we should close this PR until we know what is the design (or even the requirements) for streaming RPCs. |
3 participants
Add the integration of the
RequestRecorderto the bidirectional streaming implementation ingaxi. This ensures transport-level metrics and spans are correctly captured for bidi streams.