Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add unique number to WebFlux log prefix for Reactor Netty server #22039

Closed
spring-projects-issues opened this issue Nov 16, 2018 · 10 comments
Closed
Assignees
Milestone

Comments

@spring-projects-issues
Copy link
Collaborator

@spring-projects-issues spring-projects-issues commented Nov 16, 2018

Robbert van Waveren opened SPR-17507 and commented

Expected behavior:
Every HTTP request is logged with a unique log prefix regardless of any tcp/ip connection or thread reuse.

Observed behavior:
Consecutive HTTP requests from Chrome (using keep-alive) that are handled by the same reactor-http-nio thread are logged with the same log prefix.

 


Affects: 5.1.2

Attachments:

Issue Links:

  • #22038 HTTP GET from Chrome with WebFlux Rest Controller never completes
@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Nov 19, 2018

Rossen Stoyanchev commented

We use the channel id, intentionally matching to what Reactor Netty does. I see your point about the fact that connections are pooled and re-used. At the same time requests on the same connection do not overlap, so it doesn't prevent correctly correlating messages. It also provides a bit of extra information by showing that the same connection is being used, or not (when a connection is closed and re-opened).

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Nov 20, 2018

Robbert van Waveren commented

I've noticed that too and after reading the Javadocs it is somewhat clear to me that this can happen. Exposing it as ServerHttpRequest#getId though kind of makes it less obvious as id suggests uniqueness. 

 

Perhaps this ticket could be changed to an improvement request to for example make a separate getId and getChannelId available in the ServerWebExchange and ServerHttpRequest interfaces, if that is something you would want to support. As I feel that having truely unique http request id's would have a lot of value, especially assuming more and more requests will get multiplexed over the same channel in the future with http2 and 3. 

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Nov 21, 2018

Rossen Stoyanchev commented

For other servers we use an identity hex on the request instance, because a channel or connection id is not available. So having a separate getChannelId doesn't really make sense, as we couldn't implement it consistently across servers.

The main purpose of the getId() method is for logging purposes. We could consider appending "-N" to the id where N is an incremental number to make it unique, but I'm not convinced that's really necessary. Overall I'd really like to remain aligned with what Reactor Netty uses. Good point though about HTTP/2 where we might need to append a stream id as well. I will discuss this with the Reactor Netty team.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Nov 21, 2018

Rossen Stoyanchev commented

Scheduling tentatively for 5.2 as we should consider an improvement for HTTP/2 streams.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Dec 6, 2018

Rossen Stoyanchev commented

I've created  Reactor Netty #549 which we'll need in order to access the stream id.

@GwiYeong

This comment has been hidden.

@snicoll

This comment has been hidden.

@rstoyanchev
Copy link
Contributor

@rstoyanchev rstoyanchev commented Oct 23, 2019

HTTP/2 streams should be addressed as part of reactor/reactor-netty#549 and it looks like that will be transparent to us. In general where the underlying server exposes a connection id, we will use that, so if you need something further I believe that should be done at the Reactor Netty level. Otherwise the logs between Reactor Netty and WebFlux won't be entirely consistent.

@PyvesB
Copy link
Contributor

@PyvesB PyvesB commented Jan 17, 2020

We've enabled debug logging for incoming web server requests and have bumped into this issue as well. The Javadocs of ServerWebExchange suggest that the log prefix can be used to correlate messages for a given exchange:

* Return a log message prefix to use to correlate messages for this exchange.

However, as the underlying channel id is being used (see here), the same log prefix may be duplicated across different requests (for example if the connection is kept alive). Logs for different requests and responses will end up being mixed up together due to this.

@rstoyanchev suggestion about appending "-N" to the id where N is an incremental number to make it unique would be ideal, as it would prevent exchanges being confused with one another whilst still allowing to correlate with underlying reactor-netty log messages.

Could this issue please be reopened?

@rstoyanchev rstoyanchev added this to the 5.2.4 milestone Jan 24, 2020
@rstoyanchev rstoyanchev changed the title WebFlux log prefix improvements for Reactor Netty [SPR-17507] Add unique number to WebFlux log prefix for Reactor Netty server Jan 24, 2020
@PyvesB
Copy link
Contributor

@PyvesB PyvesB commented Jan 24, 2020

Thanks for working on this @rstoyanchev! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.