-
Notifications
You must be signed in to change notification settings - Fork 590
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
ENH: add application name option to BigQuery connect #2303
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.
Thanks @tswast, couple of small comments. As said, I find the parameter name user_agent misleading. It's not the user agent header, and not even the value of the user agent parameter of the bigquery client.
Also,even if the function to compose the user agent is rather simple, probably worth having a unit test.
The CI is currently broken, will ping you once it's fixed, to merge master here.
docs/source/release/index.rst
Outdated
| @@ -12,6 +12,7 @@ Release Notes | |||
| These release notes are for versions of ibis **1.0 and later**. Release | |||
| notes for pre-1.0 versions of ibis can be found at :doc:`release-pre-1.0` | |||
|
|
|||
| * :feature:`TODO` Add ``user_agent`` argument to ``ibis.bigquery.connect`` to allow attributing Google API requests to projects that use Ibis. | |||
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.
Don't forget to replace the TODO here, now that we've got a PR number.
ibis/bigquery/client.py
Outdated
| if user_agent: | ||
| user_agent_components.insert(0, user_agent) | ||
|
|
||
| return ClientInfo(user_agent=" ".join(user_agent_components)) |
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.
I think user_agent is a misleading name for the parameters, since it's the user agent header, but just an id added to it. I'd call it app_name or something else.
The, I personally find this implementation a bit overcomplicated. Something like this should do the same and be clearer (feel free to disagree, may be it's just a personal thing):
user_agent_ = []
if app_info: user_agent.append(app_info)
user_agent.append(f' ibis{ibis.__version__}')
return ClientInfo(user_agent=' '.join(user_agent))
setup.py
Outdated
| @@ -29,7 +29,7 @@ | |||
| 'clickhouse-driver>=0.1.3', | |||
| 'clickhouse-cityhash', | |||
| ] | |||
| bigquery_requires = ['google-cloud-bigquery>=1.0.0', 'pydata-google-auth'] | |||
| bigquery_requires = ['google-cloud-bigquery>=1.12.0', 'pydata-google-auth'] | |||
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.
This needs to be changed in ci/*.yml. I guess you need the version bump for these changes to work, but would be better if you change the version in a separate PR.
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.
Sent #2304. I can rebase after that's merged.
This is needed in order to set the application name in the user agent header. This feature to be added in: ibis-project#2303 Also, removes pin on google-api-core, now that googleapis/python-api-core#47 is closed.
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.
I decided to rename to application_name. Some Google libraries use that name for this feature.
setup.py
Outdated
| @@ -29,7 +29,7 @@ | |||
| 'clickhouse-driver>=0.1.3', | |||
| 'clickhouse-cityhash', | |||
| ] | |||
| bigquery_requires = ['google-cloud-bigquery>=1.0.0', 'pydata-google-auth'] | |||
| bigquery_requires = ['google-cloud-bigquery>=1.12.0', 'pydata-google-auth'] | |||
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.
Sent #2304. I can rebase after that's merged.
This is needed in order to set the application name in the user agent header. This feature to be added in: ibis-project#2303 Also, removes pin on google-api-core, now that googleapis/python-api-core#47 is closed.
This is needed in order to set the application name in the user agent header. This feature to be added in: ibis-project#2303 Also: * Remove pin on google-api-core, now that googleapis/python-api-core#47 is closed. * Update docs/source/release/index.rst Co-authored-by: Marc Garcia <garcia.marc@gmail.com>
This is needed in order to set the application name in the user agent header. This feature to be added in: #2303 Also: * Remove pin on google-api-core, now that googleapis/python-api-core#47 is closed. * Update docs/source/release/index.rst Co-authored-by: Marc Garcia <garcia.marc@gmail.com>
This allows Google API requests by Ibis or tools using Ibis to be attributed to the project.
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.
lgtm, thanks @tswast
|
thanks @tswast |
This is needed in order to set the application name in the user agent header. This feature to be added in: ibis-project#2303 Also: * Remove pin on google-api-core, now that googleapis/python-api-core#47 is closed. * Update docs/source/release/index.rst Co-authored-by: Marc Garcia <garcia.marc@gmail.com>
This allows Google API requests by Ibis or tools using Ibis to be
attributed to the project.