-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Add client_info
to BigQuery constructor for user-amenable user agent headers
#7806
Conversation
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
class Connection(_http.JSONConnection): | ||
"""A connection to Google BigQuery via the JSON REST API. | ||
|
||
:type client: :class:`~google.cloud.bigquery.client.Client` | ||
: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_core
is 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_agent
property toClientInfo
#7799 for unit test purposes.