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

Expose ChannelId in ReactorNettyWebsocketSession #26485

Closed
bruto1 opened this issue Feb 1, 2021 · 9 comments
Closed

Expose ChannelId in ReactorNettyWebsocketSession #26485

bruto1 opened this issue Feb 1, 2021 · 9 comments
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Milestone

Comments

@bruto1
Copy link

bruto1 commented Feb 1, 2021

Not a bug but proposal
This ID is much more unique than WebSocketSession's own, is already part of public APIs available to Netty handlers and could be used, for example, to correlate log events happening at different levels in a Webflux app without resorting to reflective access

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Feb 1, 2021
@rstoyanchev
Copy link
Contributor

A logPrefix is exposed through session.getHandshakeInfo().getLogPrefix(). On the server side this comes from ServerWebExchange#getLogPrefix() and is based on the channel id. On the client side this doesn't seem to be used but we can improve on that and pass a similar prefix based on the channel id.

@rstoyanchev rstoyanchev added status: waiting-for-feedback We need additional information before we can continue in: web Issues in web modules (web, webmvc, webflux, websocket) labels Feb 2, 2021
@bruto1
Copy link
Author

bruto1 commented Feb 2, 2021

logPrefix is based on ChannelId.shortText, which is nowhere nearly unique enough for logging purposes - longText is much more useful

It would've been convenient to be able to compute the same unique or effectively unique value pertaining to a connection from both ChannelHandlerContext and ReactorNettyWebSocketSession

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Feb 2, 2021
@rstoyanchev
Copy link
Contributor

The log prefix is unique, see #22039. It is also better to use a consistent log prefix.

If you really want to change this, you can access the channel id via ServerHttpRequest#getNativeRequest() from a WebFilter and insert it in the Reactor Context.

@rstoyanchev rstoyanchev added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Feb 2, 2021
@bruto1
Copy link
Author

bruto1 commented Feb 2, 2021

log prefix is, but it's not available from ChannelHandlerContext
the idea is to be able to generate logging events from both Netty handler level and Spring app level with some unique value which would allow to correlate them

ChannelId in general and its longValue in particular fits the bill, except there's no public API to retrieve it using WebSocketSession

As for your suggestion, request is accessible via ServerWebExchange in ReactorNettyRequestUpgradeStrategy.upgrade
I could get away with adding ChannelId to subscriber context just there without adding a new WebFilter
Would that be ok?

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Feb 2, 2021
@bruto1
Copy link
Author

bruto1 commented Feb 2, 2021

Thanks for the tip!

A bit more casting than I'd like:

    (exchange, chain) ->
            chain.filter(exchange)
                 .contextWrite(ctx -> ctx.put(ChannelId.class.getName(),
                     ((AbstractServerHttpRequest) exchange.getRequest())
                         .<ChannelOperations>getNativeRequest()
                         .channel()
                         .id()))

but I guess it is public API and probably more conventional than getting channel and id from WebSocketSession itself via reflection

@rstoyanchev
Copy link
Contributor

I thought getNativeRequest was exposed at the contract level. My bad, nevertheless getting on the level of the native request in this use case makes sense. We cannot make a channelId available for all servers since they don't all expose that.

@rstoyanchev rstoyanchev added status: declined A suggestion or change that we don't feel we should currently apply and removed status: feedback-provided Feedback has been provided status: waiting-for-triage An issue we've not yet triaged or decided on labels Feb 3, 2021
@bruto1
Copy link
Author

bruto1 commented Feb 4, 2021

not all, just in ReactorNettyWebsocketSession would be enough since it imples a particular server

@rstoyanchev
Copy link
Contributor

You would still have to downcast, but fine we can expose this for the Reactor Netty session.

@rstoyanchev rstoyanchev reopened this Feb 4, 2021
@rstoyanchev rstoyanchev self-assigned this Feb 4, 2021
@rstoyanchev rstoyanchev added type: enhancement A general enhancement and removed status: declined A suggestion or change that we don't feel we should currently apply labels Feb 4, 2021
@rstoyanchev rstoyanchev added this to the 5.3.4 milestone Feb 4, 2021
@bruto1
Copy link
Author

bruto1 commented Feb 13, 2021

Thank you, @rstoyanchev

This was referenced Mar 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

3 participants