rabbitmq / rabbitmq-stomp

RabbitMQ STOMP plugin
https://www.rabbitmq.com/stomp.html
Other
49 stars 28 forks source link

Connection infos missing in management #55

Closed essen closed 8 years ago

essen commented 8 years ago

The title mentions management but this is a rabbitmq-stomp issue. The issue also affects rabbitmq-web-stomp.

As reported on the mailing list at https://groups.google.com/forum/#!topic/rabbitmq-users/PS236sFKjKM the STOMP connections with TLS enabled do not report their state and other stats properly.

essen commented 8 years ago

Culprit is https://github.com/rabbitmq/rabbitmq-server/issues/246 which did not update the amqp_client code here: https://github.com/rabbitmq/rabbitmq-erlang-client/blob/master/src/amqp_direct_connection.erl#L193

essen commented 8 years ago

Sent a PR fixing "SSL details". But there's still no "running" state, and no channels, max, auth, client or stats.

This also didn't fix Web-STOMP which still shows no details.

I didn't try MQTT/TLS yet.

essen commented 8 years ago

Writing down what I figured so far.

STOMP does not have stats emitting code. AMQP's reader code has. STOMP only relies on amqp_direct_connection to send info about the connection (and those are only sent once, or when refresh is forced, and don't have access to things like the running state of the connection).

The values are:

michaelklishin commented 8 years ago

That can be fine if we allow STOMP/MQTT/etc inject more information into the events emitted. WDYT?

essen commented 8 years ago

We need a combination of adding info to existing events (trivial) and periodically send events for stats (less trivial, no code currently does this outside AMQP's rabbit_reader AFAIK). I'm checking who knows what for the different protocols first and will then work on a patch.

essen commented 8 years ago

Web-STOMP is basically the same as STOMP except that state is always "running" because credits are not handled. We can probably hardcode the value there.

Web-STOMP is currently lacking extra SSL infos, those are set in amqp_direct_connection, just needs to figure out why they aren't set in this case.

essen commented 8 years ago

Web-MQTT masquerades as MQTT ("MQTT 3.1.1" in my case) in the connections list. We probably want that as "Web MQTT 3.1.1" instead, to indicate this is MQTT 3.1.1 over Websocket. Unlike Web-STOMP, the SSL details for Web-MQTT are filed (this might be a good idea to check what is done and do the same for Web-STOMP).

Web-MQTT, like Web-STOMP, doesn't handle credits and so its state should always be "running".

MQTT is basically in the same state as STOMP. Let's take a look at the individual values to see what needs to be done:

johnfoldager commented 8 years ago

I guess this is not fixed for the 3.6.1 RC1 but do you think that it will be coming soon? The Milestone for this is set for 3.6.1.

michaelklishin commented 8 years ago

@johnfoldager someone who's been working on this issue is currently on leave for personal reasons. It also ended up being way more work than we expected. I'm changing milestone to 3.6.x.

michaelklishin commented 8 years ago

We did some research and most of protocol-specific connection info is actually not emitted at all. So we expanded the scope of this to make it easier to display more details for connections in the future.

essen commented 8 years ago

I am almost done with a first try at this. I got (some) stats for all connection types: MQTT/Web MQTT and STOMP/Web STOMP. Some info I do not have and will need to investigate further. Some info I can't provide, for example "frame max". I am not sure what to do about it. Not sending anything is harmless, just a bit ugly. I am also missing "auth mechanism", "client" (often not available due to protocol limitations) and "heartbeat" which I need to look further at.

I estimate the work left for what I have so far to be less than a day. It's mostly cleaning things up at this point. It's already a big step up from what we had before as most fields are now filled/updated and graphs animate as expected.

michaelklishin commented 8 years ago

Not all protocols have frame_max, so undefined or none are not unreasonable values.

essen commented 8 years ago

Started opening PRs for this. Taking more time than expected again. Testing it takes a while. Will have Web STOMP ready next.

essen commented 8 years ago

STOMP and Web STOMP are ready. They can be tested and merged independently from the coming MQTT and Web MQTT changes.

essen commented 8 years ago

I've opened a PR on all repositories. We still need to decide if N/A or something else is preferred.

There will be auth mechanism and heartbeat to do separately still but we have a pretty good start with these PRs.

michaelklishin commented 8 years ago

The scope of this has grown to include MQTT, Web STOMP, and Web MQTT. STOMP and MQTT are already merged. Their WebSocket counterparts are to follow soon.