impl(gax-internal): integrate RequestRecorder with gRPC transport#5368
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5368 +/- ##
==========================================
- Coverage 97.80% 97.59% -0.22%
==========================================
Files 222 222
Lines 46563 46609 +46
==========================================
- Hits 45540 45486 -54
- Misses 1023 1123 +100 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
coryan
left a comment
coryan
left a comment
There was a problem hiding this comment.
I don't think I understand this change.
| use ::tracing::instrument::Instrument; | ||
| let span = $crate::client_request_signals!(info: $info, method: $method); | ||
| let recorder = $crate::observability::RequestRecorder::new($info); | ||
| if let Some(current) = $crate::observability::RequestRecorder::current() { |
There was a problem hiding this comment.
I don't follow... this is where we create a request recorder and then set the recoder for a given future... Why would we notify any existing recorder (which is also unexpected) of anything?
There was a problem hiding this comment.
This is true, this block of code makes no sense. This was a helpless attempt to fix the bug in the storage integration test, but now as I double check this happened because we were not able to generate the resource name for many storage methods.
| mod storage { | ||
| use google_cloud_test_utils::errors::anydump; | ||
|
|
||
| #[ignore] |
There was a problem hiding this comment.
This requires BidiReadObject to pass. I'm not sure about the current implementation, so I wanna get this through before adding more code.
The test will be enabled again in the next PR when we add bidi streaming support.
There was a problem hiding this comment.
Ack, thanks. Consider:
#[ignore = "TODO(#....) - restore when bidi streaming works again"]
|
The main motivation behind this PR (and also why I am so rushed) is to fix a regression in our gRPC observability logic. The generated gRPC transport still expects to find If we don't want to refactor immediately, an alternative is to put I found all of these issues when I was verifying the storage integration tests: We have three problems to deal with right now:
What I'm trying to do in this PR is to see if this is the right way to give gRPC access to |
|
I think (but I am really not sure) we have two choices:
I am guessing (2) is cleaner, though maybe more risky. Are you trying to implement (1) or some alternative I did not consider? A third (less desirable option) is to release without this attribute feature. It only affects the Storage admin operations which are not used that often 🤷 |
coryan
left a comment
coryan
left a comment
There was a problem hiding this comment.
I don't think this hurts anything, so approved. I am not sure it solves anything either, but I may be wrong.
| mod storage { | ||
| use google_cloud_test_utils::errors::anydump; | ||
|
|
||
| #[ignore] |
There was a problem hiding this comment.
Ack, thanks. Consider:
#[ignore = "TODO(#....) - restore when bidi streaming works again"]| Ok(()) | ||
| } | ||
|
|
||
| #[ignore] |
There was a problem hiding this comment.
Presumably these need a TODO(#...) also.
|
I'm ok with skipping the resource id attribute for storage control until after freeze to avoid risk to the release and do things 'right' |
|
Since we are deferring the resource ID for storage control until post-freeze, what's the general sentiment on option 1 ( I'm already trying to make #1 work for Bidi streaming in a separate branch, but if that somehow misses the mark I hope reverting is not too bad 🤦♂️ |
3 participants
Add hook to record when a gRPC request starts and completes (successfully or with error).
This PR implements changes to Unary streaming only. Bidirectional streaming and Server streaming will be addressed in subsequent PRs.