impl(o11y): add status handling for gRPC network spans #3808
Conversation
westarle
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3808 +/- ##
=======================================
Coverage ? 96.09%
=======================================
Files ? 146
Lines ? 5713
Branches ? 0
=======================================
Hits ? 5490
Misses ? 223
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
westarle
force-pushed
the
feat/t4-grpc-status-handling
branch
from
2bf51ac to
f81b970
Compare
- Implement `ResponseFuture` to wrap the inner future and record status. - Add `pin-project` and `futures-util` dependencies (moved to workspace). - Populate `rpc.grpc.status_code`, `otel.status_code`, and `error.type`. - Use `to_gax_error` to convert `tonic::Status` into `gax::error::Error` for consistent `error.type` strings. - Default to `OK` status if `grpc-status` header is missing but HTTP response is successful. - Refactor `ResponseFuture::poll` to use standalone helper functions for clarity. - Limitation: Only captures status codes in HTTP headers.
westarle
force-pushed
the
feat/t4-grpc-status-handling
branch
from
f81b970 to
6cf7f3b
Compare
coryan
left a comment
coryan
left a comment
There was a problem hiding this comment.
Looks pretty good, some questions and (maybe) improvements.
| let _guard = this.span.enter(); | ||
| let result = futures_util::ready!(this.inner.poll(cx)); |
There was a problem hiding this comment.
This works, but I had to read the code of futures_util::ready! to convince myself. Maybe with more experience I will not care, but it is not obvious that this "leaves" the span when the inner future is not ready.
Maybe a small comment?
| type Error = S::Error; | ||
| // We use `Box<dyn Future...>` (type erasure) here to simplify the type signature. | ||
| // Without this, we would need to explicitly name the complex type returned by | ||
| // `.instrument()` (and any implementation changes in `call`), which can be verbose and brittle. |
There was a problem hiding this comment.
Since we are not calling .instrument() can we simplify this code / reduce the overhead?
| } | ||
|
|
||
| fn record_response_status<ResBody>(span: &tracing::Span, response: &http::Response<ResBody>) { | ||
| match tonic::Status::from_header_map(response.headers()) { |
There was a problem hiding this comment.
Does this function get called twice then? One for this layer and once in the real tonic layer? Do you have a sense of how expensive it is for the "success" case? Am I connect in assuming it is fairly fast because it returns immediately (as the header does not exist)?
https://docs.rs/tonic/latest/src/tonic/status.rs.html#465
It may be fine if it is a bit expensive, just wan to document the tradeoff.
2 participants
ResponseFutureto wrap the inner future and record status.#3766