-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Add client_info to BigQuery constructor for user-amenable user agent headers
#7806
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
Conversation
shollyman
left a comment
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.
Seems reasonable for the BQ changes. There will be a followup change for exposing client_info to the other BQ APIs like dts and storage?
Already present on those, which was a major factor in going with this design.
Once #7799 goes in, we'll be able to set At some point, I do intend to update the BQ magics to set this user agent field, as they create their own clients, and I figure we can track that as using BQ from Jupyter. |
bigquery/tests/unit/test_client.py
Outdated
| _http=http, | ||
| ) | ||
|
|
||
| table = client.get_table(self.TABLE_REF) |
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.
Lint failure here: the table variable is never used. You can just invoke client.get_table without binding the return value, or use _.
…headers
This aligns BigQuery's behavior regarding the User-Agent and
X-Goog-Api-Client headers with that of the GAPIC-based clients.
Old:
X-Goog-API-Client: gl-python/3.7.2 gccl/1.11.2
User-Agent: gcloud-python/0.29.1
New:
X-Goog-API-Client: optional-application-id/1.2.3 gl-python/3.7.2 grpc/1.20.0 gax/1.9.0 gapic/1.11.2 gccl/1.11.2
User-Agent: optional-application-id/1.2.3 gl-python/3.7.2 grpc/1.20.0 gax/1.9.0 gapic/1.11.2 gccl/1.11.2
In order to set the `optional-application-id/1.2.3`, the latest version
of `api_core` is required, but since that's an uncommon usecase and it
doesn't break, just ignore the custom User-Agent if an older version is
used, I didn't update the minimum version `setup.py`.
d0110ea to
ec4a491
Compare
| :param client: The client that owns the current connection. | ||
| """ | ||
|
|
||
| def __init__(self, client, client_info=None): |
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.
@tswast Need to document the client_info parameter in the class docstring.
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.
We should probably hoist this machinery into the google.cloud._http.JSONConnection base class.
|
|
||
| @USER_AGENT.setter | ||
| def USER_AGENT(self, value): | ||
| self._client_info.user_agent = value |
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.
These aren't constants, so why uppercase?
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.
For backwards compatibility.
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.
There's a USER_AGENT on the superclass, which we want to override the behavior of.
This aligns BigQuery's behavior regarding the User-Agent and
X-Goog-Api-Client headers with that of the GAPIC-based clients.
Old:
New:
In order to set the
optional-application-id/1.2.3, the latest versionof
api_coreis required, but since that's an uncommon usecase and itdoesn't break, just ignore the custom User-Agent if an older version is
used, I didn't update the minimum version
setup.py.This PR depends on:
user_agentproperty toClientInfo#7799 for unit test purposes.