Add clientUserAgent and snapshotSummary to the table audit event - #639
Open
cbb330 wants to merge 1 commit into
Open
Add clientUserAgent and snapshotSummary to the table audit event#639cbb330 wants to merge 1 commit into
cbb330 wants to merge 1 commit into
Conversation
cbb330
force-pushed
the
chbush/table-audit-client-version
branch
from
June 23, 2026 23:54
66b9ff1 to
ab32236
Compare
cbb330
changed the title
Two purely-additive fields on TableAuditEvent for client/commit observability (both observability only — neither gates a request): - clientUserAgent: the raw User-Agent request header, stored verbatim, so the client/runtime version can be derived at query time (e.g. openhouse-java-client/<version>). Set in the shared buildAndSendEvent, read from the bound servlet request via RequestContextHolder (mirroring ServiceAuditAspect); best-effort and never disrupts the audited operation. - snapshotSummary: the full Iceberg summary map of the snapshot the main ref points to (the same snapshot as currentSnapshotId), emitted verbatim (operation, spark.app.id/trino_query_id, engine/iceberg versions, and the core-computed file-delta counts). Emitted unaggregated so queries derive operation and write-mode signals downstream without a lossy server-side classification. Snapshot.summary() omits the operation key (Iceberg parses it out separately), so it is merged back to mirror the on-disk summary.
cbb330
force-pushed
the
chbush/table-audit-client-version
branch
from
June 24, 2026 00:24
ab32236 to
352e9f7
Compare
cbb330
changed the title
| } catch (Exception e) { | ||
| log.warn("Failed to read User-Agent header for audit event", e); | ||
| } | ||
| return null; |
Collaborator
There was a problem hiding this comment.
nit. Optional vs null.
| */ | ||
| private static Map<String, String> buildSnapshotSummary(Snapshot snapshot) { | ||
| Map<String, String> summary = new HashMap<>(); | ||
| if (snapshot.summary() != null) { |
Collaborator
There was a problem hiding this comment.
under what conditions is the summary / operation expected to be null? is a partial version of this considered valid?
| private static Map<String, String> buildSnapshotSummary(Snapshot snapshot) { | ||
| Map<String, String> summary = new HashMap<>(); | ||
| if (snapshot.summary() != null) { | ||
| summary.putAll(snapshot.summary()); |
Collaborator
There was a problem hiding this comment.
Seems fine. The contents do not contain column statistics that would subject this to privacy risks.
| summary.putAll(snapshot.summary()); | ||
| } | ||
| if (snapshot.operation() != null) { | ||
| summary.put(SNAPSHOT_OPERATION_KEY, snapshot.operation()); |
Collaborator
There was a problem hiding this comment.
is empty operation / summary tested? I want to understand the expectation when one or both is actually null given the code.
| Snapshot snapshot = SnapshotParser.fromJson(snapshotJson); | ||
| if (snapshot.snapshotId() == mainSnapshotId) { | ||
| eventBuilder.currentSnapshotTimestampMs(snapshot.timestampMillis()); | ||
| eventBuilder.snapshotSummary(buildSnapshotSummary(snapshot)); |
Collaborator
There was a problem hiding this comment.
I assume this is fast, <1ms because its a pure function and we are already emitting the data.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds two purely-additive fields to
TableAuditEventfor client/commit observability, populated server-side inTableAuditAspect(both are observability only — neither gates a request):clientUserAgent— the rawUser-Agentrequest header, stored verbatim, so the client/runtime version can be derived at query time (e.g.openhouse-java-client/<version>). Set in the sharedbuildAndSendEvent, read from the bound servlet request viaRequestContextHolder(mirroringServiceAuditAspect); best-effort and never disrupts the audited operation.snapshotSummary— the full Iceberg summary map of the snapshot themainref points to (the same snapshot ascurrentSnapshotId), emitted verbatim (operation,spark.app.id/trino_query_id, engine/iceberg versions, and the core-computed file-delta counts). Emitted unaggregated so queries derive operation and write-mode signals downstream without a lossy server-side classification — there is no per-commit field that cleanly declares copy-on-write vs merge-on-read across engines.Snapshot.summary()omits theoperationkey (Iceberg parses it out into a separate field), so it is merged back to mirror the on-disk summary. It is hooked onto the snapshotextractSnapshotInfoalready resolves forcurrentSnapshotTimestampMs; null when the request carries nomainref (branch-only commits), consistent withcurrentSnapshotId.Changes
TableAuditEventgainsclientUserAgent(String) andsnapshotSummary(Map<String,String>).snapshotSummaryis emitted unbounded by design (unlike the allowlist-filteredauditedTableProperties) — a snapshot summary is naturally a small, fixed set of keys;clientUserAgentis captured on every audited event, not just commits.Testing Done
IcebergSnapshotsApiHandlerAuditTest:testPutIcebergSnapshotsCapturesClientUserAgent— sets aUser-Agentheader and asserts verbatim capture.testPutIcebergSnapshotsEmitsRewriteSummaryVerbatim/testPutIcebergSnapshotsEmitsDeleteFileSummaryVerbatim— assert the raw summary counters are emitted for a copy-on-write overwrite and a merge-on-read delete.testPutIcebergSnapshotsContainsSnapshotInfoand the failed-path test to assert the summary is captured (including on failure), and the branch-only test to assert it is null when there is nomainref.snapshotSummaryis excluded from theReflectionEqualsexpected constants and asserted explicitly. Full audit test suite passes (IcebergSnapshotsApiHandlerAuditTest16, plusTablesApiHandlerAuditTest/DatabasesApiHandlerAuditTest).Additional Information
Part of a sliced rollout of server-side commit-audit observability. A follow-up will add
failureType(the exception class on failed commits) plus isolation of the audit emission so it can never mask the operation being audited.