Qpid
  1. Qpid
  2. QPID-4174

QMF events for (dis)connecting a client to have few more parameters

    Details

      Description

      I suggest enhancing values populated in 3 events relevant for client (dis)connection:

      clientConnect
      clientConnectFail
      clientDisconnect

      For these events, I suggest adding values:
      remoteParentPid
      remotePid
      remoteProcessName

      That is useful for various reasons:
      1) To have better information who just disconnected or failed to connect (i.e. better monitoring allowing to easily identify the process that is e.g. failing to connect repeatedly)
      2) To allow client processes (listening for the events) to easily know the disconnect event is relevant to itself by comparing its PID with remotePid.

      I am attaching a patch for it.

        Activity

        Hide
        Pavel Moravec added a comment -

        Proposed enhancement patch.

        Show
        Pavel Moravec added a comment - Proposed enhancement patch.
        Hide
        Rajith Attapattu added a comment -

        I'm not too keen about this approach.

        QMF has more details (ex process name, pid etc) at it's disposal. A monitoring application could use this already available information to correlate connections to events.
        If QMF doesn't send this information in events already, then it should probably do it.

        At the moment qpid.client_process is set as a jvm argument. This could be easily included on a per connection basis as well.
        That way you could give more meaningful identifier for each connection and then use them in monitoring applications.

        Show
        Rajith Attapattu added a comment - I'm not too keen about this approach. QMF has more details (ex process name, pid etc) at it's disposal. A monitoring application could use this already available information to correlate connections to events. If QMF doesn't send this information in events already, then it should probably do it. At the moment qpid.client_process is set as a jvm argument. This could be easily included on a per connection basis as well. That way you could give more meaningful identifier for each connection and then use them in monitoring applications.
        Hide
        Pavel Moravec added a comment -

        I agree enriching the connect / disconnect QMF event by more fields (i.e. remotePid or remoteProcessName) will be better approach - I will discuss it with Ted Ross. FYI, connect / disconnect QMF event is of form:

        {_schema_id:{_class_name:clientConnect, _hash:f59fe634-9c51-cfce-b3be-63bf063a0ac2, _package_name:org.apache.qpid.broker, _type:_event}, _severity:6, _timestamp:1343713841536668379, _values:{rhost:[::1]:5672-[::1]:44657, user:anonymous}}

        While compared to "get connections" QMF query returning much more information:

        {_create_ts:1343713833158589940, _delete_ts:0, _object_id:{_agent_epoch:9, _object_name:org.apache.qpid.broker:connection:[::1]:5672-[::1]:44656}, _schema_id:{_class_name:connection, _hash:09a39a61-d09a-6a80-1f85-670a97557a46, _package_name:org.apache.qpid.broker, _type:_data}, _update_ts:1343713833207393722, _values:{SystemConnection:False, address:[::1]:5672-[::1]:44656, authIdentity:anonymous, bytesFromClient:1157, bytesToClient:312, closing:False, federationLink:False, framesFromClient:18, framesToClient:10, incoming:True, msgsFromClient:1, msgsToClient:0, remoteParentPid:28802, remotePid:28819, remoteProcessName:watch-connectio, saslMechanism:, saslSsf:0, shadow:False, userProxyAuth:False, vhostRef:{_object_name:org.apache.qpid.broker:vhost:org.apache.qpid.broker:broker:amqp-broker,/}}}

        Show
        Pavel Moravec added a comment - I agree enriching the connect / disconnect QMF event by more fields (i.e. remotePid or remoteProcessName) will be better approach - I will discuss it with Ted Ross. FYI, connect / disconnect QMF event is of form: {_schema_id:{_class_name:clientConnect, _hash:f59fe634-9c51-cfce-b3be-63bf063a0ac2, _package_name:org.apache.qpid.broker, _type:_event}, _severity:6, _timestamp:1343713841536668379, _values:{rhost: [::1] :5672- [::1] :44657, user:anonymous}} While compared to "get connections" QMF query returning much more information: {_create_ts:1343713833158589940, _delete_ts:0, _object_id:{_agent_epoch:9, _object_name:org.apache.qpid.broker:connection: [::1] :5672- [::1] :44656}, _schema_id:{_class_name:connection, _hash:09a39a61-d09a-6a80-1f85-670a97557a46, _package_name:org.apache.qpid.broker, _type:_data}, _update_ts:1343713833207393722, _values:{SystemConnection:False, address: [::1] :5672- [::1] :44656, authIdentity:anonymous, bytesFromClient:1157, bytesToClient:312, closing:False, federationLink:False, framesFromClient:18, framesToClient:10, incoming:True, msgsFromClient:1, msgsToClient:0, remoteParentPid:28802, remotePid:28819, remoteProcessName:watch-connectio, saslMechanism:, saslSsf:0, shadow:False, userProxyAuth:False, vhostRef:{_object_name:org.apache.qpid.broker:vhost:org.apache.qpid.broker:broker:amqp-broker,/}}}
        Hide
        Pavel Moravec added a comment -

        Except for changing management-schema.xml and calls of the relevant methods, it was necessary to move some code from end of ConnectionHandler::Handler::startOk method to its beginning. That was required to have filled remote process name, PID and parentPID before raising the changed events.

        Show
        Pavel Moravec added a comment - Except for changing management-schema.xml and calls of the relevant methods, it was necessary to move some code from end of ConnectionHandler::Handler::startOk method to its beginning. That was required to have filled remote process name, PID and parentPID before raising the changed events.
        Hide
        Pavel Moravec added a comment -

        I changed the JIRA description as per my comment from 31st July, and attached patch.

        Show
        Pavel Moravec added a comment - I changed the JIRA description as per my comment from 31st July, and attached patch.
        Hide
        Ken Giusti added a comment -

        Hi Pavel,

        This is a great suggestion. I was thinking a bit about it and wondered if it would be more flexible to include the entire "client properties" map in the events rather than just the three properties mentioned above.

        What do you think? Right now, the QPID clients put the pid, ppid, and command name into the "client-properties" map. In theory, a non-QPID 0.10 client can put anything they'd like into it - so there's really no guarantee that pid, ppid, and command name have to be there. (see the description of the connection.start-ok command in the 0.10 spec).

        In AMQP 1.0, the Connection Open frame has an equivalent field for connection properties, which, I assume, would serve the same role for 1.0-based clients (see the description of the Open frame in the Performative section of the AMQP-1.0 spec)

        So, the argument list of the QMF events above would instead be something like this:

        <arg name="clientProperties" type="map" desc="A map containing optional information about the remote client"/>

        <event name="clientConnect" sev="inform" args="rhost, user, clientProperties"/>
        <event name="clientConnectFail" sev="warn" args="rhost, user, reason, clientProperties"/>
        <event name="clientDisconnect" sev="inform" args="rhost, user, clientProperties"/>

        And the pid, ppid, and command name could be accessed via the clientProperties map using the keys "qpid.client_pid", "qpid.client_ppid", and "qpid.client_process", respectively.

        The benefit of this approach is that it allows us to add new properties in the future without having to write additional QMF code to support them. The downside is that the management app would have to index the map manually.

        Just a consideration - what do you think?

        -K

        Show
        Ken Giusti added a comment - Hi Pavel, This is a great suggestion. I was thinking a bit about it and wondered if it would be more flexible to include the entire "client properties" map in the events rather than just the three properties mentioned above. What do you think? Right now, the QPID clients put the pid, ppid, and command name into the "client-properties" map. In theory, a non-QPID 0.10 client can put anything they'd like into it - so there's really no guarantee that pid, ppid, and command name have to be there. (see the description of the connection.start-ok command in the 0.10 spec). In AMQP 1.0, the Connection Open frame has an equivalent field for connection properties, which, I assume, would serve the same role for 1.0-based clients (see the description of the Open frame in the Performative section of the AMQP-1.0 spec) So, the argument list of the QMF events above would instead be something like this: <arg name="clientProperties" type="map" desc="A map containing optional information about the remote client"/> <event name="clientConnect" sev="inform" args="rhost, user, clientProperties"/> <event name="clientConnectFail" sev="warn" args="rhost, user, reason, clientProperties"/> <event name="clientDisconnect" sev="inform" args="rhost, user, clientProperties"/> And the pid, ppid, and command name could be accessed via the clientProperties map using the keys "qpid.client_pid", "qpid.client_ppid", and "qpid.client_process", respectively. The benefit of this approach is that it allows us to add new properties in the future without having to write additional QMF code to support them. The downside is that the management app would have to index the map manually. Just a consideration - what do you think? -K
        Hide
        Pavel Moravec added a comment -

        Hi Ken,
        I agree with your suggestion, good idea.

        Pavel

        Show
        Pavel Moravec added a comment - Hi Ken, I agree with your suggestion, good idea. Pavel
        Show
        Ken Giusti added a comment - Reviewboard: https://reviews.apache.org/r/6955/ Trunk submission: http://svn.apache.org/viewvc?view=revision&revision=1382830
        Show
        Ken Giusti added a comment - http://svn.apache.org/viewvc?view=revision&revision=1382830

          People

          • Assignee:
            Ken Giusti
            Reporter:
            Pavel Moravec
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development