You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2020/09/09 19:20:24 UTC
[GitHub] [druid] mghosh4 opened a new pull request #10373: Adding more dimensions to the audit log entry
mghosh4 opened a new pull request #10373:
URL: https://github.com/apache/druid/pull/10373
Fixes #10372.
Adding more dimensions to the service metric for audit log
<hr>
This PR has:
- [x] been self-reviewed.
- [ ] added documentation for new or modified features or behaviors.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/licenses.yaml)
- [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [ ] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [x] been tested in a test Druid cluster.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] mghosh4 commented on a change in pull request #10373: Adding more dimensions to the audit log entry
Posted by GitBox <gi...@apache.org>.
mghosh4 commented on a change in pull request #10373:
URL: https://github.com/apache/druid/pull/10373#discussion_r486780676
##########
File path: server/src/main/java/org/apache/druid/server/audit/SQLAuditManager.java
##########
@@ -98,6 +98,10 @@ public void doAudit(AuditEntry auditEntry, Handle handle) throws IOException
.setDimension("key", auditEntry.getKey())
.setDimension("type", auditEntry.getType())
.setDimension("author", auditEntry.getAuditInfo().getAuthor())
+ .setDimension("comment", auditEntry.getAuditInfo().getComment())
+ .setDimension("remote_address", auditEntry.getAuditInfo().getIp())
+ .setDimension("created_date", auditEntry.getAuditTime().toString())
+ .setDimension("payload", auditEntry.getPayload())
Review comment:
Actually I checked some of the emitter metric logs and it does seem `payload` has some information not part of other columns. For example, for `audit_type` `rules` it contains the actual load rule change information, similarly for `coordinator.config` it contains the config setting. Looking at this I think I would like to keep the payload information around as well.
##########
File path: server/src/main/java/org/apache/druid/server/audit/SQLAuditManager.java
##########
@@ -98,6 +98,10 @@ public void doAudit(AuditEntry auditEntry, Handle handle) throws IOException
.setDimension("key", auditEntry.getKey())
.setDimension("type", auditEntry.getType())
.setDimension("author", auditEntry.getAuditInfo().getAuthor())
+ .setDimension("comment", auditEntry.getAuditInfo().getComment())
+ .setDimension("remote_address", auditEntry.getAuditInfo().getIp())
+ .setDimension("created_date", auditEntry.getAuditTime().toString())
+ .setDimension("payload", auditEntry.getPayload())
Review comment:
Actually I checked some of the emitter metric logs and it does seem `payload` has some information not part of other columns. For example, for `audit_type` `rules` it contains the actual load rule change information, similarly for `coordinator.config` it contains the config setting. Looking at this I think I would like to keep the payload information around as well.
##########
File path: server/src/main/java/org/apache/druid/server/audit/SQLAuditManager.java
##########
@@ -98,6 +98,10 @@ public void doAudit(AuditEntry auditEntry, Handle handle) throws IOException
.setDimension("key", auditEntry.getKey())
.setDimension("type", auditEntry.getType())
.setDimension("author", auditEntry.getAuditInfo().getAuthor())
+ .setDimension("comment", auditEntry.getAuditInfo().getComment())
+ .setDimension("remote_address", auditEntry.getAuditInfo().getIp())
+ .setDimension("created_date", auditEntry.getAuditTime().toString())
+ .setDimension("payload", auditEntry.getPayload())
Review comment:
Actually I checked some of the emitter metric logs and it does seem `payload` has some information not part of other columns. For example, for `audit_type` `rules` it contains the actual load rule change information, similarly for `coordinator.config` it contains the config setting. Looking at this I think I would like to keep the payload information around as well.
##########
File path: server/src/main/java/org/apache/druid/server/audit/SQLAuditManager.java
##########
@@ -98,6 +98,10 @@ public void doAudit(AuditEntry auditEntry, Handle handle) throws IOException
.setDimension("key", auditEntry.getKey())
.setDimension("type", auditEntry.getType())
.setDimension("author", auditEntry.getAuditInfo().getAuthor())
+ .setDimension("comment", auditEntry.getAuditInfo().getComment())
+ .setDimension("remote_address", auditEntry.getAuditInfo().getIp())
+ .setDimension("created_date", auditEntry.getAuditTime().toString())
+ .setDimension("payload", auditEntry.getPayload())
Review comment:
Actually I checked some of the emitter metric logs and it does seem `payload` has some information not part of other columns. For example, for `audit_type` `rules` it contains the actual load rule change information, similarly for `coordinator.config` it contains the config setting. Looking at this I think I would like to keep the payload information around as well.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on a change in pull request #10373: Adding more dimensions to the audit log entry
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #10373:
URL: https://github.com/apache/druid/pull/10373#discussion_r489996345
##########
File path: docs/configuration/index.md
##########
@@ -330,6 +330,15 @@ Switching Request Logger routes native query's request logs to one request logge
|`druid.request.logging.nativeQueryLogger`|request logger for emitting native query's request logs.|none|
|`druid.request.logging.sqlQueryLogger`|request logger for emitting SQL query's request logs.|none|
+### Audit Logging
+
+Coordinator and Overlord log changes to lookups, segment load/drop rules, dynamic configuration changes for auditing
+
+|Property|Description|Default|
+|--------|-----------|-------|
+|`druid.audit.manager.auditHistoryMillis`|Default duration for querying audit history.|1 week|
+|`druid.audit.manager.addPayloadToAuditMetricDimension`|Boolean flag on whether to add `payload` column in service metric.|false|
Review comment:
nit: how about `includePayloadAsDimensionInMetric`?
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] mghosh4 commented on a change in pull request #10373: Adding more dimensions to the audit log entry
Posted by GitBox <gi...@apache.org>.
mghosh4 commented on a change in pull request #10373:
URL: https://github.com/apache/druid/pull/10373#discussion_r486780676
##########
File path: server/src/main/java/org/apache/druid/server/audit/SQLAuditManager.java
##########
@@ -98,6 +98,10 @@ public void doAudit(AuditEntry auditEntry, Handle handle) throws IOException
.setDimension("key", auditEntry.getKey())
.setDimension("type", auditEntry.getType())
.setDimension("author", auditEntry.getAuditInfo().getAuthor())
+ .setDimension("comment", auditEntry.getAuditInfo().getComment())
+ .setDimension("remote_address", auditEntry.getAuditInfo().getIp())
+ .setDimension("created_date", auditEntry.getAuditTime().toString())
+ .setDimension("payload", auditEntry.getPayload())
Review comment:
Actually I checked some of the emitter metric logs and it does seem `payload` has some information not part of other columns. For example, for `audit_type` `rules` it contains the actual load rule change information, similarly for `coordinator.config` it contains the config setting. Looking at this I think I would like to keep the payload information around as well.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on a change in pull request #10373: Adding more dimensions to the audit log entry
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #10373:
URL: https://github.com/apache/druid/pull/10373#discussion_r489125468
##########
File path: server/src/main/java/org/apache/druid/server/audit/SQLAuditManager.java
##########
@@ -98,6 +98,10 @@ public void doAudit(AuditEntry auditEntry, Handle handle) throws IOException
.setDimension("key", auditEntry.getKey())
.setDimension("type", auditEntry.getType())
.setDimension("author", auditEntry.getAuditInfo().getAuthor())
+ .setDimension("comment", auditEntry.getAuditInfo().getComment())
+ .setDimension("remote_address", auditEntry.getAuditInfo().getIp())
+ .setDimension("created_date", auditEntry.getAuditTime().toString())
+ .setDimension("payload", auditEntry.getPayload())
Review comment:
I would like to make sure that it will be useful before we add any new dimension since it will increase the size of metrics. The metric size could be problematic since this method is used in updating lookups and compaction configurations. If it's not clear now, then we can add it later when it turns out to be useful.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] mghosh4 commented on a change in pull request #10373: Adding more dimensions to the audit log entry
Posted by GitBox <gi...@apache.org>.
mghosh4 commented on a change in pull request #10373:
URL: https://github.com/apache/druid/pull/10373#discussion_r486780676
##########
File path: server/src/main/java/org/apache/druid/server/audit/SQLAuditManager.java
##########
@@ -98,6 +98,10 @@ public void doAudit(AuditEntry auditEntry, Handle handle) throws IOException
.setDimension("key", auditEntry.getKey())
.setDimension("type", auditEntry.getType())
.setDimension("author", auditEntry.getAuditInfo().getAuthor())
+ .setDimension("comment", auditEntry.getAuditInfo().getComment())
+ .setDimension("remote_address", auditEntry.getAuditInfo().getIp())
+ .setDimension("created_date", auditEntry.getAuditTime().toString())
+ .setDimension("payload", auditEntry.getPayload())
Review comment:
Actually I checked some of the emitter metric logs and it does seem `payload` has some information not part of other columns. For example, for `audit_type` `rules` it contains the actual load rule change information, similarly for `coordinator.config` it contains the config setting. Looking at this I think I would like to keep the payload information around as well.
##########
File path: server/src/main/java/org/apache/druid/server/audit/SQLAuditManager.java
##########
@@ -98,6 +98,10 @@ public void doAudit(AuditEntry auditEntry, Handle handle) throws IOException
.setDimension("key", auditEntry.getKey())
.setDimension("type", auditEntry.getType())
.setDimension("author", auditEntry.getAuditInfo().getAuthor())
+ .setDimension("comment", auditEntry.getAuditInfo().getComment())
+ .setDimension("remote_address", auditEntry.getAuditInfo().getIp())
+ .setDimension("created_date", auditEntry.getAuditTime().toString())
+ .setDimension("payload", auditEntry.getPayload())
Review comment:
Actually I checked some of the emitter metric logs and it does seem `payload` has some information not part of other columns. For example, for `audit_type` `rules` it contains the actual load rule change information, similarly for `coordinator.config` it contains the config setting. Looking at this I think I would like to keep the payload information around as well.
##########
File path: server/src/main/java/org/apache/druid/server/audit/SQLAuditManager.java
##########
@@ -98,6 +98,10 @@ public void doAudit(AuditEntry auditEntry, Handle handle) throws IOException
.setDimension("key", auditEntry.getKey())
.setDimension("type", auditEntry.getType())
.setDimension("author", auditEntry.getAuditInfo().getAuthor())
+ .setDimension("comment", auditEntry.getAuditInfo().getComment())
+ .setDimension("remote_address", auditEntry.getAuditInfo().getIp())
+ .setDimension("created_date", auditEntry.getAuditTime().toString())
+ .setDimension("payload", auditEntry.getPayload())
Review comment:
Actually I checked some of the emitter metric logs and it does seem `payload` has some information not part of other columns. For example, for `audit_type` `rules` it contains the actual load rule change information, similarly for `coordinator.config` it contains the config setting. Looking at this I think I would like to keep the payload information around as well.
##########
File path: server/src/main/java/org/apache/druid/server/audit/SQLAuditManager.java
##########
@@ -98,6 +98,10 @@ public void doAudit(AuditEntry auditEntry, Handle handle) throws IOException
.setDimension("key", auditEntry.getKey())
.setDimension("type", auditEntry.getType())
.setDimension("author", auditEntry.getAuditInfo().getAuthor())
+ .setDimension("comment", auditEntry.getAuditInfo().getComment())
+ .setDimension("remote_address", auditEntry.getAuditInfo().getIp())
+ .setDimension("created_date", auditEntry.getAuditTime().toString())
+ .setDimension("payload", auditEntry.getPayload())
Review comment:
Actually I checked some of the emitter metric logs and it does seem `payload` has some information not part of other columns. For example, for `audit_type` `rules` it contains the actual load rule change information, similarly for `coordinator.config` it contains the config setting. Looking at this I think I would like to keep the payload information around as well.
##########
File path: server/src/main/java/org/apache/druid/server/audit/SQLAuditManager.java
##########
@@ -98,6 +98,10 @@ public void doAudit(AuditEntry auditEntry, Handle handle) throws IOException
.setDimension("key", auditEntry.getKey())
.setDimension("type", auditEntry.getType())
.setDimension("author", auditEntry.getAuditInfo().getAuthor())
+ .setDimension("comment", auditEntry.getAuditInfo().getComment())
+ .setDimension("remote_address", auditEntry.getAuditInfo().getIp())
+ .setDimension("created_date", auditEntry.getAuditTime().toString())
+ .setDimension("payload", auditEntry.getPayload())
Review comment:
Actually I checked some of the emitter metric logs and it does seem `payload` has some information not part of other columns. For example, for `audit_type` `rules` it contains the actual load rule change information, similarly for `coordinator.config` it contains the config setting. Looking at this I think I would like to keep the payload information around as well.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on a change in pull request #10373: Adding more dimensions to the audit log entry
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #10373:
URL: https://github.com/apache/druid/pull/10373#discussion_r489575365
##########
File path: server/src/main/java/org/apache/druid/server/audit/SQLAuditManager.java
##########
@@ -98,6 +98,10 @@ public void doAudit(AuditEntry auditEntry, Handle handle) throws IOException
.setDimension("key", auditEntry.getKey())
.setDimension("type", auditEntry.getType())
.setDimension("author", auditEntry.getAuditInfo().getAuthor())
+ .setDimension("comment", auditEntry.getAuditInfo().getComment())
+ .setDimension("remote_address", auditEntry.getAuditInfo().getIp())
+ .setDimension("created_date", auditEntry.getAuditTime().toString())
+ .setDimension("payload", auditEntry.getPayload())
Review comment:
@mghosh4 I agree with you. I think it's a design flaw of how audit logging works in general, so we need to address that design holistically. Right now, even if I look at the `druid_audit` table, I need to do a json diff to actually know what change happened. I haven't created a proposal on how to fix that because I haven't had the time to come up with a meaningful solution to this, and come up with a testing / transition plan so that we don't break the existing audit logging system.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on a change in pull request #10373: Adding more dimensions to the audit log entry
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #10373:
URL: https://github.com/apache/druid/pull/10373#discussion_r490322046
##########
File path: server/src/main/java/org/apache/druid/server/audit/SQLAuditManager.java
##########
@@ -93,13 +93,19 @@ public Void withHandle(Handle handle) throws Exception
@Override
public void doAudit(AuditEntry auditEntry, Handle handle) throws IOException
{
- emitter.emit(
- new ServiceMetricEvent.Builder()
- .setDimension("key", auditEntry.getKey())
- .setDimension("type", auditEntry.getType())
- .setDimension("author", auditEntry.getAuditInfo().getAuthor())
- .build("config/audit", 1)
- );
+ ServiceMetricEvent.Builder builder = new ServiceMetricEvent.Builder()
+ .setDimension("key", auditEntry.getKey())
+ .setDimension("type", auditEntry.getType())
+ .setDimension("author", auditEntry.getAuditInfo().getAuthor())
+ .setDimension("comment", auditEntry.getAuditInfo().getComment())
+ .setDimension("remote_address", auditEntry.getAuditInfo().getIp())
+ .setDimension("created_date", auditEntry.getAuditTime().toString());
+
+ if (config.getAddPayloadToAuditMetricDimension()) {
+ builder.setDimension("payload", auditEntry.getPayload());
+ }
Review comment:
Can you add a unit test that validates this behavior please
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on a change in pull request #10373: Adding more dimensions to the audit log entry
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #10373:
URL: https://github.com/apache/druid/pull/10373#discussion_r489575365
##########
File path: server/src/main/java/org/apache/druid/server/audit/SQLAuditManager.java
##########
@@ -98,6 +98,10 @@ public void doAudit(AuditEntry auditEntry, Handle handle) throws IOException
.setDimension("key", auditEntry.getKey())
.setDimension("type", auditEntry.getType())
.setDimension("author", auditEntry.getAuditInfo().getAuthor())
+ .setDimension("comment", auditEntry.getAuditInfo().getComment())
+ .setDimension("remote_address", auditEntry.getAuditInfo().getIp())
+ .setDimension("created_date", auditEntry.getAuditTime().toString())
+ .setDimension("payload", auditEntry.getPayload())
Review comment:
@mghosh4 Yes that's correct. I agree with your concern about the dimensions not capturing the actual change. I think it's a design flaw of how audit logging works in general, so we need to address that design holistically.
Right now, even if I look at the `druid_audit` table, I need to do a json diff to actually know what change happened. I haven't created a proposal on how to fix that because I haven't had the time to come up with a meaningful solution to this, and come up with a testing / transition plan so that we don't break the existing audit logging system.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] mghosh4 commented on a change in pull request #10373: Adding more dimensions to the audit log entry
Posted by GitBox <gi...@apache.org>.
mghosh4 commented on a change in pull request #10373:
URL: https://github.com/apache/druid/pull/10373#discussion_r489175802
##########
File path: server/src/main/java/org/apache/druid/server/audit/SQLAuditManager.java
##########
@@ -98,6 +98,10 @@ public void doAudit(AuditEntry auditEntry, Handle handle) throws IOException
.setDimension("key", auditEntry.getKey())
.setDimension("type", auditEntry.getType())
.setDimension("author", auditEntry.getAuditInfo().getAuthor())
+ .setDimension("comment", auditEntry.getAuditInfo().getComment())
+ .setDimension("remote_address", auditEntry.getAuditInfo().getIp())
+ .setDimension("created_date", auditEntry.getAuditTime().toString())
+ .setDimension("payload", auditEntry.getPayload())
Review comment:
I have double checked, all the dimensions convey useful information. Which column is not clear to you?
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] mghosh4 commented on a change in pull request #10373: Adding more dimensions to the audit log entry
Posted by GitBox <gi...@apache.org>.
mghosh4 commented on a change in pull request #10373:
URL: https://github.com/apache/druid/pull/10373#discussion_r486618678
##########
File path: server/src/main/java/org/apache/druid/server/audit/SQLAuditManager.java
##########
@@ -98,6 +98,10 @@ public void doAudit(AuditEntry auditEntry, Handle handle) throws IOException
.setDimension("key", auditEntry.getKey())
.setDimension("type", auditEntry.getType())
.setDimension("author", auditEntry.getAuditInfo().getAuthor())
+ .setDimension("comment", auditEntry.getAuditInfo().getComment())
+ .setDimension("remote_address", auditEntry.getAuditInfo().getIp())
+ .setDimension("created_date", auditEntry.getAuditTime().toString())
+ .setDimension("payload", auditEntry.getPayload())
Review comment:
Typically `comment` tends to have the business reason for doing the change. So it is useful for auditing. I am on the fence about `payload`. I wasn't sure whether it has any extra information not covered by the other columns. So decided to add that one. I can remove it if that is not the case.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] mghosh4 commented on a change in pull request #10373: Adding more dimensions to the audit log entry
Posted by GitBox <gi...@apache.org>.
mghosh4 commented on a change in pull request #10373:
URL: https://github.com/apache/druid/pull/10373#discussion_r486780676
##########
File path: server/src/main/java/org/apache/druid/server/audit/SQLAuditManager.java
##########
@@ -98,6 +98,10 @@ public void doAudit(AuditEntry auditEntry, Handle handle) throws IOException
.setDimension("key", auditEntry.getKey())
.setDimension("type", auditEntry.getType())
.setDimension("author", auditEntry.getAuditInfo().getAuthor())
+ .setDimension("comment", auditEntry.getAuditInfo().getComment())
+ .setDimension("remote_address", auditEntry.getAuditInfo().getIp())
+ .setDimension("created_date", auditEntry.getAuditTime().toString())
+ .setDimension("payload", auditEntry.getPayload())
Review comment:
Actually I checked some of the emitter metric logs and it does seem `payload` has some information not part of other columns. For example, for `audit_type` `rules` it contains the actual load rule change information, similarly for `coordinator.config` it contains the config setting. Looking at this I think I would like to keep the payload information around as well.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] mghosh4 commented on a change in pull request #10373: Adding more dimensions to the audit log entry
Posted by GitBox <gi...@apache.org>.
mghosh4 commented on a change in pull request #10373:
URL: https://github.com/apache/druid/pull/10373#discussion_r489595165
##########
File path: server/src/main/java/org/apache/druid/server/audit/SQLAuditManager.java
##########
@@ -98,6 +98,10 @@ public void doAudit(AuditEntry auditEntry, Handle handle) throws IOException
.setDimension("key", auditEntry.getKey())
.setDimension("type", auditEntry.getType())
.setDimension("author", auditEntry.getAuditInfo().getAuthor())
+ .setDimension("comment", auditEntry.getAuditInfo().getComment())
+ .setDimension("remote_address", auditEntry.getAuditInfo().getIp())
+ .setDimension("created_date", auditEntry.getAuditTime().toString())
+ .setDimension("payload", auditEntry.getPayload())
Review comment:
Got it. So let me add a parameter called `addPayloadToAuditMetricDimension` in `SQLAuditManagerConfig`. I can use that to set the payload. Does that sound good? @suneet-s @jihoonson
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] mghosh4 commented on a change in pull request #10373: Adding more dimensions to the audit log entry
Posted by GitBox <gi...@apache.org>.
mghosh4 commented on a change in pull request #10373:
URL: https://github.com/apache/druid/pull/10373#discussion_r489569063
##########
File path: server/src/main/java/org/apache/druid/server/audit/SQLAuditManager.java
##########
@@ -98,6 +98,10 @@ public void doAudit(AuditEntry auditEntry, Handle handle) throws IOException
.setDimension("key", auditEntry.getKey())
.setDimension("type", auditEntry.getType())
.setDimension("author", auditEntry.getAuditInfo().getAuthor())
+ .setDimension("comment", auditEntry.getAuditInfo().getComment())
+ .setDimension("remote_address", auditEntry.getAuditInfo().getIp())
+ .setDimension("created_date", auditEntry.getAuditTime().toString())
+ .setDimension("payload", auditEntry.getPayload())
Review comment:
@suneet-s Interesting, is this because all the compaction config for 10K+ datasources get generated even when we are updating only 1? Do you think we can optimize that?
I am fine with making `payload` optional. My only concern is none of the dimension columns capture the actual change that happened which I believe is part of `payload`. This is important from the perspective of auditing.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on a change in pull request #10373: Adding more dimensions to the audit log entry
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #10373:
URL: https://github.com/apache/druid/pull/10373#discussion_r486681442
##########
File path: server/src/main/java/org/apache/druid/server/audit/SQLAuditManager.java
##########
@@ -98,6 +98,10 @@ public void doAudit(AuditEntry auditEntry, Handle handle) throws IOException
.setDimension("key", auditEntry.getKey())
.setDimension("type", auditEntry.getType())
.setDimension("author", auditEntry.getAuditInfo().getAuthor())
+ .setDimension("comment", auditEntry.getAuditInfo().getComment())
+ .setDimension("remote_address", auditEntry.getAuditInfo().getIp())
+ .setDimension("created_date", auditEntry.getAuditTime().toString())
+ .setDimension("payload", auditEntry.getPayload())
Review comment:
Thanks. How about removing `payload` for now then? We can add it later if it turns out to be useful.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on a change in pull request #10373: Adding more dimensions to the audit log entry
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #10373:
URL: https://github.com/apache/druid/pull/10373#discussion_r489529218
##########
File path: server/src/main/java/org/apache/druid/server/audit/SQLAuditManager.java
##########
@@ -98,6 +98,10 @@ public void doAudit(AuditEntry auditEntry, Handle handle) throws IOException
.setDimension("key", auditEntry.getKey())
.setDimension("type", auditEntry.getType())
.setDimension("author", auditEntry.getAuditInfo().getAuthor())
+ .setDimension("comment", auditEntry.getAuditInfo().getComment())
+ .setDimension("remote_address", auditEntry.getAuditInfo().getIp())
+ .setDimension("created_date", auditEntry.getAuditTime().toString())
+ .setDimension("payload", auditEntry.getPayload())
Review comment:
I'm concerned about the size of the payload. That is usually an unbounded json blob. So this could have scale implications. For example an audit message of a compaction change for a datasource in a cluster with 10k+ datasources can generate a payload that's several MB of text.
If reporting payload is critical for some use cases, it should be disabled by default so users who want it can opt in.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson merged pull request #10373: Adding more dimensions to the audit log entry
Posted by GitBox <gi...@apache.org>.
jihoonson merged pull request #10373:
URL: https://github.com/apache/druid/pull/10373
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on a change in pull request #10373: Adding more dimensions to the audit log entry
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #10373:
URL: https://github.com/apache/druid/pull/10373#discussion_r485989247
##########
File path: server/src/main/java/org/apache/druid/server/audit/SQLAuditManager.java
##########
@@ -98,6 +98,10 @@ public void doAudit(AuditEntry auditEntry, Handle handle) throws IOException
.setDimension("key", auditEntry.getKey())
.setDimension("type", auditEntry.getType())
.setDimension("author", auditEntry.getAuditInfo().getAuthor())
+ .setDimension("comment", auditEntry.getAuditInfo().getComment())
+ .setDimension("remote_address", auditEntry.getAuditInfo().getIp())
+ .setDimension("created_date", auditEntry.getAuditTime().toString())
+ .setDimension("payload", auditEntry.getPayload())
Review comment:
Hmm, do we need to emit `comment` and `payload` as metrics? They seem to need to be retrieved via API instead.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] mghosh4 commented on a change in pull request #10373: Adding more dimensions to the audit log entry
Posted by GitBox <gi...@apache.org>.
mghosh4 commented on a change in pull request #10373:
URL: https://github.com/apache/druid/pull/10373#discussion_r489595165
##########
File path: server/src/main/java/org/apache/druid/server/audit/SQLAuditManager.java
##########
@@ -98,6 +98,10 @@ public void doAudit(AuditEntry auditEntry, Handle handle) throws IOException
.setDimension("key", auditEntry.getKey())
.setDimension("type", auditEntry.getType())
.setDimension("author", auditEntry.getAuditInfo().getAuthor())
+ .setDimension("comment", auditEntry.getAuditInfo().getComment())
+ .setDimension("remote_address", auditEntry.getAuditInfo().getIp())
+ .setDimension("created_date", auditEntry.getAuditTime().toString())
+ .setDimension("payload", auditEntry.getPayload())
Review comment:
Got it. So let me add a parameter called `addPayloadToAuditLogMetricDimension` in `SQLAuditManagerConfig`. I can use that to set the payload. Does that sound good? @suneet-s @jihoonson
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org