-
Notifications
You must be signed in to change notification settings - Fork 152
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: track invocation id for retry metrics #741
Conversation
Not sure the right way to test this at the moment. As an example, this test fails With the failure occurring because they have different invocation ids in the headers The same thing is causing failures in many different parts of the code Especially in cases like this where it's using assert_called_once_with, I'm not even sure how I might extract the invocation id so as to insert it in the expected headers |
Ok, old tests pass now. Still need to write new tests for the new feature add. |
) | ||
else: | ||
client.download_blob_to_file(blob, file_obj, **extra_kwargs) | ||
with patch.object( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Breathtender can you test that the invocation id changes for each call of download_blob_to_file()?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, check now; lines 1778-1795 in this file.
tests/unit/test_client.py
Outdated
@@ -1775,6 +1775,24 @@ def test_download_blob_to_file_wo_chunks_w_raw(self): | |||
def test_download_blob_to_file_w_chunks_w_raw(self): | |||
self._download_blob_to_file_helper(use_chunks=True, raw_download=True) | |||
|
|||
def test_download_blob_to_file_multiple(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test_download_blob_have_different_uuid
|
||
self.assertNotEqual( | ||
blob._do_download.call_args_list[0][0][3]["X-Goog-API-Client"], | ||
blob._do_download.call_args_list[1][0][3]["X-Goog-API-Client"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just want to confirm the UUID is changing right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's what this is testing.
(Pdb) blob._do_download.call_args_list[0][0][3]['X-Goog-API-Client']
'gcloud-python/2.2.1 gl-python/3.8.12 gax/2.7.1 gccl/2.2.1 gccl-invocation-id/90296518-9508-4a41-83f6-247eba7712a2'
(Pdb) blob._do_download.call_args_list[1][0][3]['X-Goog-API-Client']
'gcloud-python/2.2.1 gl-python/3.8.12 gax/2.7.1 gccl/2.2.1 gccl-invocation-id/b7fc4b3a-4cf4-48c1-9475-c95096336c83'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Breathtender re-raising this; do you know if there's a separate UUID per resumable upload request?
I didn't ask this in prior discussions and may have missed it.
No description provided.