Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ARTEMIS-1472 - PN_TRACE_FRM as System.out logging #1601

Closed
wants to merge 2 commits into from
Closed

ARTEMIS-1472 - PN_TRACE_FRM as System.out logging #1601

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
92cefc8
ARTEMIS-1472 - PN_TRACE_FRM as System.out logging
clebertsuconic Oct 19, 2017
a905a80
NO-JIRA: removing out dated TODO
clebertsuconic Oct 20, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 0 additions & 18 deletions artemis-commons/src/main/java/org/apache/activemq/artemis/utils/ByteUtil.java
View file
Open in desktop
Expand Up @@ -25,7 +25,6 @@
import org.apache.activemq.artemis.api.core.ActiveMQBuffer;
import org.apache.activemq.artemis.api.core.SimpleString;
import org.apache.activemq.artemis.logs.ActiveMQUtilBundle;
import org.jboss.logging.Logger;

public class ByteUtil {

Expand All @@ -39,23 +38,6 @@ public class ByteUtil {
private static final Pattern MEGA = Pattern.compile(prefix + "m" + suffix, Pattern.CASE_INSENSITIVE);
private static final Pattern GIGA = Pattern.compile(prefix + "g" + suffix, Pattern.CASE_INSENSITIVE);

public static void debugFrame(Logger logger, String message, ByteBuf byteIn) {
if (logger.isTraceEnabled()) {
int location = byteIn.readerIndex();
// debugging
byte[] frame = new byte[byteIn.writerIndex()];
byteIn.readBytes(frame);

try {
logger.trace(message + "\n" + ByteUtil.formatGroup(ByteUtil.bytesToHex(frame), 8, 16));
} catch (Exception e) {
logger.warn(e.getMessage(), e);
}

byteIn.readerIndex(location);
}
}

public static String formatGroup(String str, int groupSize, int lineBreak) {
StringBuffer buffer = new StringBuffer();

Expand Down
8 changes: 8 additions & 0 deletions ...src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/ProtonProtocolManager.java
View file
Open in desktop
Expand Up @@ -43,14 +43,22 @@
import org.apache.activemq.artemis.spi.core.protocol.RemotingConnection;
import org.apache.activemq.artemis.spi.core.remoting.Acceptor;
import org.apache.activemq.artemis.spi.core.remoting.Connection;
import org.jboss.logging.Logger;

/**
* A proton protocol manager, basically reads the Proton Input and maps proton resources to ActiveMQ Artemis resources
*/
public class ProtonProtocolManager extends AbstractProtocolManager<AMQPMessage, AmqpInterceptor, ActiveMQProtonRemotingConnection> implements NotificationListener {

private static final Logger logger = Logger.getLogger(ProtonProtocolManager.class);
private static final List<String> websocketRegistryNames = Arrays.asList("amqp");

/**
* This property has been used by Proton to emit some debug data on System.out.
* if you set it to anything other than NULL it will be used on logging.info.
* */
public static final boolean PN_TRACE_FRM = System.getProperty("PN_TRACE_FRM") != null;

private final List<AmqpInterceptor> incomingInterceptors = new ArrayList<>();
private final List<AmqpInterceptor> outgoingInterceptors = new ArrayList<>();

Expand Down
18 changes: 16 additions & 2 deletions ...src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/AMQPConnectionContext.java
View file
Open in desktop
Expand Up @@ -57,6 +57,7 @@
import static org.apache.activemq.artemis.protocol.amqp.proton.AmqpSupport.NETWORK_HOST;
import static org.apache.activemq.artemis.protocol.amqp.proton.AmqpSupport.PORT;
import static org.apache.activemq.artemis.protocol.amqp.proton.AmqpSupport.SCHEME;
import static org.apache.activemq.artemis.protocol.amqp.broker.ProtonProtocolManager.PN_TRACE_FRM;

public class AMQPConnectionContext extends ProtonInitializable implements EventHandler {

Expand Down Expand Up @@ -143,13 +144,26 @@ public SASLResult getSASLResult() {
}

public void inputBuffer(ByteBuf buffer) {
if (log.isTraceEnabled()) {
ByteUtil.debugFrame(log, "Buffer Received ", buffer);
if (PN_TRACE_FRM) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't personally tie these things together, to me they are separate and just because I am asking proton to trace frames doesn't mean I want all the bytes logged. I do the former often enough, but only very rarely do the latter.

When I do the latter its sometimes by using config toggle to activate adding Nettys own byte logging debug LoggingHandler, though e.g for Qpid JMS we do also have specific toggles to activate something more like this.

debugFrame(buffer);
}

handler.inputBuffer(buffer);
}

private static void debugFrame(ByteBuf byteIn) {
int location = byteIn.readerIndex();
// debugging
byte[] frame = new byte[byteIn.writerIndex()];
byteIn.readBytes(frame);

if (PN_TRACE_FRM) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was checked already before calling the method. Ignoring that it would probably also be better done around the whole contents of the method to avoid creating the byte array but then not using it and then needing to readjust the buffer index later.

log.info("Received frame \n" + ByteUtil.formatGroup(ByteUtil.bytesToHex(frame), 8, 16));
}
byteIn.readerIndex(location);
}


public void destroy() {
connectionCallback.close();
}
Expand Down
8 changes: 8 additions & 0 deletions ...otocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/handler/Events.java
View file
Open in desktop
Expand Up @@ -17,9 +17,14 @@
package org.apache.activemq.artemis.protocol.amqp.proton.handler;

import org.apache.qpid.proton.engine.Event;
import org.jboss.logging.Logger;

import static org.apache.activemq.artemis.protocol.amqp.broker.ProtonProtocolManager.PN_TRACE_FRM;

public final class Events {

private static final Logger logger = Logger.getLogger(Events.class);

public static void dispatch(Event event, EventHandler handler) throws Exception {
switch (event.getType()) {
case CONNECTION_INIT:
Expand Down Expand Up @@ -92,6 +97,9 @@ public static void dispatch(Event event, EventHandler handler) throws Exception
handler.onDelivery(event.getDelivery());
break;
default:
if (PN_TRACE_FRM) {
logger.info("event: " + event + " not being treated");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also doesn't feel like it should be tied to having proton trace its frames to me, I'd just make this a regular trace logger.

}
break;
}
}
Expand Down
6 changes: 5 additions & 1 deletion ...src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/handler/ProtonHandler.java
View file
Open in desktop
Expand Up @@ -47,6 +47,8 @@
import io.netty.buffer.ByteBuf;
import io.netty.buffer.PooledByteBufAllocator;

import static org.apache.activemq.artemis.protocol.amqp.broker.ProtonProtocolManager.PN_TRACE_FRM;

public class ProtonHandler extends ProtonInitializable {

private static final Logger log = Logger.getLogger(ProtonHandler.class);
Expand Down Expand Up @@ -443,7 +445,9 @@ private void dispatch() {
inDispatch = true;
while ((ev = collector.peek()) != null) {
for (EventHandler h : handlers) {
if (log.isTraceEnabled()) {
if (PN_TRACE_FRM) {
log.info("Handling " + ev + " towards " + h);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also doesn't feel like it should be tied to having proton trace its frames to me. Given the existing trace logging I wouldn't do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is often what I need when debugging AMQP.. I wanted to have an easy easy to turn on and debug at console without having to turn trace for everything (It's a bit diefficult to do individually).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not saying you don't need it, or that they often wont be combined eventually, but I rarely ever want to turn that logging on at precisely the same time as I might turn the frame logging on. They don't even go to the same place, so I really feel they are separate and should have their own control toggles as they do currently. I also wouldn't generally use another projects magic toggle to control behaviour in this one.

} else if (log.isTraceEnabled()) {
log.trace("Handling " + ev + " towards " + h);
}
try {
Expand Down
9 changes: 0 additions & 9 deletions ...-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ServerSessionImpl.java
View file
Open in desktop
Expand Up @@ -1678,15 +1678,6 @@ public synchronized RoutingStatus doSend(final Transaction tx,

RoutingType routingType = msg.getRoutingType();

/* TODO-now: How to address here with AMQP?
if (originalAddress != null) {
if (originalAddress.toString().startsWith("anycast:")) {
routingType = RoutingType.ANYCAST;
} else if (originalAddress.toString().startsWith("multicast:")) {
routingType = RoutingType.MULTICAST;
}
} */

Pair<SimpleString, RoutingType> art = getAddressAndRoutingType(msg.getAddressSimpleString(), routingType);

// Consumer
Expand Down