Skip to content
This repository has been archived by the owner on Nov 17, 2020. It is now read-only.

Use user specified connection name in logs #101

Conversation

binarin
Copy link
Contributor

@binarin binarin commented Jun 1, 2016

@binarin binarin force-pushed the rabbitmq-common-log-user-specified-connection-name branch from 4c7d882 to d675c73 Compare June 1, 2016 13:41
@michaelklishin
Copy link
Member

As far as I see this replaces connection name with a user-provided one. We cannot do this because TCP socket-based names also provide valuable information (and are used in the HTTP API).

We should append user-provided names to the socket-derived ID.

@binarin
Copy link
Contributor Author

binarin commented Jun 1, 2016

It's actually appended, like this - <<Name/binary, " - ", UserSpecifiedName/binary>>. I decided to do it that way, because adding new field to #connection{} would also require updating all logging statements.

@binarin
Copy link
Contributor Author

binarin commented Jun 1, 2016

Also I didn't think about management plugin - with this patch there is no need in special handling of custom connection name, it is automatically shown as a part of name.

@binarin
Copy link
Contributor Author

binarin commented Jun 1, 2016

Looks like this patch is not that useful in current form, exception logging is using old name.

@michaelklishin
Copy link
Member

@binarin I personally would simply update a few call sites that log connection events. That's where most of the value is compared to what we have today.

@binarin binarin force-pushed the rabbitmq-common-log-user-specified-connection-name branch from d675c73 to 2d982d9 Compare June 1, 2016 14:09
@binarin binarin force-pushed the rabbitmq-common-log-user-specified-connection-name branch from 2d982d9 to a3926e1 Compare June 1, 2016 14:15
@binarin
Copy link
Contributor Author

binarin commented Jun 1, 2016

Updated the patch - looks like this is the only simple way to log something useful. Because adding field to #connection{} will require moving exception handling into mainloop/recvloop.

@michaelklishin michaelklishin self-assigned this Jun 1, 2016
@michaelklishin michaelklishin added this to the 3.6.3 milestone Jun 1, 2016
@michaelklishin
Copy link
Member

Sorry but we cannot merge this as is. This changes connection name internally and thus connection ID as reported by the management UI, for example:

[
    {
…
        "client_properties": {
            "capabilities": {
                "authentication_failure_close": true,
                "basic.nack": true,
                "connection.blocked": true,
                "consumer_cancel_notify": true,
                "exchange_exchange_bindings": true,
                "publisher_confirms": true
            },
            "connection_name": "Bunny REPL 1",
            "information": "http://rubybunny.info",
            "platform": "ruby 2.3.1p112 (2016-04-26 revision 54768) [x86_64-darwin15]",
            "product": "Bunny",
            "version": "2.5.0.pre"
        },
…
        "name": "127.0.0.1:49991 -> 127.0.0.1:5672 - Bunny REPL 1",
…
        "user": "guest",
        "vhost": "/"
    }
]

@michaelklishin
Copy link
Member

…and this will definitely break some monitoring tools in ways that we cannot foresee. Maybe a more significant set of changes for 3.7.0 is what we should do instead.

@binarin
Copy link
Contributor Author

binarin commented Jun 20, 2016

Will it be accepted as-is to master? If not, what are the requirements to make it suitable for either stable or master?

@michaelklishin
Copy link
Member

Connection IDs as used by the HTTP API stay unchanged.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants