[GitHub] activemq-artemis pull request #1592: NO-JIRA improve MQTT protocol logging

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
34 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1592: NO-JIRA improve MQTT protocol logging

pgfox
GitHub user jbertram opened a pull request:

    https://github.com/apache/activemq-artemis/pull/1592

    NO-JIRA improve MQTT protocol logging

   

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/jbertram/activemq-artemis master_work

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/activemq-artemis/pull/1592.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #1592
   
----
commit 5c072a4975588e336cfc3a1904e12c2aff6ac1f4
Author: Justin Bertram <[hidden email]>
Date:   2017-10-17T19:03:08Z

    NO-JIRA improve MQTT protocol logging

----


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1592: NO-JIRA improve MQTT protocol logging

pgfox
Github user michaelandrepearce commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/1592#discussion_r145227400
 
    --- Diff: artemis-protocols/artemis-mqtt-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/mqtt/MQTTUtil.java ---
    @@ -149,16 +154,52 @@ public static void logMessage(MQTTSessionState state, MqttMessage message, boole
              if (message.fixedHeader() != null) {
                 log.append(message.fixedHeader().messageType().toString());
     
    -            if (message.variableHeader() instanceof MqttPublishVariableHeader) {
    -               log.append("(" + ((MqttPublishVariableHeader) message.variableHeader()).messageId() + ") " + message.fixedHeader().qosLevel());
    -            } else if (message.variableHeader() instanceof MqttMessageIdVariableHeader) {
    +            if (message.variableHeader() instanceof MqttMessageIdVariableHeader) {
                    log.append("(" + ((MqttMessageIdVariableHeader) message.variableHeader()).messageId() + ")");
                 }
     
    -            if (message.fixedHeader().messageType() == MqttMessageType.SUBSCRIBE) {
    -               for (MqttTopicSubscription sub : ((MqttSubscribeMessage) message).payload().topicSubscriptions()) {
    -                  log.append("\n\t" + sub.topicName() + " : " + sub.qualityOfService());
    -               }
    +            switch (message.fixedHeader().messageType()) {
    +               case PUBLISH:
    +                  MqttPublishVariableHeader publishHeader = (MqttPublishVariableHeader) message.variableHeader();
    +                  log.append("(" + publishHeader.packetId() + ")")
    +                     .append(" topic=" + publishHeader.topicName())
    +                     .append(", qos=" + message.fixedHeader().qosLevel())
    +                     .append(", retain=" + message.fixedHeader().isRetain())
    +                     .append(", dup=" + message.fixedHeader().isDup());
    +                     //.append(" payload=" + ((MqttPublishMessage)message).payload().toString(StandardCharsets.UTF_8));
    +                  break;
    +               case CONNECT:
    +                  MqttConnectVariableHeader connectHeader = (MqttConnectVariableHeader) message.variableHeader();
    +                  MqttConnectPayload payload = ((MqttConnectMessage)message).payload();
    +                  log.append(" name=").append(connectHeader.name())
    +                     .append(", version=").append(connectHeader.version())
    +                     .append(", hasUserName=").append(connectHeader.hasUserName())
    +                     .append(", hasPassword=").append(connectHeader.hasPassword())
    +                     .append(", isWillRetain=").append(connectHeader.isWillRetain())
    +                     .append(", isWillFlag=").append(connectHeader.isWillFlag())
    +                     .append(", isCleanSession=").append(connectHeader.isCleanSession())
    +                     .append(", keepAliveTimeSeconds=").append(connectHeader.keepAliveTimeSeconds())
    +                     .append(", clientIdentifier=").append(payload.clientIdentifier())
    +                     .append(", willTopic=").append(payload.willTopic())
    +                     .append(", willMessage=").append(payload.willMessage())
    +                     .append(", userName=").append(payload.userName());
    +                     //.append(", password=").append(payload.password());
    --- End diff --
   
    rather than commented out should this just be removed.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1592: NO-JIRA improve MQTT protocol logging

pgfox
In reply to this post by pgfox
Github user michaelandrepearce commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/1592#discussion_r145227520
 
    --- Diff: artemis-protocols/artemis-mqtt-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/mqtt/MQTTUtil.java ---
    @@ -149,16 +154,52 @@ public static void logMessage(MQTTSessionState state, MqttMessage message, boole
              if (message.fixedHeader() != null) {
                 log.append(message.fixedHeader().messageType().toString());
     
    -            if (message.variableHeader() instanceof MqttPublishVariableHeader) {
    -               log.append("(" + ((MqttPublishVariableHeader) message.variableHeader()).messageId() + ") " + message.fixedHeader().qosLevel());
    -            } else if (message.variableHeader() instanceof MqttMessageIdVariableHeader) {
    +            if (message.variableHeader() instanceof MqttMessageIdVariableHeader) {
                    log.append("(" + ((MqttMessageIdVariableHeader) message.variableHeader()).messageId() + ")");
                 }
     
    -            if (message.fixedHeader().messageType() == MqttMessageType.SUBSCRIBE) {
    -               for (MqttTopicSubscription sub : ((MqttSubscribeMessage) message).payload().topicSubscriptions()) {
    -                  log.append("\n\t" + sub.topicName() + " : " + sub.qualityOfService());
    -               }
    +            switch (message.fixedHeader().messageType()) {
    +               case PUBLISH:
    +                  MqttPublishVariableHeader publishHeader = (MqttPublishVariableHeader) message.variableHeader();
    +                  log.append("(" + publishHeader.packetId() + ")")
    +                     .append(" topic=" + publishHeader.topicName())
    +                     .append(", qos=" + message.fixedHeader().qosLevel())
    +                     .append(", retain=" + message.fixedHeader().isRetain())
    +                     .append(", dup=" + message.fixedHeader().isDup());
    +                     //.append(" payload=" + ((MqttPublishMessage)message).payload().toString(StandardCharsets.UTF_8));
    --- End diff --
   
    ditto as per other comment.
    "rather than commented out should this just be removed."


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1592: NO-JIRA improve MQTT protocol logging

pgfox
In reply to this post by pgfox
Github user ppatierno commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1592
 
    Some comments :
   
    - You can avoid to show will information if the isWillFlag is false so there aren't empty fields to show
    - avoid to show the will message payload as you are avoiding to show the publish payload. I guess you are using an old version of the MQTT Netty codec where will payload is a String but in the newer version it's a byte array like publish payload
    - having the CONNACK packet as well with the result code and if there is a session is useful
    - having PUBACK, PUBREL, PUBCOMP are useful as well with related packet-id


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1592: NO-JIRA improve MQTT protocol logging

pgfox
In reply to this post by pgfox
Github user jbertram commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/1592#discussion_r145230933
 
    --- Diff: artemis-protocols/artemis-mqtt-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/mqtt/MQTTUtil.java ---
    @@ -149,16 +154,52 @@ public static void logMessage(MQTTSessionState state, MqttMessage message, boole
              if (message.fixedHeader() != null) {
                 log.append(message.fixedHeader().messageType().toString());
     
    -            if (message.variableHeader() instanceof MqttPublishVariableHeader) {
    -               log.append("(" + ((MqttPublishVariableHeader) message.variableHeader()).messageId() + ") " + message.fixedHeader().qosLevel());
    -            } else if (message.variableHeader() instanceof MqttMessageIdVariableHeader) {
    +            if (message.variableHeader() instanceof MqttMessageIdVariableHeader) {
                    log.append("(" + ((MqttMessageIdVariableHeader) message.variableHeader()).messageId() + ")");
                 }
     
    -            if (message.fixedHeader().messageType() == MqttMessageType.SUBSCRIBE) {
    -               for (MqttTopicSubscription sub : ((MqttSubscribeMessage) message).payload().topicSubscriptions()) {
    -                  log.append("\n\t" + sub.topicName() + " : " + sub.qualityOfService());
    -               }
    +            switch (message.fixedHeader().messageType()) {
    +               case PUBLISH:
    +                  MqttPublishVariableHeader publishHeader = (MqttPublishVariableHeader) message.variableHeader();
    +                  log.append("(" + publishHeader.packetId() + ")")
    +                     .append(" topic=" + publishHeader.topicName())
    +                     .append(", qos=" + message.fixedHeader().qosLevel())
    +                     .append(", retain=" + message.fixedHeader().isRetain())
    +                     .append(", dup=" + message.fixedHeader().isDup());
    +                     //.append(" payload=" + ((MqttPublishMessage)message).payload().toString(StandardCharsets.UTF_8));
    +                  break;
    +               case CONNECT:
    +                  MqttConnectVariableHeader connectHeader = (MqttConnectVariableHeader) message.variableHeader();
    +                  MqttConnectPayload payload = ((MqttConnectMessage)message).payload();
    +                  log.append(" name=").append(connectHeader.name())
    +                     .append(", version=").append(connectHeader.version())
    +                     .append(", hasUserName=").append(connectHeader.hasUserName())
    +                     .append(", hasPassword=").append(connectHeader.hasPassword())
    +                     .append(", isWillRetain=").append(connectHeader.isWillRetain())
    +                     .append(", isWillFlag=").append(connectHeader.isWillFlag())
    +                     .append(", isCleanSession=").append(connectHeader.isCleanSession())
    +                     .append(", keepAliveTimeSeconds=").append(connectHeader.keepAliveTimeSeconds())
    +                     .append(", clientIdentifier=").append(payload.clientIdentifier())
    +                     .append(", willTopic=").append(payload.willTopic())
    +                     .append(", willMessage=").append(payload.willMessage())
    +                     .append(", userName=").append(payload.userName());
    +                     //.append(", password=").append(payload.password());
    --- End diff --
   
    From a security perspective I think that's a fair point.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1592: NO-JIRA improve MQTT protocol logging

pgfox
In reply to this post by pgfox
Github user ppatierno commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/1592#discussion_r145231002
 
    --- Diff: artemis-protocols/artemis-mqtt-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/mqtt/MQTTUtil.java ---
    @@ -149,16 +154,52 @@ public static void logMessage(MQTTSessionState state, MqttMessage message, boole
              if (message.fixedHeader() != null) {
                 log.append(message.fixedHeader().messageType().toString());
     
    -            if (message.variableHeader() instanceof MqttPublishVariableHeader) {
    -               log.append("(" + ((MqttPublishVariableHeader) message.variableHeader()).messageId() + ") " + message.fixedHeader().qosLevel());
    -            } else if (message.variableHeader() instanceof MqttMessageIdVariableHeader) {
    +            if (message.variableHeader() instanceof MqttMessageIdVariableHeader) {
                    log.append("(" + ((MqttMessageIdVariableHeader) message.variableHeader()).messageId() + ")");
                 }
     
    -            if (message.fixedHeader().messageType() == MqttMessageType.SUBSCRIBE) {
    -               for (MqttTopicSubscription sub : ((MqttSubscribeMessage) message).payload().topicSubscriptions()) {
    -                  log.append("\n\t" + sub.topicName() + " : " + sub.qualityOfService());
    -               }
    +            switch (message.fixedHeader().messageType()) {
    +               case PUBLISH:
    +                  MqttPublishVariableHeader publishHeader = (MqttPublishVariableHeader) message.variableHeader();
    +                  log.append("(" + publishHeader.packetId() + ")")
    +                     .append(" topic=" + publishHeader.topicName())
    +                     .append(", qos=" + message.fixedHeader().qosLevel())
    +                     .append(", retain=" + message.fixedHeader().isRetain())
    +                     .append(", dup=" + message.fixedHeader().isDup());
    +                     //.append(" payload=" + ((MqttPublishMessage)message).payload().toString(StandardCharsets.UTF_8));
    +                  break;
    +               case CONNECT:
    +                  MqttConnectVariableHeader connectHeader = (MqttConnectVariableHeader) message.variableHeader();
    +                  MqttConnectPayload payload = ((MqttConnectMessage)message).payload();
    +                  log.append(" name=").append(connectHeader.name())
    +                     .append(", version=").append(connectHeader.version())
    +                     .append(", hasUserName=").append(connectHeader.hasUserName())
    +                     .append(", hasPassword=").append(connectHeader.hasPassword())
    +                     .append(", isWillRetain=").append(connectHeader.isWillRetain())
    +                     .append(", isWillFlag=").append(connectHeader.isWillFlag())
    +                     .append(", isCleanSession=").append(connectHeader.isCleanSession())
    +                     .append(", keepAliveTimeSeconds=").append(connectHeader.keepAliveTimeSeconds())
    +                     .append(", clientIdentifier=").append(payload.clientIdentifier())
    +                     .append(", willTopic=").append(payload.willTopic())
    +                     .append(", willMessage=").append(payload.willMessage())
    +                     .append(", userName=").append(payload.userName());
    +                     //.append(", password=").append(payload.password());
    +                  break;
    +               case SUBSCRIBE:
    +                  for (MqttTopicSubscription sub : ((MqttSubscribeMessage) message).payload().topicSubscriptions()) {
    +                     log.append("\n\t" + sub.topicName() + " : " + sub.qualityOfService());
    +                  }
    +                  break;
    +               case SUBACK:
    +                  for (Integer qos : ((MqttSubAckMessage) message).payload().grantedQoSLevels()) {
    +                     log.append("\n\t" + qos);
    --- End diff --
   
    Ditto as above


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1592: NO-JIRA improve MQTT protocol logging

pgfox
In reply to this post by pgfox
Github user ppatierno commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/1592#discussion_r145230929
 
    --- Diff: artemis-protocols/artemis-mqtt-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/mqtt/MQTTUtil.java ---
    @@ -149,16 +154,52 @@ public static void logMessage(MQTTSessionState state, MqttMessage message, boole
              if (message.fixedHeader() != null) {
                 log.append(message.fixedHeader().messageType().toString());
     
    -            if (message.variableHeader() instanceof MqttPublishVariableHeader) {
    -               log.append("(" + ((MqttPublishVariableHeader) message.variableHeader()).messageId() + ") " + message.fixedHeader().qosLevel());
    -            } else if (message.variableHeader() instanceof MqttMessageIdVariableHeader) {
    +            if (message.variableHeader() instanceof MqttMessageIdVariableHeader) {
                    log.append("(" + ((MqttMessageIdVariableHeader) message.variableHeader()).messageId() + ")");
                 }
     
    -            if (message.fixedHeader().messageType() == MqttMessageType.SUBSCRIBE) {
    -               for (MqttTopicSubscription sub : ((MqttSubscribeMessage) message).payload().topicSubscriptions()) {
    -                  log.append("\n\t" + sub.topicName() + " : " + sub.qualityOfService());
    -               }
    +            switch (message.fixedHeader().messageType()) {
    +               case PUBLISH:
    +                  MqttPublishVariableHeader publishHeader = (MqttPublishVariableHeader) message.variableHeader();
    +                  log.append("(" + publishHeader.packetId() + ")")
    +                     .append(" topic=" + publishHeader.topicName())
    +                     .append(", qos=" + message.fixedHeader().qosLevel())
    +                     .append(", retain=" + message.fixedHeader().isRetain())
    +                     .append(", dup=" + message.fixedHeader().isDup());
    +                     //.append(" payload=" + ((MqttPublishMessage)message).payload().toString(StandardCharsets.UTF_8));
    +                  break;
    +               case CONNECT:
    +                  MqttConnectVariableHeader connectHeader = (MqttConnectVariableHeader) message.variableHeader();
    +                  MqttConnectPayload payload = ((MqttConnectMessage)message).payload();
    +                  log.append(" name=").append(connectHeader.name())
    +                     .append(", version=").append(connectHeader.version())
    +                     .append(", hasUserName=").append(connectHeader.hasUserName())
    +                     .append(", hasPassword=").append(connectHeader.hasPassword())
    +                     .append(", isWillRetain=").append(connectHeader.isWillRetain())
    +                     .append(", isWillFlag=").append(connectHeader.isWillFlag())
    +                     .append(", isCleanSession=").append(connectHeader.isCleanSession())
    +                     .append(", keepAliveTimeSeconds=").append(connectHeader.keepAliveTimeSeconds())
    +                     .append(", clientIdentifier=").append(payload.clientIdentifier())
    +                     .append(", willTopic=").append(payload.willTopic())
    +                     .append(", willMessage=").append(payload.willMessage())
    +                     .append(", userName=").append(payload.userName());
    +                     //.append(", password=").append(payload.password());
    +                  break;
    +               case SUBSCRIBE:
    +                  for (MqttTopicSubscription sub : ((MqttSubscribeMessage) message).payload().topicSubscriptions()) {
    +                     log.append("\n\t" + sub.topicName() + " : " + sub.qualityOfService());
    --- End diff --
   
    It should shows the packet-id as well


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1592: NO-JIRA improve MQTT protocol logging

pgfox
In reply to this post by pgfox
Github user ppatierno commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/1592#discussion_r145231034
 
    --- Diff: artemis-protocols/artemis-mqtt-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/mqtt/MQTTUtil.java ---
    @@ -149,16 +154,52 @@ public static void logMessage(MQTTSessionState state, MqttMessage message, boole
              if (message.fixedHeader() != null) {
                 log.append(message.fixedHeader().messageType().toString());
     
    -            if (message.variableHeader() instanceof MqttPublishVariableHeader) {
    -               log.append("(" + ((MqttPublishVariableHeader) message.variableHeader()).messageId() + ") " + message.fixedHeader().qosLevel());
    -            } else if (message.variableHeader() instanceof MqttMessageIdVariableHeader) {
    +            if (message.variableHeader() instanceof MqttMessageIdVariableHeader) {
                    log.append("(" + ((MqttMessageIdVariableHeader) message.variableHeader()).messageId() + ")");
                 }
     
    -            if (message.fixedHeader().messageType() == MqttMessageType.SUBSCRIBE) {
    -               for (MqttTopicSubscription sub : ((MqttSubscribeMessage) message).payload().topicSubscriptions()) {
    -                  log.append("\n\t" + sub.topicName() + " : " + sub.qualityOfService());
    -               }
    +            switch (message.fixedHeader().messageType()) {
    +               case PUBLISH:
    +                  MqttPublishVariableHeader publishHeader = (MqttPublishVariableHeader) message.variableHeader();
    +                  log.append("(" + publishHeader.packetId() + ")")
    +                     .append(" topic=" + publishHeader.topicName())
    +                     .append(", qos=" + message.fixedHeader().qosLevel())
    +                     .append(", retain=" + message.fixedHeader().isRetain())
    +                     .append(", dup=" + message.fixedHeader().isDup());
    +                     //.append(" payload=" + ((MqttPublishMessage)message).payload().toString(StandardCharsets.UTF_8));
    +                  break;
    +               case CONNECT:
    +                  MqttConnectVariableHeader connectHeader = (MqttConnectVariableHeader) message.variableHeader();
    +                  MqttConnectPayload payload = ((MqttConnectMessage)message).payload();
    +                  log.append(" name=").append(connectHeader.name())
    +                     .append(", version=").append(connectHeader.version())
    +                     .append(", hasUserName=").append(connectHeader.hasUserName())
    +                     .append(", hasPassword=").append(connectHeader.hasPassword())
    +                     .append(", isWillRetain=").append(connectHeader.isWillRetain())
    +                     .append(", isWillFlag=").append(connectHeader.isWillFlag())
    +                     .append(", isCleanSession=").append(connectHeader.isCleanSession())
    +                     .append(", keepAliveTimeSeconds=").append(connectHeader.keepAliveTimeSeconds())
    +                     .append(", clientIdentifier=").append(payload.clientIdentifier())
    +                     .append(", willTopic=").append(payload.willTopic())
    +                     .append(", willMessage=").append(payload.willMessage())
    +                     .append(", userName=").append(payload.userName());
    +                     //.append(", password=").append(payload.password());
    +                  break;
    +               case SUBSCRIBE:
    +                  for (MqttTopicSubscription sub : ((MqttSubscribeMessage) message).payload().topicSubscriptions()) {
    +                     log.append("\n\t" + sub.topicName() + " : " + sub.qualityOfService());
    +                  }
    +                  break;
    +               case SUBACK:
    +                  for (Integer qos : ((MqttSubAckMessage) message).payload().grantedQoSLevels()) {
    +                     log.append("\n\t" + qos);
    +                  }
    +                  break;
    +               case UNSUBSCRIBE:
    +                  for (String topic : ((MqttUnsubscribeMessage) message).payload().topics()) {
    +                     log.append("\n\t" + topic);
    --- End diff --
   
    Ditto as above


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1592: NO-JIRA improve MQTT protocol logging

pgfox
In reply to this post by pgfox
Github user jbertram commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/1592#discussion_r145231515
 
    --- Diff: artemis-protocols/artemis-mqtt-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/mqtt/MQTTUtil.java ---
    @@ -149,16 +154,52 @@ public static void logMessage(MQTTSessionState state, MqttMessage message, boole
              if (message.fixedHeader() != null) {
                 log.append(message.fixedHeader().messageType().toString());
     
    -            if (message.variableHeader() instanceof MqttPublishVariableHeader) {
    -               log.append("(" + ((MqttPublishVariableHeader) message.variableHeader()).messageId() + ") " + message.fixedHeader().qosLevel());
    -            } else if (message.variableHeader() instanceof MqttMessageIdVariableHeader) {
    +            if (message.variableHeader() instanceof MqttMessageIdVariableHeader) {
                    log.append("(" + ((MqttMessageIdVariableHeader) message.variableHeader()).messageId() + ")");
                 }
     
    -            if (message.fixedHeader().messageType() == MqttMessageType.SUBSCRIBE) {
    -               for (MqttTopicSubscription sub : ((MqttSubscribeMessage) message).payload().topicSubscriptions()) {
    -                  log.append("\n\t" + sub.topicName() + " : " + sub.qualityOfService());
    -               }
    +            switch (message.fixedHeader().messageType()) {
    +               case PUBLISH:
    +                  MqttPublishVariableHeader publishHeader = (MqttPublishVariableHeader) message.variableHeader();
    +                  log.append("(" + publishHeader.packetId() + ")")
    +                     .append(" topic=" + publishHeader.topicName())
    +                     .append(", qos=" + message.fixedHeader().qosLevel())
    +                     .append(", retain=" + message.fixedHeader().isRetain())
    +                     .append(", dup=" + message.fixedHeader().isDup());
    +                     //.append(" payload=" + ((MqttPublishMessage)message).payload().toString(StandardCharsets.UTF_8));
    --- End diff --
   
    I disagree that this should be removed as it can serve as a really convenient way to look at the payload granted they're UTF-8 encoded strings.  The only reason it's commented out is because the MQTT spec has no rules on the format of the payload.  It's just an array of bytes.  However, if you happen to be working with UTF-8 encoded string then this might prove helpful.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1592: NO-JIRA improve MQTT protocol logging

pgfox
In reply to this post by pgfox
Github user ppatierno commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/1592#discussion_r145232049
 
    --- Diff: artemis-protocols/artemis-mqtt-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/mqtt/MQTTUtil.java ---
    @@ -149,16 +154,52 @@ public static void logMessage(MQTTSessionState state, MqttMessage message, boole
              if (message.fixedHeader() != null) {
                 log.append(message.fixedHeader().messageType().toString());
     
    -            if (message.variableHeader() instanceof MqttPublishVariableHeader) {
    -               log.append("(" + ((MqttPublishVariableHeader) message.variableHeader()).messageId() + ") " + message.fixedHeader().qosLevel());
    -            } else if (message.variableHeader() instanceof MqttMessageIdVariableHeader) {
    +            if (message.variableHeader() instanceof MqttMessageIdVariableHeader) {
                    log.append("(" + ((MqttMessageIdVariableHeader) message.variableHeader()).messageId() + ")");
                 }
     
    -            if (message.fixedHeader().messageType() == MqttMessageType.SUBSCRIBE) {
    -               for (MqttTopicSubscription sub : ((MqttSubscribeMessage) message).payload().topicSubscriptions()) {
    -                  log.append("\n\t" + sub.topicName() + " : " + sub.qualityOfService());
    -               }
    +            switch (message.fixedHeader().messageType()) {
    +               case PUBLISH:
    +                  MqttPublishVariableHeader publishHeader = (MqttPublishVariableHeader) message.variableHeader();
    +                  log.append("(" + publishHeader.packetId() + ")")
    +                     .append(" topic=" + publishHeader.topicName())
    +                     .append(", qos=" + message.fixedHeader().qosLevel())
    +                     .append(", retain=" + message.fixedHeader().isRetain())
    +                     .append(", dup=" + message.fixedHeader().isDup());
    +                     //.append(" payload=" + ((MqttPublishMessage)message).payload().toString(StandardCharsets.UTF_8));
    --- End diff --
   
    Yes I agree that is useful if you are using UTF8 encoded string as payload. Is it possible having this logging line as configurable (disabled by default) so that people using Strings can enable it for showing the payload.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1592: NO-JIRA improve MQTT protocol logging

pgfox
In reply to this post by pgfox
Github user tabish121 commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/1592#discussion_r145232266
 
    --- Diff: artemis-protocols/artemis-mqtt-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/mqtt/MQTTUtil.java ---
    @@ -149,16 +154,52 @@ public static void logMessage(MQTTSessionState state, MqttMessage message, boole
              if (message.fixedHeader() != null) {
                 log.append(message.fixedHeader().messageType().toString());
     
    -            if (message.variableHeader() instanceof MqttPublishVariableHeader) {
    -               log.append("(" + ((MqttPublishVariableHeader) message.variableHeader()).messageId() + ") " + message.fixedHeader().qosLevel());
    -            } else if (message.variableHeader() instanceof MqttMessageIdVariableHeader) {
    +            if (message.variableHeader() instanceof MqttMessageIdVariableHeader) {
                    log.append("(" + ((MqttMessageIdVariableHeader) message.variableHeader()).messageId() + ")");
                 }
     
    -            if (message.fixedHeader().messageType() == MqttMessageType.SUBSCRIBE) {
    -               for (MqttTopicSubscription sub : ((MqttSubscribeMessage) message).payload().topicSubscriptions()) {
    -                  log.append("\n\t" + sub.topicName() + " : " + sub.qualityOfService());
    -               }
    +            switch (message.fixedHeader().messageType()) {
    +               case PUBLISH:
    +                  MqttPublishVariableHeader publishHeader = (MqttPublishVariableHeader) message.variableHeader();
    +                  log.append("(" + publishHeader.packetId() + ")")
    +                     .append(" topic=" + publishHeader.topicName())
    +                     .append(", qos=" + message.fixedHeader().qosLevel())
    +                     .append(", retain=" + message.fixedHeader().isRetain())
    +                     .append(", dup=" + message.fixedHeader().isDup());
    +                     //.append(" payload=" + ((MqttPublishMessage)message).payload().toString(StandardCharsets.UTF_8));
    --- End diff --
   
    If that's the case would configuration to turn it on / off be better than requiring a complete rebuild in order to use them once uncommented ?
   
    Sidebar is there a gating mechanism on the logger being used to check if it's even enabled, all those append calls for something that might not actually be turned on seems less than ideal.  


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1592: NO-JIRA improve MQTT protocol logging

pgfox
In reply to this post by pgfox
Github user jbertram commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/1592#discussion_r145234463
 
    --- Diff: artemis-protocols/artemis-mqtt-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/mqtt/MQTTUtil.java ---
    @@ -149,16 +154,52 @@ public static void logMessage(MQTTSessionState state, MqttMessage message, boole
              if (message.fixedHeader() != null) {
                 log.append(message.fixedHeader().messageType().toString());
     
    -            if (message.variableHeader() instanceof MqttPublishVariableHeader) {
    -               log.append("(" + ((MqttPublishVariableHeader) message.variableHeader()).messageId() + ") " + message.fixedHeader().qosLevel());
    -            } else if (message.variableHeader() instanceof MqttMessageIdVariableHeader) {
    +            if (message.variableHeader() instanceof MqttMessageIdVariableHeader) {
                    log.append("(" + ((MqttMessageIdVariableHeader) message.variableHeader()).messageId() + ")");
                 }
     
    -            if (message.fixedHeader().messageType() == MqttMessageType.SUBSCRIBE) {
    -               for (MqttTopicSubscription sub : ((MqttSubscribeMessage) message).payload().topicSubscriptions()) {
    -                  log.append("\n\t" + sub.topicName() + " : " + sub.qualityOfService());
    -               }
    +            switch (message.fixedHeader().messageType()) {
    +               case PUBLISH:
    +                  MqttPublishVariableHeader publishHeader = (MqttPublishVariableHeader) message.variableHeader();
    +                  log.append("(" + publishHeader.packetId() + ")")
    +                     .append(" topic=" + publishHeader.topicName())
    +                     .append(", qos=" + message.fixedHeader().qosLevel())
    +                     .append(", retain=" + message.fixedHeader().isRetain())
    +                     .append(", dup=" + message.fixedHeader().isDup());
    +                     //.append(" payload=" + ((MqttPublishMessage)message).payload().toString(StandardCharsets.UTF_8));
    --- End diff --
   
    The method starts with:
   
    if (logger.isTraceEnabled())
   
    So none of this will be executed if TRACE (or equivalent) isn't turned on in the logging configuration.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1592: NO-JIRA improve MQTT protocol logging

pgfox
In reply to this post by pgfox
Github user ppatierno commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/1592#discussion_r145234734
 
    --- Diff: artemis-protocols/artemis-mqtt-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/mqtt/MQTTUtil.java ---
    @@ -149,16 +154,52 @@ public static void logMessage(MQTTSessionState state, MqttMessage message, boole
              if (message.fixedHeader() != null) {
                 log.append(message.fixedHeader().messageType().toString());
     
    -            if (message.variableHeader() instanceof MqttPublishVariableHeader) {
    -               log.append("(" + ((MqttPublishVariableHeader) message.variableHeader()).messageId() + ") " + message.fixedHeader().qosLevel());
    -            } else if (message.variableHeader() instanceof MqttMessageIdVariableHeader) {
    +            if (message.variableHeader() instanceof MqttMessageIdVariableHeader) {
                    log.append("(" + ((MqttMessageIdVariableHeader) message.variableHeader()).messageId() + ")");
                 }
     
    -            if (message.fixedHeader().messageType() == MqttMessageType.SUBSCRIBE) {
    -               for (MqttTopicSubscription sub : ((MqttSubscribeMessage) message).payload().topicSubscriptions()) {
    -                  log.append("\n\t" + sub.topicName() + " : " + sub.qualityOfService());
    -               }
    +            switch (message.fixedHeader().messageType()) {
    +               case PUBLISH:
    +                  MqttPublishVariableHeader publishHeader = (MqttPublishVariableHeader) message.variableHeader();
    +                  log.append("(" + publishHeader.packetId() + ")")
    +                     .append(" topic=" + publishHeader.topicName())
    +                     .append(", qos=" + message.fixedHeader().qosLevel())
    +                     .append(", retain=" + message.fixedHeader().isRetain())
    +                     .append(", dup=" + message.fixedHeader().isDup());
    +                     //.append(" payload=" + ((MqttPublishMessage)message).payload().toString(StandardCharsets.UTF_8));
    --- End diff --
   
    And there is no way for making one of them configurable as well ? For example printing the PUBLISH payload ?


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1592: NO-JIRA improve MQTT protocol logging

pgfox
In reply to this post by pgfox
Github user jbertram commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/1592#discussion_r145235145
 
    --- Diff: artemis-protocols/artemis-mqtt-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/mqtt/MQTTUtil.java ---
    @@ -149,16 +154,52 @@ public static void logMessage(MQTTSessionState state, MqttMessage message, boole
              if (message.fixedHeader() != null) {
                 log.append(message.fixedHeader().messageType().toString());
     
    -            if (message.variableHeader() instanceof MqttPublishVariableHeader) {
    -               log.append("(" + ((MqttPublishVariableHeader) message.variableHeader()).messageId() + ") " + message.fixedHeader().qosLevel());
    -            } else if (message.variableHeader() instanceof MqttMessageIdVariableHeader) {
    +            if (message.variableHeader() instanceof MqttMessageIdVariableHeader) {
                    log.append("(" + ((MqttMessageIdVariableHeader) message.variableHeader()).messageId() + ")");
                 }
     
    -            if (message.fixedHeader().messageType() == MqttMessageType.SUBSCRIBE) {
    -               for (MqttTopicSubscription sub : ((MqttSubscribeMessage) message).payload().topicSubscriptions()) {
    -                  log.append("\n\t" + sub.topicName() + " : " + sub.qualityOfService());
    -               }
    +            switch (message.fixedHeader().messageType()) {
    +               case PUBLISH:
    +                  MqttPublishVariableHeader publishHeader = (MqttPublishVariableHeader) message.variableHeader();
    +                  log.append("(" + publishHeader.packetId() + ")")
    +                     .append(" topic=" + publishHeader.topicName())
    +                     .append(", qos=" + message.fixedHeader().qosLevel())
    +                     .append(", retain=" + message.fixedHeader().isRetain())
    +                     .append(", dup=" + message.fixedHeader().isDup());
    +                     //.append(" payload=" + ((MqttPublishMessage)message).payload().toString(StandardCharsets.UTF_8));
    +                  break;
    +               case CONNECT:
    +                  MqttConnectVariableHeader connectHeader = (MqttConnectVariableHeader) message.variableHeader();
    +                  MqttConnectPayload payload = ((MqttConnectMessage)message).payload();
    +                  log.append(" name=").append(connectHeader.name())
    +                     .append(", version=").append(connectHeader.version())
    +                     .append(", hasUserName=").append(connectHeader.hasUserName())
    +                     .append(", hasPassword=").append(connectHeader.hasPassword())
    +                     .append(", isWillRetain=").append(connectHeader.isWillRetain())
    +                     .append(", isWillFlag=").append(connectHeader.isWillFlag())
    +                     .append(", isCleanSession=").append(connectHeader.isCleanSession())
    +                     .append(", keepAliveTimeSeconds=").append(connectHeader.keepAliveTimeSeconds())
    +                     .append(", clientIdentifier=").append(payload.clientIdentifier())
    +                     .append(", willTopic=").append(payload.willTopic())
    +                     .append(", willMessage=").append(payload.willMessage())
    +                     .append(", userName=").append(payload.userName());
    +                     //.append(", password=").append(payload.password());
    +                  break;
    +               case SUBSCRIBE:
    +                  for (MqttTopicSubscription sub : ((MqttSubscribeMessage) message).payload().topicSubscriptions()) {
    +                     log.append("\n\t" + sub.topicName() + " : " + sub.qualityOfService());
    --- End diff --
   
    Packet-ID is already logged above before the switch statement starts.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1592: NO-JIRA improve MQTT protocol logging

pgfox
In reply to this post by pgfox
Github user ppatierno commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/1592#discussion_r145235736
 
    --- Diff: artemis-protocols/artemis-mqtt-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/mqtt/MQTTUtil.java ---
    @@ -149,16 +154,52 @@ public static void logMessage(MQTTSessionState state, MqttMessage message, boole
              if (message.fixedHeader() != null) {
                 log.append(message.fixedHeader().messageType().toString());
     
    -            if (message.variableHeader() instanceof MqttPublishVariableHeader) {
    -               log.append("(" + ((MqttPublishVariableHeader) message.variableHeader()).messageId() + ") " + message.fixedHeader().qosLevel());
    -            } else if (message.variableHeader() instanceof MqttMessageIdVariableHeader) {
    +            if (message.variableHeader() instanceof MqttMessageIdVariableHeader) {
                    log.append("(" + ((MqttMessageIdVariableHeader) message.variableHeader()).messageId() + ")");
                 }
     
    -            if (message.fixedHeader().messageType() == MqttMessageType.SUBSCRIBE) {
    -               for (MqttTopicSubscription sub : ((MqttSubscribeMessage) message).payload().topicSubscriptions()) {
    -                  log.append("\n\t" + sub.topicName() + " : " + sub.qualityOfService());
    -               }
    +            switch (message.fixedHeader().messageType()) {
    +               case PUBLISH:
    +                  MqttPublishVariableHeader publishHeader = (MqttPublishVariableHeader) message.variableHeader();
    +                  log.append("(" + publishHeader.packetId() + ")")
    +                     .append(" topic=" + publishHeader.topicName())
    +                     .append(", qos=" + message.fixedHeader().qosLevel())
    +                     .append(", retain=" + message.fixedHeader().isRetain())
    +                     .append(", dup=" + message.fixedHeader().isDup());
    +                     //.append(" payload=" + ((MqttPublishMessage)message).payload().toString(StandardCharsets.UTF_8));
    +                  break;
    +               case CONNECT:
    +                  MqttConnectVariableHeader connectHeader = (MqttConnectVariableHeader) message.variableHeader();
    +                  MqttConnectPayload payload = ((MqttConnectMessage)message).payload();
    +                  log.append(" name=").append(connectHeader.name())
    +                     .append(", version=").append(connectHeader.version())
    +                     .append(", hasUserName=").append(connectHeader.hasUserName())
    +                     .append(", hasPassword=").append(connectHeader.hasPassword())
    +                     .append(", isWillRetain=").append(connectHeader.isWillRetain())
    +                     .append(", isWillFlag=").append(connectHeader.isWillFlag())
    +                     .append(", isCleanSession=").append(connectHeader.isCleanSession())
    +                     .append(", keepAliveTimeSeconds=").append(connectHeader.keepAliveTimeSeconds())
    +                     .append(", clientIdentifier=").append(payload.clientIdentifier())
    +                     .append(", willTopic=").append(payload.willTopic())
    +                     .append(", willMessage=").append(payload.willMessage())
    +                     .append(", userName=").append(payload.userName());
    +                     //.append(", password=").append(payload.password());
    +                  break;
    +               case SUBSCRIBE:
    +                  for (MqttTopicSubscription sub : ((MqttSubscribeMessage) message).payload().topicSubscriptions()) {
    +                     log.append("\n\t" + sub.topicName() + " : " + sub.qualityOfService());
    --- End diff --
   
    @jbertram sorry you are right !


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1592: NO-JIRA improve MQTT protocol logging

pgfox
In reply to this post by pgfox
Github user jbertram commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/1592#discussion_r145235737
 
    --- Diff: artemis-protocols/artemis-mqtt-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/mqtt/MQTTUtil.java ---
    @@ -149,16 +154,52 @@ public static void logMessage(MQTTSessionState state, MqttMessage message, boole
              if (message.fixedHeader() != null) {
                 log.append(message.fixedHeader().messageType().toString());
     
    -            if (message.variableHeader() instanceof MqttPublishVariableHeader) {
    -               log.append("(" + ((MqttPublishVariableHeader) message.variableHeader()).messageId() + ") " + message.fixedHeader().qosLevel());
    -            } else if (message.variableHeader() instanceof MqttMessageIdVariableHeader) {
    +            if (message.variableHeader() instanceof MqttMessageIdVariableHeader) {
                    log.append("(" + ((MqttMessageIdVariableHeader) message.variableHeader()).messageId() + ")");
                 }
     
    -            if (message.fixedHeader().messageType() == MqttMessageType.SUBSCRIBE) {
    -               for (MqttTopicSubscription sub : ((MqttSubscribeMessage) message).payload().topicSubscriptions()) {
    -                  log.append("\n\t" + sub.topicName() + " : " + sub.qualityOfService());
    -               }
    +            switch (message.fixedHeader().messageType()) {
    +               case PUBLISH:
    +                  MqttPublishVariableHeader publishHeader = (MqttPublishVariableHeader) message.variableHeader();
    +                  log.append("(" + publishHeader.packetId() + ")")
    +                     .append(" topic=" + publishHeader.topicName())
    +                     .append(", qos=" + message.fixedHeader().qosLevel())
    +                     .append(", retain=" + message.fixedHeader().isRetain())
    +                     .append(", dup=" + message.fixedHeader().isDup());
    +                     //.append(" payload=" + ((MqttPublishMessage)message).payload().toString(StandardCharsets.UTF_8));
    +                  break;
    +               case CONNECT:
    +                  MqttConnectVariableHeader connectHeader = (MqttConnectVariableHeader) message.variableHeader();
    +                  MqttConnectPayload payload = ((MqttConnectMessage)message).payload();
    +                  log.append(" name=").append(connectHeader.name())
    +                     .append(", version=").append(connectHeader.version())
    +                     .append(", hasUserName=").append(connectHeader.hasUserName())
    +                     .append(", hasPassword=").append(connectHeader.hasPassword())
    +                     .append(", isWillRetain=").append(connectHeader.isWillRetain())
    +                     .append(", isWillFlag=").append(connectHeader.isWillFlag())
    +                     .append(", isCleanSession=").append(connectHeader.isCleanSession())
    +                     .append(", keepAliveTimeSeconds=").append(connectHeader.keepAliveTimeSeconds())
    +                     .append(", clientIdentifier=").append(payload.clientIdentifier())
    +                     .append(", willTopic=").append(payload.willTopic())
    +                     .append(", willMessage=").append(payload.willMessage())
    +                     .append(", userName=").append(payload.userName());
    +                     //.append(", password=").append(payload.password());
    +                  break;
    +               case SUBSCRIBE:
    +                  for (MqttTopicSubscription sub : ((MqttSubscribeMessage) message).payload().topicSubscriptions()) {
    +                     log.append("\n\t" + sub.topicName() + " : " + sub.qualityOfService());
    +                  }
    +                  break;
    +               case SUBACK:
    +                  for (Integer qos : ((MqttSubAckMessage) message).payload().grantedQoSLevels()) {
    +                     log.append("\n\t" + qos);
    --- End diff --
   
    Packet-ID is already logged above before the switch statement starts.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1592: NO-JIRA improve MQTT protocol logging

pgfox
In reply to this post by pgfox
Github user jbertram commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/1592#discussion_r145235758
 
    --- Diff: artemis-protocols/artemis-mqtt-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/mqtt/MQTTUtil.java ---
    @@ -149,16 +154,52 @@ public static void logMessage(MQTTSessionState state, MqttMessage message, boole
              if (message.fixedHeader() != null) {
                 log.append(message.fixedHeader().messageType().toString());
     
    -            if (message.variableHeader() instanceof MqttPublishVariableHeader) {
    -               log.append("(" + ((MqttPublishVariableHeader) message.variableHeader()).messageId() + ") " + message.fixedHeader().qosLevel());
    -            } else if (message.variableHeader() instanceof MqttMessageIdVariableHeader) {
    +            if (message.variableHeader() instanceof MqttMessageIdVariableHeader) {
                    log.append("(" + ((MqttMessageIdVariableHeader) message.variableHeader()).messageId() + ")");
                 }
     
    -            if (message.fixedHeader().messageType() == MqttMessageType.SUBSCRIBE) {
    -               for (MqttTopicSubscription sub : ((MqttSubscribeMessage) message).payload().topicSubscriptions()) {
    -                  log.append("\n\t" + sub.topicName() + " : " + sub.qualityOfService());
    -               }
    +            switch (message.fixedHeader().messageType()) {
    +               case PUBLISH:
    +                  MqttPublishVariableHeader publishHeader = (MqttPublishVariableHeader) message.variableHeader();
    +                  log.append("(" + publishHeader.packetId() + ")")
    +                     .append(" topic=" + publishHeader.topicName())
    +                     .append(", qos=" + message.fixedHeader().qosLevel())
    +                     .append(", retain=" + message.fixedHeader().isRetain())
    +                     .append(", dup=" + message.fixedHeader().isDup());
    +                     //.append(" payload=" + ((MqttPublishMessage)message).payload().toString(StandardCharsets.UTF_8));
    +                  break;
    +               case CONNECT:
    +                  MqttConnectVariableHeader connectHeader = (MqttConnectVariableHeader) message.variableHeader();
    +                  MqttConnectPayload payload = ((MqttConnectMessage)message).payload();
    +                  log.append(" name=").append(connectHeader.name())
    +                     .append(", version=").append(connectHeader.version())
    +                     .append(", hasUserName=").append(connectHeader.hasUserName())
    +                     .append(", hasPassword=").append(connectHeader.hasPassword())
    +                     .append(", isWillRetain=").append(connectHeader.isWillRetain())
    +                     .append(", isWillFlag=").append(connectHeader.isWillFlag())
    +                     .append(", isCleanSession=").append(connectHeader.isCleanSession())
    +                     .append(", keepAliveTimeSeconds=").append(connectHeader.keepAliveTimeSeconds())
    +                     .append(", clientIdentifier=").append(payload.clientIdentifier())
    +                     .append(", willTopic=").append(payload.willTopic())
    +                     .append(", willMessage=").append(payload.willMessage())
    +                     .append(", userName=").append(payload.userName());
    +                     //.append(", password=").append(payload.password());
    +                  break;
    +               case SUBSCRIBE:
    +                  for (MqttTopicSubscription sub : ((MqttSubscribeMessage) message).payload().topicSubscriptions()) {
    +                     log.append("\n\t" + sub.topicName() + " : " + sub.qualityOfService());
    +                  }
    +                  break;
    +               case SUBACK:
    +                  for (Integer qos : ((MqttSubAckMessage) message).payload().grantedQoSLevels()) {
    +                     log.append("\n\t" + qos);
    +                  }
    +                  break;
    +               case UNSUBSCRIBE:
    +                  for (String topic : ((MqttUnsubscribeMessage) message).payload().topics()) {
    +                     log.append("\n\t" + topic);
    --- End diff --
   
    Packet-ID is already logged above before the switch statement starts.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1592: NO-JIRA improve MQTT protocol logging

pgfox
In reply to this post by pgfox
Github user ppatierno commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/1592#discussion_r145235939
 
    --- Diff: artemis-protocols/artemis-mqtt-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/mqtt/MQTTUtil.java ---
    @@ -149,16 +154,52 @@ public static void logMessage(MQTTSessionState state, MqttMessage message, boole
              if (message.fixedHeader() != null) {
                 log.append(message.fixedHeader().messageType().toString());
     
    -            if (message.variableHeader() instanceof MqttPublishVariableHeader) {
    -               log.append("(" + ((MqttPublishVariableHeader) message.variableHeader()).messageId() + ") " + message.fixedHeader().qosLevel());
    -            } else if (message.variableHeader() instanceof MqttMessageIdVariableHeader) {
    +            if (message.variableHeader() instanceof MqttMessageIdVariableHeader) {
                    log.append("(" + ((MqttMessageIdVariableHeader) message.variableHeader()).messageId() + ")");
                 }
     
    -            if (message.fixedHeader().messageType() == MqttMessageType.SUBSCRIBE) {
    -               for (MqttTopicSubscription sub : ((MqttSubscribeMessage) message).payload().topicSubscriptions()) {
    -                  log.append("\n\t" + sub.topicName() + " : " + sub.qualityOfService());
    -               }
    +            switch (message.fixedHeader().messageType()) {
    +               case PUBLISH:
    +                  MqttPublishVariableHeader publishHeader = (MqttPublishVariableHeader) message.variableHeader();
    +                  log.append("(" + publishHeader.packetId() + ")")
    +                     .append(" topic=" + publishHeader.topicName())
    +                     .append(", qos=" + message.fixedHeader().qosLevel())
    +                     .append(", retain=" + message.fixedHeader().isRetain())
    +                     .append(", dup=" + message.fixedHeader().isDup());
    +                     //.append(" payload=" + ((MqttPublishMessage)message).payload().toString(StandardCharsets.UTF_8));
    +                  break;
    +               case CONNECT:
    +                  MqttConnectVariableHeader connectHeader = (MqttConnectVariableHeader) message.variableHeader();
    +                  MqttConnectPayload payload = ((MqttConnectMessage)message).payload();
    +                  log.append(" name=").append(connectHeader.name())
    +                     .append(", version=").append(connectHeader.version())
    +                     .append(", hasUserName=").append(connectHeader.hasUserName())
    +                     .append(", hasPassword=").append(connectHeader.hasPassword())
    +                     .append(", isWillRetain=").append(connectHeader.isWillRetain())
    +                     .append(", isWillFlag=").append(connectHeader.isWillFlag())
    +                     .append(", isCleanSession=").append(connectHeader.isCleanSession())
    +                     .append(", keepAliveTimeSeconds=").append(connectHeader.keepAliveTimeSeconds())
    +                     .append(", clientIdentifier=").append(payload.clientIdentifier())
    +                     .append(", willTopic=").append(payload.willTopic())
    +                     .append(", willMessage=").append(payload.willMessage())
    +                     .append(", userName=").append(payload.userName());
    +                     //.append(", password=").append(payload.password());
    +                  break;
    +               case SUBSCRIBE:
    +                  for (MqttTopicSubscription sub : ((MqttSubscribeMessage) message).payload().topicSubscriptions()) {
    +                     log.append("\n\t" + sub.topicName() + " : " + sub.qualityOfService());
    +                  }
    +                  break;
    +               case SUBACK:
    +                  for (Integer qos : ((MqttSubAckMessage) message).payload().grantedQoSLevels()) {
    +                     log.append("\n\t" + qos);
    --- End diff --
   
    As above you are right sorry !


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1592: NO-JIRA improve MQTT protocol logging

pgfox
In reply to this post by pgfox
Github user ppatierno commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/1592#discussion_r145235982
 
    --- Diff: artemis-protocols/artemis-mqtt-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/mqtt/MQTTUtil.java ---
    @@ -149,16 +154,52 @@ public static void logMessage(MQTTSessionState state, MqttMessage message, boole
              if (message.fixedHeader() != null) {
                 log.append(message.fixedHeader().messageType().toString());
     
    -            if (message.variableHeader() instanceof MqttPublishVariableHeader) {
    -               log.append("(" + ((MqttPublishVariableHeader) message.variableHeader()).messageId() + ") " + message.fixedHeader().qosLevel());
    -            } else if (message.variableHeader() instanceof MqttMessageIdVariableHeader) {
    +            if (message.variableHeader() instanceof MqttMessageIdVariableHeader) {
                    log.append("(" + ((MqttMessageIdVariableHeader) message.variableHeader()).messageId() + ")");
                 }
     
    -            if (message.fixedHeader().messageType() == MqttMessageType.SUBSCRIBE) {
    -               for (MqttTopicSubscription sub : ((MqttSubscribeMessage) message).payload().topicSubscriptions()) {
    -                  log.append("\n\t" + sub.topicName() + " : " + sub.qualityOfService());
    -               }
    +            switch (message.fixedHeader().messageType()) {
    +               case PUBLISH:
    +                  MqttPublishVariableHeader publishHeader = (MqttPublishVariableHeader) message.variableHeader();
    +                  log.append("(" + publishHeader.packetId() + ")")
    +                     .append(" topic=" + publishHeader.topicName())
    +                     .append(", qos=" + message.fixedHeader().qosLevel())
    +                     .append(", retain=" + message.fixedHeader().isRetain())
    +                     .append(", dup=" + message.fixedHeader().isDup());
    +                     //.append(" payload=" + ((MqttPublishMessage)message).payload().toString(StandardCharsets.UTF_8));
    +                  break;
    +               case CONNECT:
    +                  MqttConnectVariableHeader connectHeader = (MqttConnectVariableHeader) message.variableHeader();
    +                  MqttConnectPayload payload = ((MqttConnectMessage)message).payload();
    +                  log.append(" name=").append(connectHeader.name())
    +                     .append(", version=").append(connectHeader.version())
    +                     .append(", hasUserName=").append(connectHeader.hasUserName())
    +                     .append(", hasPassword=").append(connectHeader.hasPassword())
    +                     .append(", isWillRetain=").append(connectHeader.isWillRetain())
    +                     .append(", isWillFlag=").append(connectHeader.isWillFlag())
    +                     .append(", isCleanSession=").append(connectHeader.isCleanSession())
    +                     .append(", keepAliveTimeSeconds=").append(connectHeader.keepAliveTimeSeconds())
    +                     .append(", clientIdentifier=").append(payload.clientIdentifier())
    +                     .append(", willTopic=").append(payload.willTopic())
    +                     .append(", willMessage=").append(payload.willMessage())
    +                     .append(", userName=").append(payload.userName());
    +                     //.append(", password=").append(payload.password());
    +                  break;
    +               case SUBSCRIBE:
    +                  for (MqttTopicSubscription sub : ((MqttSubscribeMessage) message).payload().topicSubscriptions()) {
    +                     log.append("\n\t" + sub.topicName() + " : " + sub.qualityOfService());
    +                  }
    +                  break;
    +               case SUBACK:
    +                  for (Integer qos : ((MqttSubAckMessage) message).payload().grantedQoSLevels()) {
    +                     log.append("\n\t" + qos);
    +                  }
    +                  break;
    +               case UNSUBSCRIBE:
    +                  for (String topic : ((MqttUnsubscribeMessage) message).payload().topics()) {
    +                     log.append("\n\t" + topic);
    --- End diff --
   
    As above you are right sorry !


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1592: NO-JIRA improve MQTT protocol logging

pgfox
In reply to this post by pgfox
Github user jbertram commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/1592#discussion_r145238929
 
    --- Diff: artemis-protocols/artemis-mqtt-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/mqtt/MQTTUtil.java ---
    @@ -149,16 +154,52 @@ public static void logMessage(MQTTSessionState state, MqttMessage message, boole
              if (message.fixedHeader() != null) {
                 log.append(message.fixedHeader().messageType().toString());
     
    -            if (message.variableHeader() instanceof MqttPublishVariableHeader) {
    -               log.append("(" + ((MqttPublishVariableHeader) message.variableHeader()).messageId() + ") " + message.fixedHeader().qosLevel());
    -            } else if (message.variableHeader() instanceof MqttMessageIdVariableHeader) {
    +            if (message.variableHeader() instanceof MqttMessageIdVariableHeader) {
                    log.append("(" + ((MqttMessageIdVariableHeader) message.variableHeader()).messageId() + ")");
                 }
     
    -            if (message.fixedHeader().messageType() == MqttMessageType.SUBSCRIBE) {
    -               for (MqttTopicSubscription sub : ((MqttSubscribeMessage) message).payload().topicSubscriptions()) {
    -                  log.append("\n\t" + sub.topicName() + " : " + sub.qualityOfService());
    -               }
    +            switch (message.fixedHeader().messageType()) {
    +               case PUBLISH:
    +                  MqttPublishVariableHeader publishHeader = (MqttPublishVariableHeader) message.variableHeader();
    +                  log.append("(" + publishHeader.packetId() + ")")
    +                     .append(" topic=" + publishHeader.topicName())
    +                     .append(", qos=" + message.fixedHeader().qosLevel())
    +                     .append(", retain=" + message.fixedHeader().isRetain())
    +                     .append(", dup=" + message.fixedHeader().isDup());
    +                     //.append(" payload=" + ((MqttPublishMessage)message).payload().toString(StandardCharsets.UTF_8));
    --- End diff --
   
    My only thought here is to use something really basic like a system property or something.  I don't think this is worth changing the configuration schema to support a new option in broker.xml.  


---
12