-
Couldn't load subscription status.
- Fork 380
Feat/add client version headers #2652
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
base: main
Are you sure you want to change the base?
Conversation
af393dc to
0d2e197
Compare
| session.headers.update(header_properties) | ||
| session.headers["Content-type"] = "application/json" | ||
| session.headers["User-Agent"] = f"PyIceberg/{__version__}" | ||
| session.headers["X-Client-Version"] = __version__ |
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 @jaimeferj for working on this. Wouldn't this make the User-Agent header the line above redundant?
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.
java's HTTP client uses X-Client-Version might be good to align, even though its redundant :)
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.
Good one, thanks!
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 for the PR!
Looks like there are a lot of changes here just to plumb through a commit hash. Honestly, I dont know how useful that is.
Pyiceberg builds are immutable. Once we build, 0.10.0, it will always use the same commit.
I think sending a pyiceberg.__version__ is enough to identity the client.
WDYT?
| session.headers.update(header_properties) | ||
| session.headers["Content-type"] = "application/json" | ||
| session.headers["User-Agent"] = f"PyIceberg/{__version__}" | ||
| session.headers["X-Client-Version"] = __version__ |
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.
java's HTTP client uses X-Client-Version might be good to align, even though its redundant :)
Closes #2373
Rationale for this change
This PR adds client version and git commit tracking headers to match the Java Iceberg client implementation. The Java client sends
X-Client-VersionandX-Client-Git-Commit-Shortheaders with all REST catalog requests for better client tracking and debugging.This implementation:
setuptools_scmto capture git version information at build timeThis provides better visibility into which PyIceberg versions and builds are making requests to REST catalogs, helping with
debugging and client tracking.
Are these changes tested?
Yes
test_client_version_headersto verify both headers are set correctlyX-Client-Versionmatches the package versionX-Client-Git-Commit-Shortis a valid 7-character hex git hashAre there any user-facing changes?
Yes, but minimal:
X-Client-VersionandX-Client-Git-Commit-Short