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

[asfgit]

Github user asfgit closed the pull request at:

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


---

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

[michaelandrepearce]

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

https://github.com/apache/activemq-artemis/pull/1592#discussion_r145683623
  
--- Diff: 
artemis-protocols/artemis-mqtt-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/mqtt/MQTTUtil.java
 ---
@@ -133,37 +142,84 @@ public static Message createPubRelMessage(MQTTSession 
session, SimpleString addr
 
public static void logMessage(MQTTSessionState state, MqttMessage 
message, boolean inbound) {
   if (logger.isTraceEnabled()) {
+ traceMessage(state, message, inbound);
--- End diff --

nice +1


---

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

[ppatierno]

Github user ppatierno commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/1592#discussion_r145441606
  
--- Diff: 
artemis-protocols/artemis-mqtt-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/mqtt/MQTTUtil.java
 ---
@@ -67,6 +76,8 @@
 
public static final int DEFAULT_KEEP_ALIVE_FREQUENCY = 5000;
 
+   public static final String logUTF8PublishPayload = 
System.getProperty("org.apache.activemq.artemis.core.protocol.mqtt.logUTF8PublishPayload");
--- End diff --

So for people who need to have the entire payload because it's in a 
reasonable range size, they need to use Wireshark ?


---

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

[tabish121]

Github user tabish121 commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/1592#discussion_r145438163
  
--- Diff: 
artemis-protocols/artemis-mqtt-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/mqtt/MQTTUtil.java
 ---
@@ -67,6 +76,8 @@
 
public static final int DEFAULT_KEEP_ALIVE_FREQUENCY = 5000;
 
+   public static final String logUTF8PublishPayload = 
System.getProperty("org.apache.activemq.artemis.core.protocol.mqtt.logUTF8PublishPayload");
--- End diff --

We do that in 5.x for things like logs on STOMP messages where you don't 
want to dump the full content portion of the frame but a small bit can help, I 
can't recall what the default is but something like 60 chars or the like is 
used.  You end up with a content string of "ABCDZYZ"


---

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

[jbertram]

Github user jbertram commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/1592#discussion_r145437240
  
--- Diff: 
artemis-protocols/artemis-mqtt-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/mqtt/MQTTUtil.java
 ---
@@ -67,6 +76,8 @@
 
public static final int DEFAULT_KEEP_ALIVE_FREQUENCY = 5000;
 
+   public static final String logUTF8PublishPayload = 
System.getProperty("org.apache.activemq.artemis.core.protocol.mqtt.logUTF8PublishPayload");
--- End diff --

To keep this as simple as possible I'll get rid of the system property and 
always log the payload as UTF-8, but also always limit the output to a certain 
number of characters so we don't get hundreds of megs of data (potentially).


---

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

[ppatierno]

Github user ppatierno commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/1592#discussion_r145434452
  
--- Diff: 
artemis-protocols/artemis-mqtt-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/mqtt/MQTTUtil.java
 ---
@@ -67,6 +76,8 @@
 
public static final int DEFAULT_KEEP_ALIVE_FREQUENCY = 5000;
 
+   public static final String logUTF8PublishPayload = 
System.getProperty("org.apache.activemq.artemis.core.protocol.mqtt.logUTF8PublishPayload");
--- End diff --

My 2 cents. The PUBLISH payload has a maximum size of 256 MB. I could have 
a use case where I need tracing packets but not printing 256 MB of bytes on the 
console. Use case where could be a problem in the messages flow that is not 
dependent on the payload. In this moment using just TRACE enabled or not it's 
just about having or not the logging. Maybe we could use DEBUG for normal 
packets logging and then TRACE for printing the payload as well.


---

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

[clebertsuconic]

Github user clebertsuconic commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/1592#discussion_r145407743
  
--- Diff: 
artemis-protocols/artemis-mqtt-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/mqtt/MQTTUtil.java
 ---
@@ -67,6 +76,8 @@
 
public static final int DEFAULT_KEEP_ALIVE_FREQUENCY = 5000;
 
+   public static final String logUTF8PublishPayload = 
System.getProperty("org.apache.activemq.artemis.core.protocol.mqtt.logUTF8PublishPayload");
--- End diff --

this is about tracing.. why don't you just always log the UTF?
if you don't need that level of information, just take trace out.


of you could do something like.. have this whole thing at debug... and if 
trace, throw the body on log.trace.

The system property is a bit odd here. 


---

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

[clebertsuconic]

Github user clebertsuconic commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/1592#discussion_r145407343
  
--- Diff: 
artemis-protocols/artemis-mqtt-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/mqtt/MQTTUtil.java
 ---
@@ -149,16 +160,62 @@ 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());
+  if (logUTF8PublishPayload != null) {
+ log.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())
--- End diff --

@jbertram I would create the logMessage @franz1981 is suggesting, but my 
concern here is not much about perf (since this method is already only called 
upon logging).. but out of making the code concise. Just a suggestion though.


---

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

[franz1981]

Github user franz1981 commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/1592#discussion_r145358049
  
--- Diff: 
artemis-protocols/artemis-mqtt-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/mqtt/MQTTUtil.java
 ---
@@ -149,16 +160,62 @@ 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());
+  if (logUTF8PublishPayload != null) {
+ log.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())
--- End diff --

@jbertram In order to avoid the JVM to compile `logMessage` with a big 
method (killing futher optimisations of the caller) I suggest to move the log 
message building it in a separate method (eg `private static void 
traceMessage(...)`) and use a pattern of calls like this:
```
public static void logMessage(MQTTSessionState state, MqttMessage 
message, boolean inbound) {
   if (logger.isTraceEnabled()) {
  traceMessage(state, message, inbound);
   }
}
```
This will let the JVM inline the `logMessage` call when is not needed.


---

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

[ppatierno]

Github user ppatierno commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/1592#discussion_r145321323
  
--- Diff: 
artemis-protocols/artemis-mqtt-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/mqtt/MQTTUtil.java
 ---
@@ -149,16 +160,62 @@ 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());
+  if (logUTF8PublishPayload != null) {
+ log.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())
--- End diff --

name and version are related to the protocol (i.e. "MQTT" and protocol 
level which is 4 for 3.1.1). @jbertram what do you think to log something like, 
protocol=(name, version) ? without having two distinct fields ?


---

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

[jbertram]

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.  


---

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

[ppatierno]

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 !


---

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

[ppatierno]

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 !


---

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

[jbertram]

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.


---

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

[jbertram]

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.


---

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

[ppatierno]

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 !


---

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

[jbertram]

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.


---

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

[ppatierno]

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 ?


---

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

[jbertram]

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.


---

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

[tabish121]

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.  


---

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

[ppatierno]

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.


---

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

[jbertram]

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.


---

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

[ppatierno]

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


---

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

[ppatierno]

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


---

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

[ppatierno]

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


---

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

[jbertram]

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.


---

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

[michaelandrepearce]

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."


---

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

[michaelandrepearce]

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.


---

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

[jbertram]

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 
Date:   2017-10-17T19:03:08Z

NO-JIRA improve MQTT protocol logging




---