You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@activemq.apache.org by pgfox <gi...@git.apache.org> on 2018/01/19 18:55:03 UTC
[GitHub] activemq-artemis pull request #1796: ARTEMIS-1623 ActiveMQServerPlugin impl ...
GitHub user pgfox opened a pull request:
https://github.com/apache/activemq-artemis/pull/1796
ARTEMIS-1623 ActiveMQServerPlugin impl for logging various broker events
Similar concept to the ActiveMQ 5.x loggingBrokerPlugin.
Commit includes:
- plugin impl
- tests using byteman for checking logging with AMQP, CORE and Openwire
- doc on how to configure plugin
For configuration details please see broker-plugins.md
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/pgfox/activemq-artemis logging_brokerplugin
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/activemq-artemis/pull/1796.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 #1796
----
commit 941a87cd4843c7a2ec3fee117d702450dc4efb85
Author: Pat Fox <pa...@...>
Date: 2018-01-06T13:06:39Z
ARTEMIS-1623 ActiveMQServerPlugin impl for logging various broker events
Similiar concept to the ActiveMQ 5.x loggingBrokerPlugin
----
---
[GitHub] activemq-artemis issue #1796: ARTEMIS-1623 ActiveMQServerPlugin impl for log...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1796
Could the logging events, have LOG codes, e.g. like ActiveMQServerLogger
---
[GitHub] activemq-artemis issue #1796: ARTEMIS-1623 ActiveMQServerPlugin impl for log...
Posted by pgfox <gi...@git.apache.org>.
Github user pgfox commented on the issue:
https://github.com/apache/activemq-artemis/pull/1796
I still had to do the suggested changes we discussed :(
Sorry I did not think the would be merged yet. I hope to get those changes pushed to the PR in the next day or so.
---
[GitHub] activemq-artemis pull request #1796: ARTEMIS-1623 ActiveMQServerPlugin impl ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/activemq-artemis/pull/1796
---
[GitHub] activemq-artemis issue #1796: ARTEMIS-1623 ActiveMQServerPlugin impl for log...
Posted by pgfox <gi...@git.apache.org>.
Github user pgfox commented on the issue:
https://github.com/apache/activemq-artemis/pull/1796
@jbertram , @michaelandrepearce Thanks for reviewing this PR. I will try to implement all the above suggestions.
on the DEBUG vs. INFO I will make it clearer in the docs; there is a before/after on most of the plugin events . For INFO I log a slightly reduced log in the "after" point. For DEBUG I log a more detailed entry before and after.
I will try to follow the established pattern for the LOG codes; create an interface LoggingActiveMQServerPluginLogger that extends the BasicLogger. It there any restriction on the 2 digit Logger Code I use - I think "83" is the last number currently used so I guess "84" would be ok? Or is there a pattern to choosing that value?
---
[GitHub] activemq-artemis issue #1796: ARTEMIS-1623 ActiveMQServerPlugin impl for log...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1796
@pgfox sounds good/sensible to me, unless anyone says otherwise I would have chosen the same.
---
[GitHub] activemq-artemis issue #1796: ARTEMIS-1623 ActiveMQServerPlugin impl for log...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/1796
@pgfox I had missed there were still pending things.. I read it wrongly.
Can you send another PR? or should I revert and you sending it again?
---
[GitHub] activemq-artemis issue #1796: ARTEMIS-1623 ActiveMQServerPlugin impl for log...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1796
Did this get merged? I’m not sure it was ready I was expecting some changes from @pgfox
---
[GitHub] activemq-artemis issue #1796: ARTEMIS-1623 ActiveMQServerPlugin impl for log...
Posted by pgfox <gi...@git.apache.org>.
Github user pgfox commented on the issue:
https://github.com/apache/activemq-artemis/pull/1796
@clebertsuconic
There was some outstanding improvements suggested by @jbertram @michaelandrepearce (above) on this PR that I have not finish yet.
Can I just open a new PR for those improvement when I have them completed? I should have them done in the next day or so. Sorry, for any confusion this has caused.
---
[GitHub] activemq-artemis issue #1796: ARTEMIS-1623 ActiveMQServerPlugin impl for log...
Posted by pgfox <gi...@git.apache.org>.
Github user pgfox commented on the issue:
https://github.com/apache/activemq-artemis/pull/1796
@clebertsuconic no problem Clebert. I will create a new PR when I have those pending things completed.
---
[GitHub] activemq-artemis issue #1796: ARTEMIS-1623 ActiveMQServerPlugin impl for log...
Posted by jbertram <gi...@git.apache.org>.
Github user jbertram commented on the issue:
https://github.com/apache/activemq-artemis/pull/1796
Couple of things:
- Nice work!
- It's not clear to me why some things are logged at DEBUG vs. INFO. Perhaps the documentation could address this.
- Since you're using the "f" logging method variants it's really not necessary to do the isXEnabled() check before logging since those methods do that check already.
- In the places where you're logging either DEBUG or INFO and the format is the same you can use `org.jboss.logging.Logger#logf` and just pass in the level you want to use so you don't have duplicated code.
---