ARTEMIS-1472 - PN_TRACE_FRM as System.out logging #1601
Conversation
|
@gemmellr ^^^ |
AMQP developers will usually set PN_TRACE_FRM as a System.property to get out some information from Proton regarding the frame processing. It would be a nice idea to also throw some log.info when that property is set at Artemis.. around AMQP Processing, making it easier to Debug AMQP packets.
| byte[] frame = new byte[byteIn.writerIndex()]; | ||
| byteIn.readBytes(frame); | ||
|
|
||
| if (PN_TRACE_FRM) { |
There was a problem hiding this comment.
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.
| @@ -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"); | |||
There was a problem hiding this comment.
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.
| @@ -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); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
| @@ -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) { | |||
There was a problem hiding this comment.
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.
|
I wouldn't tie together all the bits into this one env flag, most often logging the binary stuff adds nothing other than noise in all but some very specific cases. To be honest activating logging based on env vars seems weird in a modern Java app where we have fancy things like loggers that can be configured in config files specific to the running application. |
|
@tabish121 I will close this PR... The only reason I did it, was because someone was doing debug with PN_TRACE, and I thought it would make sense to have extra context... this is gone thanks! |
|
Not trying to criticize really, just seems like we should try and make it easier in a way that leverages the logging and logging config that way you could control easier the level of granularity on what you are getting and narrow it down when trying to find specific issues. While the 5.x stuff is far from perfect it does use a separate logger for frames which you can toggle on or off in the log4j.properties file to help you filter AMQP code debugger data from the frame tracing. |
|
@tabish121 you're right.. I will think of something like that... will reopen the JIRA.. and revisit this with another PR. |
AMQP developers will usually set PN_TRACE_FRM as a System.property to get out some information from Proton regarding the frame processing.
It would be a nice idea to also throw some log.info when that property is set at Artemis.. around AMQP Processing, making it easier to Debug AMQP packets.