Parse Iceberg client language and version from request headers - #4236
Parse Iceberg client language and version from request headers#4236adutra wants to merge 1 commit into
Conversation
| public interface IcebergClientInfo { | ||
|
|
||
| /** Programming language of an Iceberg REST client, as determined from request headers. */ | ||
| enum Language { |
There was a problem hiding this comment.
Do we need an enum here? I think a string would be more flexible when clients provides a lang not in this list.
There was a problem hiding this comment.
The language is inferred, not provided. So I think an enum is fine.
|
|
||
| /** Returns the Iceberg client library version, or empty if it cannot be determined. */ | ||
| @Value.Parameter(order = 1) | ||
| Optional<String> icebergVersion(); |
There was a problem hiding this comment.
Would client version a more descriptive name? icebergVersion may be confused with table versions.
Not a blocker, just brain storming: should we add a field called client name? For example, PyIceberg is the client name, 0.9.1 is the client version.
There was a problem hiding this comment.
Basically, we could have 3 field Client language, Client name, and Client version. WDYT?
There was a problem hiding this comment.
Not opposed to that, but wouldn't client name be a bit redundant with client language? I.e. PyIceberg vs PYTHON.
dimas-b
left a comment
dimas-b
left a comment
There was a problem hiding this comment.
Looks like a useful feature to me 👍
This change introduces a new `IcebergClientInfo` component to identify the Iceberg client library making each REST request. This information may be useful not only for logging and tracing, but also for future enhancements such as when deciding: - How to decode path segments (the decoding algorithm is likely to change) - How to process a remote signing request (the spec is likely to evolve) The information is sourced from two HTTP headers: - `X-Client-Version`: carries the client version string for Java and Python. - `User-Agent`: carries the version for Python, Go, Rust, and C++. The client info is propagated to: - OpenTelemetry spans via `TracingFilter` - SLF4J MDC via `LoggingMDCFilter`
c3e612d to
b6d1cc4
Compare
| } | ||
| } | ||
|
|
||
| // 2. Parse User-Agent for other clients, which (except for Python) send a stale, hardcoded |
There was a problem hiding this comment.
+1 on the logic here. I think User-Agent is more reliable, except for Java clients which don't currently include Iceberg version metadata there. This leads to two implications::
- Given
X-Client-Versionisn't part of IRC spec. It is a convention only working for Java clients, we can change the Iceberg Java clients to send back Iceberg version informations via headerUser-Agent, so that the iceberg clients across language behavior are more consistent. However, we still have to keep this logic in Polaris to be compatible with the old clients until they are irrelevant. - Alternative option: Make
X-Client-Versionpart of the IRC, so that other clients need to follow it.
| Language | X-Client-Version | User-Agent | Version Extraction Strategy | Implementation Status |
|---|---|---|---|---|
| Java | Apache Iceberg 1.10.1 (commit abc123...) |
Apache-HttpClient/5.4 (Java/11.0.25) |
Parse X-Client-Version | |
| Python | PyIceberg 0.9.1 |
PyIceberg/0.9.1 |
Parse User-Agent (X-Client-Version also works) | ✅ Consistent |
| Go | 0.14.1 |
GoIceberg/0.5.0 |
Parse User-Agent | |
| Rust | 0.14.1 |
iceberg-rs/0.3.0 |
Parse User-Agent | |
| C++ | (not sent) | iceberg-cpp/0.3.0-SNAPSHOT |
Parse User-Agent |
|
|
||
| } else if (userAgentHeader.startsWith("iceberg-rs/")) { | ||
| // Rust: "iceberg-rs/<version>" | ||
| int start = "iceberg-rs/".length(); |
There was a problem hiding this comment.
I don't think language inferring is reliable. The Iceberg rust clients may change it to Iceberg-rs or following the go's convention RustIceberg. I'd suggest to keep the client name as is. I'm also concerned the version inferring, which isn't necessary reliable, what if next rust/go version starts to adopt the Java client information format Apache Iceberg 1.10.1 (commit abc123...). Do we have strong use case for client versions? If not, I'd suggest to just keep as is in Polaris. Then we leave the parsing logic to the downstream.
There was a problem hiding this comment.
Do we have strong use case for client versions?
Not now, but maybe one day e.g. in order to spot Java clients using the wrong percent-encoding to encode URL path segments (see apache/iceberg#15989).
The Iceberg rust clients may change it to Iceberg-rs or following the go's convention RustIceberg
While they could do this, I don't think they would. Changing the format of a well-established User-Agent header is a delicate operation.
There was a problem hiding this comment.
And to complete my answer: a component like this is necessarily brittle, and can only operate on a best-effort basis. If clients change their headers, this component will need to be updated accordingly.
There was a problem hiding this comment.
I'm hesitant to rely on brittle inference for any feature. If Polaris ever need to depend on some information from client-side infor. to switch logic. I'd prefer explicit ways, like spec changes or headers.
|
This is really nice change @adutra thank you for adding this ! |
|
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
github-project-automation
Bot
moved this from PRs In Progress
to Done
in Basic Kanban Board
4 participants
This change introduces a new
IcebergClientInfocomponent to identify the Iceberg client library making each REST request.This information may be useful not only for logging and tracing, but also for future enhancements such as when deciding:
The information is sourced from two HTTP headers:
X-Client-Version: carries the client version string for Java and Python.User-Agent: carries the version for Python, Go, Rust, and C++.The client info is propagated to:
TracingFilterLoggingMDCFilterChecklist
CHANGELOG.md(if needed)site/content/in-dev/unreleased(if needed)