- Notifications
You must be signed in to change notification settings - Fork1.6k
Addclient_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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
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?
tswast commentedApr 25, 2019
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: thetable variable is never used. You can just invokeclient.get_table without binding the return value, or use_.
…headersThis aligns BigQuery's behavior regarding the User-Agent andX-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.1New: 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.2In order to set the `optional-application-id/1.2.3`, the latest versionof `api_core` is required, but since that's an uncommon usecase and itdoesn't break, just ignore the custom User-Agent if an older version isused, I didn't update the minimum version `setup.py`.
d0110ea toec4a491Compare| :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 theclient_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 thegoogle.cloud._http.JSONConnection base class.
| @USER_AGENT.setter | ||
| defUSER_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 aUSER_AGENT on the superclass, which we want to override the behavior of.
Uh oh!
There was an error while loading.Please reload this page.
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.