Skip to content
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

removed clientId from userAgent string #277

Merged
merged 3 commits into from
Feb 7, 2025

Conversation

shivam2680
Copy link
Contributor

@shivam2680 shivam2680 commented Feb 7, 2025

Description

Issue reported: While migrating usage logic metric, userAgent tag sometimes emitted tokens.
This PR removes clientID from userAgent string thus mitigating possibility of such scenarios.

Testing

Unit tests modified to verify correct userAgentString is built.

@shivam2680 shivam2680 force-pushed the shivam2680/user-agent-change branch from f48650f to 962f9b0 Compare February 7, 2025 19:29
if: ${{ matrix.node-version == '14' }}
uses: actions/setup-python@v4
with:
python-version: '3.10'
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is not related to this PR, can you put it in a separated one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changes done in PR

Signed-off-by: Shivam Raj <[email protected]>
@shivam2680 shivam2680 merged commit 771fb3b into main Feb 7, 2025
7 of 8 checks passed
@kravets-levko
Copy link
Contributor

kravets-levko commented Feb 13, 2025

@jackyhu-db @shivam2680 this fix is probably a mistake. clientId comes from user, and is intended to be used by users. It later appears in query history, so users may put some ID string there to know who submitted a query.

If by any chance you get some unwanted data there - it's not the library who did it, it's whatever that uses the library. So instead of removing this parameter, you had to check the code that uses the library.

Upd. Just to clarify: this parameter is a part of library's public interface. You cannot just remove it without incrementing major version number

@jackyhu-db
Copy link
Collaborator

@jackyhu-db @shivam2680 this fix is probably a mistake. clientId comes from user, and is intended to be used by users. It later appears in query history, so users may put some ID string there to know who submitted a query.

If by any chance you get some unwanted data there - it's not the library who did it, it's whatever that uses the library. So instead of removing this parameter, you had to check the code that uses the library.

Upd. Just to clarify: this parameter is a part of library's public interface. You cannot just remove it without incrementing major version number

Hi @kravets-levko the clientId should not be passed in the user agent as it may also include service principal ID, the user agent is mainly used to track the application/client (e.g node.js driver or some other app that uses the driver) instead of the individual client ID.

@kravets-levko
Copy link
Contributor

@jackyhu-db clientId is exclusively for the user-agent. Service principal ID should be passed via oauthClientId

@jackyhu-db
Copy link
Collaborator

jackyhu-db commented Feb 14, 2025

@jackyhu-db clientId is exclusively for the user-agent. Service principal ID should be passed via oauthClientId

@kravets-levko this clientId field may confuse the client. If it is only intended to be used for user-agent, we SHOULD name it explicitly as we did in other driver. See PySQL https://github.com/databricks/databricks-sql-python/blob/main/src/databricks/sql/client.py#L179

@kravets-levko
Copy link
Contributor

@jackyhu-db Yes, I agree. But totally removing it is not an option. Renaming sounds more reasonable

@jackyhu-db
Copy link
Collaborator

@jackyhu-db Yes, I agree. But totally removing it is not an option. Renaming sounds more reasonable

I agreed. @shivam2680 can you create another change if you agree as well?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants