ARTEMIS-2273 Adding Audit Log #2579
Conversation
| @@ -1516,6 +1541,9 @@ public synchronized RoutingStatus send(Transaction tx, | |||
| final boolean direct, | |||
| boolean noAutoCreateQueue, | |||
| RoutingContext routingContext) throws Exception { | |||
| if (AuditLogger.isEnabled()) { | |||
| AuditLogger.sendMessage(this, getUsername(), tx, messageParameter, direct, noAutoCreateQueue, routingContext); | |||
There was a problem hiding this comment.
Shouldn't this be done on PostOfficeImpl::send?
Then the same would apply to all the protocols.
Currently your implementation would ignore anything coming from Federation, MQTT AMQP or Stomp, since those will call PostOfficeImpl::send directly.
There was a problem hiding this comment.
If there is audit on this. I think there needs to be some audit level seperation. E.g. users may want to audit management actions but not want to audit log every message going through the broker this would be very expensive on the hot path
There was a problem hiding this comment.
I am -1 it for this reason to avoid this just being merged without this being addressed.
There was a problem hiding this comment.
Just to add i like the idea, just think there needs to be finer control on what to audit especially hot paths like send message and other bits.
Also is this a chance to extend the plugin interfaces and implement this as a plugin. I actually thought we had some logger plugin thinking about it
There was a problem hiding this comment.
Please see: org.apache.activemq.artemis.core.server.plugin.impl.LoggingActiveMQServerPlugin
It be good to extend this (also it has the finer control already started), rather than another implementation to avoid competing solutions to the same problems and maintenance in two places.
There was a problem hiding this comment.
Shouldn't this be done on PostOfficeImpl::send?
Then the same would apply to all the protocols.
Currently your implementation would ignore anything coming from Federation, MQTT AMQP or Stomp, since those will call PostOfficeImpl::send directly.
The send() is auditing the message coming into the broker. So I think it can cover other protocols. right? (currently the audit doesn't record the messages going out but it logs the consumer creations).
There was a problem hiding this comment.
Hi @michaelandrepearce Just had a look at the plugin class my impression is it's a big too complicated for audit log. But let me think about it more. Also I agree the idea to fine control the logging on 'hot' paths so to give user a choice to turn off them particularly.
Thanks
There was a problem hiding this comment.
Whats complicated about it? You define plugin in broker xml and voila...
The point being this is exactly the use case for plugins
There was a problem hiding this comment.
the log format is different. and the logging plugin serves a different purpose that auditing. (actually in amq5 they are totally different impls). and it brings in extra config.
for hot path (actually so far there is only one sending message) I can use log level to easily control it.
So I'd for the moment don't touch the logging plugin.
wdyt?
gaohoward
force-pushed
the
a_e985_audit2
branch
2 times, most recently
from
49cdd7b
to
79affb1
Compare
|
I think as long as its a users choice its fine to have it on or off(default), if the user has it on they accept the hit. |
|
@andytaylor point here is to make it more usable. E.g. audit is important for lots of people but what hit they take and need to audit is different. Especially hot path. |
|
I updated the branch to allow user to control the logging of hot path (i.e. sending message here). |
|
I don’t think using debug versus info is a good choice on this case. You should make a new logger for messaging logger. Or use broker plugin and enable / disable based on that plugin. |
|
Agree with clebert here. I wasn't meaning just seperate send message, it was just one example on why you want to toggle what gets audit. You want to give flexibility to toggle different areas just like the plugin is doing. Still -1 atm only to avoid this being merged until bits are addressed, im 100% +1 the idea and where its going. |
|
I don’t think it’s feasible to control every single point. I think we should separate messages from everything else. Otherwise too many options on confit also make it difficult to admin. And that’s the same even if you used plugin. |
|
Agreed. I think the level the plugin today has is about the right level of flexibility. So if that was similar here that be best. |
|
OK guys. I'll make the hot path logging as a separate logger. |
The Audit log allows user to log some important actions, such as ones performed via management APIs or clients, like queue management, sending messages, etc. The log tries to record who (the user if any) doing what (like deleting a queue) with arguments (if any) and timestamps. By default the audit log is disabled. Through configuration can be easily turned on.
gaohoward
force-pushed
the
a_e985_audit2
branch
from
79affb1
to
775c2bf
Compare
|
Just updated the branch. Now we have 2 loggers. one is base logger and the other is message logger (for hot path logging i.e. sending messages) |
|
Turning my frown upside down. +1 thanks for addressing. Agreed we could split out more later with further specific loggers. |
|
@clebertsuconic @gaohoward since this change, i can still build from command line, but its causing a massive issue within my IDE as its causing generated classes "AuditLogger_$logger" in the parent target folder, which don't obviously then have access to logging libs (as declared in submodule), and thus meaning i cannot debug unit tests within my IDE. If i revert this change its all fine. Can you have a look at please, as its kinda made working and debugging really hard now, as i keep having to revert this commit to work locally within the IDE. Im using IntelliJ. |
|
fyi, i think its something to do with javadoc plugin causing it, other modules that declare logging have a profile for javadoc that seems to be the thing stopping it from causing issue for those loggers |
|
Have sent possible fix on PR 2654 |
|
@michaelandrepearce I am working with IDEA fine. Some times I have to git clean -xdf And rebuild my entire workspace.. But nothing related to this change. |
The Audit log allows user to log some important actions,
such as ones performed via management APIs or clients,
like queue management, sending messages, etc.
The log tries to record who (the user if any) doing what
(like deleting a queue) with arguments (if any) and timestamps.
By default the audit log is disabled. Through configuration can
be easily turned on.