Add description for StompBrokerRelayMessageHandler [SPR-16801]

[spring-projects-issues]

Filip Hrisafov opened SPR-16801 and commented

Currently the StompBrokerRelayMessageHandler uses the relayHost and relayPort in its toString method and also when connecting the "system" session (here).

In case no custom TcpOperations are provided then this is all good. However, if one needs to provide customized TcpOperations then the information that is logged is misleading.

Currently I misusing the relayHost to put my custom information (like the list of the round robin addresses). However, this is quite brittle, as it relays on the fact that I know how the internals of the StompBrokerRelayMessageHandler are done and the fact that those fields are not used when custom TcpOperations are provided.

I would propose to add a description to the StompBrokerRelayMessageHandler to allow the callers to provide customization for this information. Of course this would also require to add that to StompBrokerRelayRegistration in order to be able to pass it to the message handler. The description can even be a Supplier, which would allow to provide even more accurate information (in combination with the ClientOptions.Builder<?> that can be passed to the ReactorNettyTcpClient)

An additional benefit of this would be in the case when a Spring Boot actuator endpoint is used where one can use the BrokerAvailabilityEvent to show the up / down status for STOMP. This is not yet part of Boot, but we have a custom endpoint that does this.

---

Issue Links:

• Spring Websockets Broker relay supporting a cluster of STOMP endpoint addresses [SPR-12452] #17057 Spring Websockets Broker relay supporting a cluster of STOMP endpoint addresses

Referenced from: commits 37b0ed9, c555fef

[spring-projects-issues]

Rossen Stoyanchev commented

Indeed, after #17057 where TcpOperations can be configured through the Java config, it doesn't make sense to log the relay host and port. However, I'd prefer to avoid introducing yet another option into the mix that could also be easily overlooked.

I experimented with ReactorNettyTcpClient#toString() delegating to TcpClient. I get the following with Reactor Netty 0.7.x in master (same version is used in 5.0.x too):

TcpClient: connecting to /127.0.0.1:63074

We could work with that in StompBrokerRelayMessageHandler to always log via TcpOperations#toString(), instead of logging the relayHost/Port. If the output needs improvement, then we would ask the Reactor Netty team.

What is the version you're requesting this for? I did not check the output in 4.3.x yet. It may be different and the older reactor-net dependency is unlikely to change. Even there however you could wrap TcpOperations to override toString would be a way to get the same effect.

[spring-projects-issues]

Filip Hrisafov commented

We switched to Boot 2.0 and Spring 5. So for me it works to have it in 5.0.x only.

I had a quick look in the TcpClient and yes it is good to direct it to it. However, the way that its toString is created is less than ideal. It calls ClientOptions#asSimpleString() and that one just invokes the ClientOptions#getAddress() multiple times:

StringBuilder s = new StringBuilder();
if (getAddress() == null) {
    s.append("connecting to no base address");
}
else {
    s.append("connecting to ").append(getAddress());
}
if (proxyOptions != null) {
     s.append(" through ").append(proxyOptions.getType()).append(" proxy");
}
return s.toString();

And getAddress() calls the supplier actually, which means that if I have a round robin supplier the information would not be correct. This even leads to the fact that the current implementation in Reactor might be "dangerous" as it assumes that the Supplier creates the same instance on each call. Should I create an issue for Reactor for this?

In any case if the StompBrokerRelayMessageHandler directs to TcpOperations#toString(), I can in theory provide my own class with a custom description and override the ReactorNettyTcpClient#toString

[spring-projects-issues]

Rossen Stoyanchev commented

Indeed TcpClient#toString isn't very helpful for a custom Supplier where the address is per connection. I don't see what it could show differently but a Reactor Netty ticket might make sense still, at least to avoid logging the address if a custom Supplier was given.

Perhaps all we need to do in StompBrokerRelayMessageBroker is avoid logging the host and port, unless they were explicitly provided, and rely on logging from TcpClient instead? I see this with reactor.ipc at DEBUG level:

15:06:00 [tcp-client-loop-nio-1] TcpClient[DEBUG] - [id: 0x4e6eeca3] CONNECT: /127.0.0.1:61613

[spring-projects-issues]

Filip Hrisafov commented

I created a Reactor ticket (gh-353).

 
Directing to the TcpOperations would be more than enough from core perspective, it would be also quite easy to use a custom client that just extends the ReactorNettyTcpClient with a custom toString.

Thanks for looking into it

[spring-projects-issues]

Rossen Stoyanchev commented

Filip Hrisafov, to be clear what I meant is to stop logging any host or port information from StompBrokerRelayMessageHandler. Since that information may vary with each attempt to connect, it makes more sense to log it while connecting. TcpClient already logs something (the example from my previous comment) at DEBUG level, but we could also add a similar message in ReactorNettyTcpClient, for example around here so that changing the log level for spring-messaging would be self-sufficient.

What do you think?

[spring-projects-issues]

Filip Hrisafov commented

Not logging in the StompBrokerRelayMessageHandler is completely fine. My main reason for this was the BrokerAvailabilityEvent and it's information. That one invokes the toString() of the source, which is the StompBrokerRelayMessageHandler. I am using that event in a customized Spring Boot STOMP health endpoint.The BrokerAvailabilityEvent is also printed out at INFO level as well (when there is a change). All this will be of tremendous information to us.

The last thing would be to change the toString() in reactor-netty. I would appreciate if you can help out and to get the attention of the reactor team on this as well.

[spring-projects-issues]

Rossen Stoyanchev commented

I've updated StompBrokerRelayMessageHandler to delegate logging to TcpOperations that in turn delegates to TcpClient. I've also added a debug log the actual address whenever a connection is established.

[spring-projects-issues]

Filip Hrisafov commented

It's awesome. Thanks a lot @rstoya for adding this support.

[spring-projects-issues]

Rossen Stoyanchev commented

No worries, it's a logical complement to #17057.