You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@activemq.apache.org by GitBox <gi...@apache.org> on 2020/04/30 13:29:56 UTC
[GitHub] [activemq-artemis] andytaylor opened a new pull request #3107: ARTEMIS-2648 - audit logging improvements
andytaylor opened a new pull request #3107:
URL: https://github.com/apache/activemq-artemis/pull/3107
https://issues.apache.org/jira/browse/ARTEMIS-2648
A description of what I have done can be found on the Jira. Id like however some feedback on something, where I have added new logging I have removed the log message that was previously in the same place so where there was a log message for creating a queue I have added a success and failure log. If anyone thinks this is a big deal i can add back the old messages but this will mean there are multiple logs potentially occurring.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #3107: ARTEMIS-2648 - audit logging improvements
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on pull request #3107:
URL: https://github.com/apache/activemq-artemis/pull/3107#issuecomment-622081694
ArtemisFeatureTest fails as we talked about.
just using the tag to avoid someone to merge it before it's fixed.
let me know when it's updated and I will remove the tag.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #3107: ARTEMIS-2648 - audit logging improvements
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on pull request #3107:
URL: https://github.com/apache/activemq-artemis/pull/3107#issuecomment-622989873
this branch has no test failures now. We just need to address asynchronous operations and the thread local that was needed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3107: ARTEMIS-2648 - audit logging improvements
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3107:
URL: https://github.com/apache/activemq-artemis/pull/3107#discussion_r418986910
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/handler/ProtonHandler.java
##########
@@ -497,6 +498,9 @@ private void dispatch() {
if (log.isTraceEnabled()) {
log.trace("Handling " + ev + " towards " + h);
}
+ if (AuditLogger.isAnyLoggingEnabled()) {
Review comment:
there are no test failures on your branch now.. we just need to resolve the thread local versus thread changes due to asynchronous operations on the broker.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3107: ARTEMIS-2648 - audit logging improvements
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3107:
URL: https://github.com/apache/activemq-artemis/pull/3107#discussion_r418606661
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/handler/ProtonHandler.java
##########
@@ -497,6 +498,9 @@ private void dispatch() {
if (log.isTraceEnabled()) {
log.trace("Handling " + ev + " towards " + h);
}
+ if (AuditLogger.isAnyLoggingEnabled()) {
Review comment:
Do you need to do this for every event? It seems you only need to this only once.. so I think you could do this at the beggining of the method, before the loop starts.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3107: ARTEMIS-2648 - audit logging improvements
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3107:
URL: https://github.com/apache/activemq-artemis/pull/3107#discussion_r418536427
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/handler/EventHandler.java
##########
@@ -91,4 +91,7 @@ default boolean flowControl(ReadyListener readyListener) {
return true;
}
+ default String getRemoteAddress() {
Review comment:
You don't need this. you're using AMQPConnection directly, no need to change to change EventHandler.
Can you remove it?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] andytaylor commented on a change in pull request #3107: ARTEMIS-2648 - audit logging improvements
Posted by GitBox <gi...@apache.org>.
andytaylor commented on a change in pull request #3107:
URL: https://github.com/apache/activemq-artemis/pull/3107#discussion_r418582481
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/handler/EventHandler.java
##########
@@ -91,4 +91,7 @@ default boolean flowControl(ReadyListener readyListener) {
return true;
}
+ default String getRemoteAddress() {
Review comment:
It is on every event so it is available to the Audit logger
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3107: ARTEMIS-2648 - audit logging improvements
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3107:
URL: https://github.com/apache/activemq-artemis/pull/3107#discussion_r418537560
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/handler/EventHandler.java
##########
@@ -91,4 +91,7 @@ default boolean flowControl(ReadyListener readyListener) {
return true;
}
+ default String getRemoteAddress() {
Review comment:
actually, you do use it...
But you are calling this on every proton event. isn't there a better place to call that?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3107: ARTEMIS-2648 - audit logging improvements
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3107:
URL: https://github.com/apache/activemq-artemis/pull/3107#discussion_r418986910
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/handler/ProtonHandler.java
##########
@@ -497,6 +498,9 @@ private void dispatch() {
if (log.isTraceEnabled()) {
log.trace("Handling " + ev + " towards " + h);
}
+ if (AuditLogger.isAnyLoggingEnabled()) {
Review comment:
there are no test failures on your branch now.. we just need to resolve the thread local versus thread changes due to asynchronous operations on the broker.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3107: ARTEMIS-2648 - audit logging improvements
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3107:
URL: https://github.com/apache/activemq-artemis/pull/3107#discussion_r418541402
##########
File path: artemis-protocols/artemis-openwire-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/openwire/OpenWireConnection.java
##########
@@ -287,6 +288,9 @@ public void bufferReceived(Object connectionID, ActiveMQBuffer buffer) {
if (logger.isTraceEnabled()) {
traceBufferReceived(connectionID, command);
}
+ if (AuditLogger.isAnyLoggingEnabled()) {
+ AuditLogger.setRemoteAddress(getRemoteAddress());
Review comment:
this could eventually fail in other protocols as well. you would endup informing a wrong remote address for Audit Operations.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3107: ARTEMIS-2648 - audit logging improvements
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3107:
URL: https://github.com/apache/activemq-artemis/pull/3107#discussion_r418539838
##########
File path: artemis-protocols/artemis-openwire-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/openwire/OpenWireConnection.java
##########
@@ -287,6 +288,9 @@ public void bufferReceived(Object connectionID, ActiveMQBuffer buffer) {
if (logger.isTraceEnabled()) {
traceBufferReceived(connectionID, command);
}
+ if (AuditLogger.isAnyLoggingEnabled()) {
+ AuditLogger.setRemoteAddress(getRemoteAddress());
Review comment:
this won't work...
openWireActor.act() will transfer the execution to a different thread.
you will lose the ThreadLocal here.
The netty loop is reused for many connections.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] andytaylor commented on a change in pull request #3107: ARTEMIS-2648 - audit logging improvements
Posted by GitBox <gi...@apache.org>.
andytaylor commented on a change in pull request #3107:
URL: https://github.com/apache/activemq-artemis/pull/3107#discussion_r418582625
##########
File path: artemis-protocols/artemis-openwire-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/openwire/OpenWireConnection.java
##########
@@ -287,6 +288,9 @@ public void bufferReceived(Object connectionID, ActiveMQBuffer buffer) {
if (logger.isTraceEnabled()) {
traceBufferReceived(connectionID, command);
}
+ if (AuditLogger.isAnyLoggingEnabled()) {
+ AuditLogger.setRemoteAddress(getRemoteAddress());
Review comment:
ok, I will address
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3107: ARTEMIS-2648 - audit logging improvements
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3107:
URL: https://github.com/apache/activemq-artemis/pull/3107#discussion_r418595771
##########
File path: artemis-protocols/artemis-openwire-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/openwire/OpenWireConnection.java
##########
@@ -287,6 +288,9 @@ public void bufferReceived(Object connectionID, ActiveMQBuffer buffer) {
if (logger.isTraceEnabled()) {
traceBufferReceived(connectionID, command);
}
+ if (AuditLogger.isAnyLoggingEnabled()) {
+ AuditLogger.setRemoteAddress(getRemoteAddress());
Review comment:
Just an idea. We have OperationContext, which will hold operations to the Session, and it has a thread local already.
Why don't you somehow inject the session on the operation context and use the thread local from there. We recover it back whenever we do a new operation on the session. It is a reliable place.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3107: ARTEMIS-2648 - audit logging improvements
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3107:
URL: https://github.com/apache/activemq-artemis/pull/3107#discussion_r418596091
##########
File path: artemis-protocols/artemis-openwire-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/openwire/OpenWireConnection.java
##########
@@ -287,6 +288,9 @@ public void bufferReceived(Object connectionID, ActiveMQBuffer buffer) {
if (logger.isTraceEnabled()) {
traceBufferReceived(connectionID, command);
}
+ if (AuditLogger.isAnyLoggingEnabled()) {
+ AuditLogger.setRemoteAddress(getRemoteAddress());
Review comment:
I mean.. why don't you inject the data you need on the operation context.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org