iceberg: couple logging improvements #27715
Merged
Conversation
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
It's very verbose as is.
There was a problem hiding this comment.
Pull Request Overview
This PR improves logging in the Iceberg REST client for better debugging and operational visibility. The changes add trace-level logging for failed REST requests and throttle frequent warning messages about default partition specs with AWS Glue.
- Added trace-level logging with request payloads when REST client operations fail
- Implemented rate limiting for AWS Glue default partition spec warnings to reduce log noise
- Enhanced error diagnostics by logging request details on failure
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/v/iceberg/rest_client/catalog_client.cc | Added trace logging for failed requests with payloads and improved error handling structure |
| src/v/iceberg/rest_client/BUILD | Added dependency on iobuf_parser for payload logging functionality |
| src/v/datalake/coordinator/coordinator.cc | Implemented rate limiting for AWS Glue partition spec warning messages |
Comment on lines
56
to
66
| template<typename T> | ||
| void maybe_log_payload_as_json( | ||
| ss::logger& l, ss::log_level lvl, std::string_view msg, const T& payload) { | ||
| if (!l.is_enabled(lvl)) { | ||
| return; | ||
| } | ||
| auto buf = serialize_payload_as_json(payload); | ||
| iobuf_parser p(std::move(buf)); | ||
| vlogl(iceberg::log, lvl, "{}: {}", msg, p.read_string_safe(4_KiB)); | ||
| } |
There was a problem hiding this comment.
This function should have a docstring explaining its purpose, parameters, and the 4_KiB limit rationale. It's unclear why 4_KiB was chosen as the safe read limit for payload logging.
Copilot uses AI. Check for mistakes.
| } | ||
| auto buf = serialize_payload_as_json(payload); | ||
| iobuf_parser p(std::move(buf)); | ||
| vlogl(iceberg::log, lvl, "{}: {}", msg, p.read_string_safe(4_KiB)); |
There was a problem hiding this comment.
The 4_KiB limit should be defined as a named constant instead of a magic number. Consider defining it as constexpr auto max_payload_log_size = 4_KiB; at the top of the file.
Copilot uses AI. Check for mistakes.
andrwng
requested review from
wdberkeley,
dotnwat and
nvartolomei
and removed request for
wdberkeley
wdberkeley
previously approved these changes
Sep 24, 2025
dotnwat
previously approved these changes
Sep 24, 2025
andrwng
force-pushed
the
iceberg-log-tweak
branch
from
232dda3 to
138231a
Compare
|
Force pushed a non-functional bug (s/iceberg::logger/l) |
dotnwat
previously approved these changes
Sep 24, 2025
wdberkeley
previously approved these changes
Sep 24, 2025
Retry command for Build#72892please wait until all jobs are finished before running the slash command |
When there is an error returned from the REST catalog, we have very little context about what we sent. This commit addresses this by logging the logical contents of the request as JSON. I considered logging at a lower level, and logging the entire HTTP request payload (e.g. in perform_request() after signing and such), but opted to place the at the callsite because the context we have there is already const and available to be logged conditionally (vs in perform_request(), the request is moved, making conditional logging trickier).
andrwng
force-pushed
the
iceberg-log-tweak
branch
from
138231a to
e315237
Compare
dotnwat
approved these changes
Sep 25, 2025
CI test resultstest results on build#72910
|
|
/backport v25.2.x |
|
/backport v25.1.x |
|
Failed to create a backport PR to v25.1.x branch. I tried: |
|
Failed to create a backport PR to v25.2.x branch. I tried: |
This was referenced Sep 25, 2025
This was referenced Sep 25, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Backports Required
Release Notes
Improvements