Skip to content

feat: Add protobuf runtime version to x-goog-api-client header #2368

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

Merged
merged 2 commits into from
May 6, 2025

Conversation

parthea
Copy link
Contributor

@parthea parthea commented Mar 19, 2025

See https://cloud.google.com/apis/docs/system-parameters#definitions
Googlers see b/401039328

This change is built on top of #2386 which fixes a presubmit which is failing at main.

Before:

urlRequestHeaders:
    X-Goog-Api-Client: "gl-python/3.9.16 rest/[email protected] gax/2.24.2 gapic/0.0.0"

After (with googleapis/python-api-core#812):

urlRequestHeaders:
    X-Goog-Api-Client: "gl-python/3.9.16 rest/[email protected] gax/2.24.2 gapic/0.0.0 pb/6.30.2"

You can test the change by forcing installation of PR googleapis/python-api-core#812 using the change below

py3133partheniou@partheniou-vm-3:~/git/gapic-generator-python$ git diff noxfile.py
diff --git a/noxfile.py b/noxfile.py
index 117ca47c..899fdd4a 100644
--- a/noxfile.py
+++ b/noxfile.py
@@ -373,6 +373,10 @@ def showcase_library(
                     "--upgrade",
                     "google-auth[aiohttp]!=2.40.0",
                 )
+                session.install(
+                    "--ignore-installed",
+                    "google-api-core @ git+https://github.com/googleapis/python-api-core.git@add-protobuf-runtime-header",
+                )
         else:
             # The ads templates do not have constraints files.

and then run nox -s showcase_w_rest_async-3.9

@product-auto-label product-auto-label bot added the size: s Pull request size is small. label Mar 19, 2025
@parthea parthea force-pushed the add-protobuf-runtime branch from 8901a49 to fe73db3 Compare March 19, 2025 21:22
@product-auto-label product-auto-label bot added size: m Pull request size is medium. and removed size: s Pull request size is small. labels Mar 19, 2025
@parthea parthea force-pushed the add-protobuf-runtime branch from fe73db3 to b59e0fa Compare March 19, 2025 21:23
@parthea parthea closed this Mar 20, 2025
@parthea parthea changed the title for testing purposes feat: Add protobuf runtime version to x-goog-api-client header May 6, 2025
@parthea parthea reopened this May 6, 2025
@parthea parthea marked this pull request as ready for review May 6, 2025 15:05
@parthea parthea requested a review from a team as a code owner May 6, 2025 15:05
@parthea parthea force-pushed the add-protobuf-runtime branch from 5f62265 to 88efc0e Compare May 6, 2025 18:49
Copy link
Contributor

@vchudnov-g vchudnov-g left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that the implementation in https://github.com/googleapis/python-api-core/blob/118bd96f3907234351972409834ab5309cdfcee4/google/api_core/client_info.py#L114 is problematic. Not only does it depend on us constructing the format string in the same order as the __dict__ entries, but it also requires these guard statements at the calling site. We can/should refactor that code to make this unnecessary going forward, except we'll still need to account for people using api_core from before the refactor.....we'll be stuck with these guard clauses ~indefinitely now.

@parthea
Copy link
Contributor Author

parthea commented May 6, 2025

Correct, the format string has a fixed order but it's not clear how problematic this is.

but it also requires these guard statements at the calling site

The guard statements are often necessary for backwards compatibility reasons for any new feature that we add to google-api-core

@parthea parthea merged commit 9c05dbe into main May 6, 2025
121 checks passed
@parthea parthea deleted the add-protobuf-runtime branch May 6, 2025 22:59
@vchudnov-g
Copy link
Contributor

Yeah, ignore my comment about the order. I got confused.

But this is something that shouldn't need a guard clause, if api-core had been implemented differently. IMO, we should be able to set arbitrary key-value pairs in ClientInfo, without the need for the guard clause here. When emitting the header, ClientInfo would only look for the keys it knows about to create the format string. So older versions would not use the protobuf info, but as soon as people were using the newer version of api-core, that would become part of the header. This would make things more extensible, though admittedly it would be less type-safe, since a typo would cause the header token to be omitted.

Oh, well. Hindsight.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: m Pull request size is medium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants