Skip to content
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

Merged
merged 11 commits into from Apr 12, 2022

Conversation

unforced
Copy link
Contributor

@unforced unforced commented Mar 25, 2022

No description provided.

@unforced unforced requested review from a team as code owners March 25, 2022 21:53
@product-auto-label product-auto-label bot added the api: storage Issues related to the googleapis/python-storage API. label Mar 25, 2022
google/cloud/storage/_http.py Outdated Show resolved Hide resolved
@unforced
Copy link
Contributor Author

Not sure the right way to test this at the moment.

As an example, this test fails
https://github.com/googleapis/python-storage/blob/main/tests/unit/test_client.py#L1725

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

@unforced
Copy link
Contributor Author

Ok, old tests pass now. Still need to write new tests for the new feature add.

@product-auto-label product-auto-label bot added the size: l Pull request size is large. label Apr 1, 2022
)
else:
client.download_blob_to_file(blob, file_obj, **extra_kwargs)
with patch.object(
Copy link
Member

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()?

Copy link
Contributor Author

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.

@@ -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):
Copy link
Member

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"],
Copy link
Member

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?

Copy link
Contributor Author

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'

Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: storage Issues related to the googleapis/python-storage API. size: l Pull request size is large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants