On Wed, 14 May 2025 09:31:07 GMT, Volkan Yazici <vyaz...@openjdk.org> wrote:
>> Includes request method, request URI, response status code, and HTTP/2 >> stream ID while logging response headers in the HTTP Client. >> >> ### Demonstration >> >> Snippets from running JTreg against >> `test/jdk/java/net/httpclient/HeadTest.java`: >> >> **Before:** >> >> INFO: HEADERS: REQUEST HEADERS: >> HEAD /transfer/ HTTP/1.1 >> Content-Length: 0 >> Host: 127.0.0.1:43647 >> ... >> INFO: HEADERS: RESPONSE HEADERS: >> connection: Upgrade >> upgrade: h2c >> ... >> INFO: HEADERS: RESPONSE HEADERS: >> :status: 304 >> content-length: 300 >> ... >> INFO: HEADERS: HEADERS FRAME (streamid=1): >> :authority: 127.0.0.1:50611 >> :method: GET >> :path: / >> >> >> **After:** >> >> INFO: HEADERS: REQUEST HEADERS: >> HEAD /transfer/ >> Content-Length: 0 >> Host: 127.0.0.1:43647 >> ... >> INFO: HEADERS: RESPONSE HEADERS: >> GET http://127.0.0.1:48497/ 101 >> connection: Upgrade >> upgrade: h2c >> ... >> INFO: HEADERS: RESPONSE HEADERS (streamid=1): >> GET http://127.0.0.1:48497/ 304 >> :status: 304 >> content-length: 300 >> ... >> INFO: HEADERS: HEADERS FRAME (streamid=1): >> GET https://127.0.0.1:50611/ >> :authority: 127.0.0.1:50611 >> :method: GET >> :path: / > > Volkan Yazici has updated the pull request incrementally with three > additional commits since the last revision: > > - Match header logging in `Http1Request` with others > - Shorten code > - Improve header logging in more places Some suggested changes. I'd rather not localize streamId and status code - so I'd prefer we use %s. src/java.net.http/share/classes/jdk/internal/net/http/Http1Request.java line 93: > 91: int protocolFieldPrefixIndex = line.lastIndexOf(' '); > 92: assert protocolFieldPrefixIndex > 0; > 93: sb.append(" ").append(line, 0, > protocolFieldPrefixIndex).append('\n'); I'd rather print the whole line here - including HTTP/1.1. Nice to have it indented differently though. src/java.net.http/share/classes/jdk/internal/net/http/Http1Response.java line 227: > 225: if (Log.headers()) { > 226: StringBuilder sb = new StringBuilder("RESPONSE > HEADERS:\n"); > 227: sb.append(" %s %s %d\n".formatted(request.method(), > request.uri(), responseCode)); Suggestion: sb.append(" %s %s %s\n".formatted(request.method(), request.uri(), responseCode)); src/java.net.http/share/classes/jdk/internal/net/http/Http2Connection.java line 1688: > 1686: oh.streamid(stream.streamid); > 1687: if (Log.headers()) { > 1688: StringBuilder sb = new StringBuilder("HEADERS FRAME > (streamid=%d):\n".formatted(stream.streamid)); Suggestion: StringBuilder sb = new StringBuilder("HEADERS FRAME (streamid=%s):\n".formatted(stream.streamid)); src/java.net.http/share/classes/jdk/internal/net/http/Stream.java line 666: > 664: if (Log.headers()) { > 665: StringBuilder sb = new StringBuilder("RESPONSE HEADERS > (streamid=%d):\n".formatted(streamid)); > 666: sb.append(" %s %s %d\n".formatted(request.method(), > request.uri(), responseCode)); Suggestion: sb.append(" %s %s %s\n".formatted(request.method(), request.uri(), responseCode)); src/java.net.http/share/classes/jdk/internal/net/http/Stream.java line 677: > 675: } else { > 676: if (Log.headers()) { > 677: StringBuilder sb = new StringBuilder("TRAILING HEADERS > (streamid=%d):\n".formatted(streamid)); Suggestion: StringBuilder sb = new StringBuilder("TRAILING HEADERS (streamid=%s):\n".formatted(streamid)); src/java.net.http/share/classes/jdk/internal/net/http/Stream.java line 678: > 676: if (Log.headers()) { > 677: StringBuilder sb = new StringBuilder("TRAILING HEADERS > (streamid=%d):\n".formatted(streamid)); > 678: sb.append(" %s %s %d\n".formatted(request.method(), > request.uri(), responseCode)); These are trailing headers - printing URI and response code would be confusing here. Only printing streamId should be fine. Suggestion: src/java.net.http/share/classes/jdk/internal/net/http/Stream.java line 1775: > 1773: > 1774: if (Log.headers()) { > 1775: StringBuilder sb = new StringBuilder("RESPONSE > HEADERS (streamid=%d):\n".formatted(streamid)); Suggestion: StringBuilder sb = new StringBuilder("RESPONSE HEADERS (streamid=%s):\n".formatted(streamid)); src/java.net.http/share/classes/jdk/internal/net/http/Stream.java line 1776: > 1774: if (Log.headers()) { > 1775: StringBuilder sb = new StringBuilder("RESPONSE > HEADERS (streamid=%d):\n".formatted(streamid)); > 1776: sb.append(" %s %s > %d\n".formatted(request.method(), request.uri(), responseCode)); Suggestion: sb.append(" %s %s %s\n".formatted(request.method(), request.uri(), responseCode)); src/java.net.http/share/classes/jdk/internal/net/http/Stream.java line 1788: > 1786: if (Log.headers()) { > 1787: StringBuilder sb = new StringBuilder("TRAILING > HEADERS (streamid=%d):\n".formatted(streamid)); > 1788: sb.append(" %s %s > %d\n".formatted(request.method(), request.uri(), responseCode)); Suggestion: StringBuilder sb = new StringBuilder("TRAILING HEADERS (streamid=%s):\n".formatted(streamid)); sb.append(" %s %s %s\n".formatted(request.method(), request.uri(), responseCode)); ------------- PR Review: https://git.openjdk.org/jdk/pull/25201#pullrequestreview-2839695414 PR Review Comment: https://git.openjdk.org/jdk/pull/25201#discussion_r2088643521 PR Review Comment: https://git.openjdk.org/jdk/pull/25201#discussion_r2088654005 PR Review Comment: https://git.openjdk.org/jdk/pull/25201#discussion_r2088653338 PR Review Comment: https://git.openjdk.org/jdk/pull/25201#discussion_r2088652653 PR Review Comment: https://git.openjdk.org/jdk/pull/25201#discussion_r2088651795 PR Review Comment: https://git.openjdk.org/jdk/pull/25201#discussion_r2088648853 PR Review Comment: https://git.openjdk.org/jdk/pull/25201#discussion_r2088650531 PR Review Comment: https://git.openjdk.org/jdk/pull/25201#discussion_r2088657325 PR Review Comment: https://git.openjdk.org/jdk/pull/25201#discussion_r2088658535
