[GitHub] activemq-artemis pull request #1601: ARTEMIS-1472 - PN_TRACE_FRM as System.o...

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

[GitHub] activemq-artemis pull request #1601: ARTEMIS-1472 - PN_TRACE_FRM as System.o...

clebertsuconic-3
GitHub user clebertsuconic opened a pull request:

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

    ARTEMIS-1472 - PN_TRACE_FRM as System.out logging

    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.

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

    $ git pull https://github.com/clebertsuconic/activemq-artemis debug

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

    https://github.com/apache/activemq-artemis/pull/1601.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 #1601
   
----
commit c8be28bc7d7d84cfdee464591f12d7bf80104189
Author: Clebert Suconic <[hidden email]>
Date:   2017-10-19T23:58:06Z

    ARTEMIS-1472 - PN_TRACE_FRM as System.out logging
   
    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.

----


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

[GitHub] activemq-artemis issue #1601: ARTEMIS-1472 - PN_TRACE_FRM as System.out logg...

clebertsuconic-3
Github user clebertsuconic commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1601
 
    @gemmellr ^^^


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

[GitHub] activemq-artemis pull request #1601: ARTEMIS-1472 - PN_TRACE_FRM as System.o...

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user gemmellr commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/1601#discussion_r145906703
 
    --- Diff: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/AMQPConnectionContext.java ---
    @@ -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) {
    +         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) {
    --- End diff --
   
    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.


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

[GitHub] activemq-artemis pull request #1601: ARTEMIS-1472 - PN_TRACE_FRM as System.o...

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user gemmellr commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/1601#discussion_r145907083
 
    --- Diff: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/handler/ProtonHandler.java ---
    @@ -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);
    --- End diff --
   
    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.


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

[GitHub] activemq-artemis pull request #1601: ARTEMIS-1472 - PN_TRACE_FRM as System.o...

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user gemmellr commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/1601#discussion_r145906975
 
    --- Diff: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/handler/Events.java ---
    @@ -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");
    --- End diff --
   
    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.


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

[GitHub] activemq-artemis pull request #1601: ARTEMIS-1472 - PN_TRACE_FRM as System.o...

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user gemmellr commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/1601#discussion_r145910546
 
    --- Diff: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/AMQPConnectionContext.java ---
    @@ -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) {
    --- End diff --
   
    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.


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

[GitHub] activemq-artemis pull request #1601: ARTEMIS-1472 - PN_TRACE_FRM as System.o...

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user clebertsuconic commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/1601#discussion_r145949482
 
    --- Diff: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/handler/ProtonHandler.java ---
    @@ -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);
    --- End diff --
   
    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).


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

[GitHub] activemq-artemis issue #1601: ARTEMIS-1472 - PN_TRACE_FRM as System.out logg...

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user tabish121 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1601
 
    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.  


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

[GitHub] activemq-artemis issue #1601: ARTEMIS-1472 - PN_TRACE_FRM as System.out logg...

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user clebertsuconic commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1601
 
    @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!


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

[GitHub] activemq-artemis pull request #1601: ARTEMIS-1472 - PN_TRACE_FRM as System.o...

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user clebertsuconic closed the pull request at:

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


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

[GitHub] activemq-artemis pull request #1601: ARTEMIS-1472 - PN_TRACE_FRM as System.o...

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user gemmellr commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/1601#discussion_r145959626
 
    --- Diff: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/handler/ProtonHandler.java ---
    @@ -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);
    --- End diff --
   
    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.


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

[GitHub] activemq-artemis issue #1601: ARTEMIS-1472 - PN_TRACE_FRM as System.out logg...

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user tabish121 commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1601
 
    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.  


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

[GitHub] activemq-artemis issue #1601: ARTEMIS-1472 - PN_TRACE_FRM as System.out logg...

clebertsuconic-3
In reply to this post by clebertsuconic-3
Github user clebertsuconic commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1601
 
    @tabish121 you're right.. I will think of something like that... will reopen the JIRA.. and revisit this with another PR.


---