RFR: 8351347: HttpClient Improve logging of response headers [v2]
Daniel Fuchs
dfuchs at openjdk.org
Wed May 14 10:57:55 UTC 2025
On Wed, 14 May 2025 09:31:07 GMT, Volkan Yazici <vyazici at 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
More information about the net-dev
mailing list