Introduce a new configuration that skip storing audit payload if payload size exceed limit and skip storing null fields for audit payload #11078

maytasm · 2021-04-07T06:11:29Z

Introduce a new configuration that skip storing audit payload if payload size exceed limit and skip storing null fields for audit payload

Description

Audit log such as coordinator.compaction.config can get very large as datasources and their configurations churn. This can cause problems such as

Metadata database taking up too much space / running out of storage space
Configuration change (such as compaction configuration) failing as we are not able to insert new row into audit table due to exceeding the database size limit (max_allowed_packet, etc)

This PR introduce a new configuration that skip storing audit payload if payload size exceed configured limit. The default value is set to -1 which disables this check, meaning Druid will always store audit payload regardless of it's size. This PR also skip storing null fields for audit payload in the effort of saving storage space as those are unnecessary.

This PR has:

been self-reviewed.
- using the concurrency checklist (Remove this item if the PR doesn't have any relation to concurrency.)
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
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 is met.
added integration tests.
been tested in a test Druid cluster.

kfaraz

Nit: Perhaps a more readable name for the config would be druid.audit.manager.maxPayloadSizeBytes? I don't think the exact behaviour (skipping the store in this case) needs to be a part of the config name.

maytasm · 2021-04-07T17:10:17Z

Nit: Perhaps a more readable name for the config would be druid.audit.manager.maxPayloadSizeBytes? I don't think the exact behaviour (skipping the store in this case) needs to be a part of the config name.

I like that name better. Done

jihoonson · 2021-04-07T20:59:33Z

Added 'Design Review' as it introduces a new user-facing configuration.

suneet-s

Approach LGTM.

The defaults seem good as well 👍

suneet-s · 2021-04-07T23:19:46Z

docs/configuration/index.md

@@ -338,6 +338,8 @@ Coordinator and Overlord log changes to lookups, segment load/drop rules, dynami
 |--------|-----------|-------|
 |`druid.audit.manager.auditHistoryMillis`|Default duration for querying audit history.|1 week|
 |`druid.audit.manager.includePayloadAsDimensionInMetric`|Boolean flag on whether to add `payload` column in service metric.|false|
+|`druid.audit.manager.maxPayloadSizeBytes`|The maximum size of audit payload, in bytes, to store in Druid's metadata store audit table. If the size of audit payload exceed this value, the audit log would be stored with a messaging indicating that the payload was omitted instead. Setting maxPayloadSizeBytes to -1 (default value) disables this check, meaning Druid will always store audit payload regardless of it's size.|-1|


nit: This way we can avoid the update to the .spelling file

Suggested change

|`druid.audit.manager.maxPayloadSizeBytes`|The maximum size of audit payload, in bytes, to store in Druid's metadata store audit table. If the size of audit payload exceed this value, the audit log would be stored with a messaging indicating that the payload was omitted instead. Setting maxPayloadSizeBytes to -1 (default value) disables this check, meaning Druid will always store audit payload regardless of it's size.|-1|

|`druid.audit.manager.maxPayloadSizeBytes`|The maximum size of audit payload, in bytes, to store in Druid's metadata store audit table. If the size of audit payload exceed this value, the audit log would be stored with a messaging indicating that the payload was omitted instead. Setting `maxPayloadSizeBytes` to -1 (default value) disables this check, meaning Druid will always store audit payload regardless of it's size.|-1|

suneet-s · 2021-04-07T23:22:16Z

core/src/main/java/org/apache/druid/audit/AuditManager.java

@@ -28,6 +28,8 @@

 public interface AuditManager
 {
+  String PAYLOAD_SKIP_MESSAGE = "Payload was not stored as the payload size exceed limit configured by druid.audit.manager.maxPayloadSizeBytes";


javadocs please

suneet-s · 2021-04-07T23:23:18Z

core/src/main/java/org/apache/druid/common/config/ConfigSerde.java

@@ -25,5 +25,6 @@
 {
  byte[] serialize(T obj);
  String serializeToString(T obj);
+  String serializeSkipNullToString(T obj);


I don't understand this part of the code so well, could you add a javadoc and talk about what the expectations of this function are and how they're different from serializeToString above. Maybe javadocs will help clarify this for me

suneet-s · 2021-04-07T23:27:52Z

core/src/main/java/org/apache/druid/common/config/JacksonConfigManager.java

@@ -50,6 +52,7 @@ public JacksonConfigManager(
    this.configManager = configManager;
    this.jsonMapper = jsonMapper;
    this.auditManager = auditManager;
+    this.jsonMapperSkipNull = jsonMapper.copy().setSerializationInclusion(JsonInclude.Include.NON_NULL);


I think this object should be bound in the JacksonModule as a singleton.

Even though JacksonConfigManager is a singleton, if this binding changes in the future, we could inadvertently create an ObjectMapper per JacksonConfigManager instead of a singleton that can be shared across the process.

~~Otherwise, each time we create a new JacksonConfigMapper, we will create a new ObjectMapper, even though, jsonMapper is a singleton.~~

FrankChen021 · 2021-04-08T03:13:47Z

server/src/main/java/org/apache/druid/server/audit/SQLAuditManagerConfig.java

@@ -31,6 +31,9 @@
  @JsonProperty
  private boolean includePayloadAsDimensionInMetric = false;

+  @JsonProperty
+  private long maxPayloadSizeBytes = -1;


Could this property declared as type of HumanReadableBytes ?

HumanReadableBytes won't work here. I am using the value -1 to disabled this check / unlimited max payload size. This is the same as config like maxBytesInMemory where setting maxBytesInMemory to -1 disables the check.

Does HumanReadableBytes not work with -1? I thought it does. @FrankChen021 I noticed we have no unit tests for HumanReadableBytes. It would be nice to have some 🙂.

This is the same as config like maxBytesInMemory where setting maxBytesInMemory to -1 disables the check.

To me, the design of maxBytesInMemory is not the best because what -1 or 0 mean is not intuitive which means you have to learn those by reading the doc. Similar idea for this, I think it's better to be HumanReadableBytes. You can add another flag to enable/disable the check if you want.

Does HumanReadableBytes not work with -1?

HumanReadableBytes does not work with negative value from config since I think there's no such scenario that a size could be negative. But it can accept negative value from code.

Here I think there're some ways:

if -1 is allowed in configuration, we could remove that constraint in HumanReadableBytes, and apply HumanReadableBytesRange annotation to make sure its value is in valid range

or, if we leave that constraint, user must delete this configuration to disable the check

or, provide an independent flag to enable or disable the check as suggested by @jihoonson

IMO, 3rd is better.

I noticed we have no unit tests for HumanReadableBytes. It would be nice to have some 🙂.

HumanReadableBytes has been covered by UT in HumanReadableBytesTest file.

@jihoonson Ah, I was wrong. HumanReadableBytes does allow -1. For example, CaffeineCacheConfig's sizeInBytes also uses HumanReadableBytes and set default to -1. Changed this to use HumanReadableBytes. However, I kept the default value as -1 which is use for disabling the check. This is consistent with a few other configs we have in Druid right now. We can discuss on changing all of the configs and standardize disable/enable in a separate PR.

@FrankChen021 Negative values should not be set by the user. We can set it to -1 in the code for disabling the check and if user set it (which is to enable it), then it must be set to a positive number.

@FrankChen021 oh you are right. There is a class for unit tests. Sorry, a filter that excludes all test classes might be on in my intellij when I searched last time 😓

FrankChen021 · 2021-04-08T03:19:59Z

I have a question. Users have no idea of the size of payload, are there any suggestions that tell users how to set this configuration ? Can we skip storing payload based on value of type ?

maytasm · 2021-04-08T05:37:18Z

I have a question. Users have no idea of the size of payload, are there any suggestions that tell users how to set this configuration ? Can we skip storing payload based on value of type ?

In another PR, I am adding an auto cleanup based on size limit for the druid_audit metadata table. This work similar to druid.indexer.logs.kill.* which periodically clean up task entries in the druid_tasks table. Basically in my other PR, user will be able to configured the max number of entries they want to store in the druid_audit metadata table, and Druid will automatically remove from oldest entry (FIFO) when the table entries exceed this size limit. Using both of these configuration (this PR and the other PR), Druid operator will be able to control the size of the audit metadata table. For example, if you want to limit your audit table metadata to 20GB (i.e. that is how much storage you have on your machine), and say you want keep x number of last audit entries, you can then set maxPayloadSizeBytes to 20GB / x
This make sure that the audit table will not grow uncontrollably and bringing down / effecting your whole Druid cluster.

jihoonson · 2021-04-08T22:00:14Z

core/src/main/java/org/apache/druid/audit/AuditManager.java

+   * This String is the default message stored instead of the actual audit payload if the audit payload size
+   * exceeded the maximum size limit configuration
+   */
+  String PAYLOAD_SKIP_MESSAGE = "Payload was not stored as the payload size exceed limit configured by druid.audit.manager.maxPayloadSizeBytes";


I think it might be better to include what was the limit when payload was skipped. Maybe something like Payload was not stored as its size exceeds the limit [%d] configured by druid.audit.manager.maxPayloadSizeBytes.

jihoonson · 2021-04-08T22:01:21Z

core/src/main/java/org/apache/druid/common/config/ConfigSerde.java

+   * @param obj to be serialize
+   * @param skipNull if true, then skip serialization of any field with null value.
+   *                 This can be used to reduce the size of the resulting String.
+   * @return String serialization of the input without fields with null values


Is this true only when skipNull is true?

Oops. Updated.

jihoonson · 2021-04-08T22:06:57Z

core/src/main/java/org/apache/druid/guice/annotations/JsonOnlyNonNullValueSerialization.java

+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+
+/**


Javadoc would be nice.

jihoonson · 2021-04-08T22:15:39Z

server/src/main/java/org/apache/druid/server/audit/SQLAuditManagerConfig.java

@@ -31,6 +31,9 @@
  @JsonProperty
  private boolean includePayloadAsDimensionInMetric = false;

+  @JsonProperty
+  private long maxPayloadSizeBytes = -1;


Does HumanReadableBytes not work with -1? I thought it does. @FrankChen021 I noticed we have no unit tests for HumanReadableBytes. It would be nice to have some 🙂.

This is the same as config like maxBytesInMemory where setting maxBytesInMemory to -1 disables the check.

To me, the design of maxBytesInMemory is not the best because what -1 or 0 mean is not intuitive which means you have to learn those by reading the doc. Similar idea for this, I think it's better to be HumanReadableBytes. You can add another flag to enable/disable the check if you want.

jihoonson · 2021-04-08T22:51:15Z

core/src/main/java/org/apache/druid/common/config/JacksonConfigManager.java

@@ -77,13 +83,14 @@ public JacksonConfigManager(
                  .key(key)
                  .type(key)
                  .auditInfo(auditInfo)
-                  .payload(configSerde.serializeToString(val))
+                  .payload(configSerde.serializeToString(val, true))


I think we need a feature flag for this too. Otherwise, it will break existing user applications that work based on audit logs if any.

jihoonson · 2021-04-08T23:10:18Z

core/src/main/java/org/apache/druid/guice/annotations/JsonOnlyNonNullValueSerialization.java

+@Retention(RetentionPolicy.RUNTIME)
+@BindingAnnotation
+@PublicApi
+public @interface JsonOnlyNonNullValueSerialization


This name seems quite too long to me 😅 How about JsonNonNull?

FrankChen021 · 2021-04-09T02:22:36Z

I have a question. Users have no idea of the size of payload, are there any suggestions that tell users how to set this configuration ? Can we skip storing payload based on value of type ?

In another PR, I am adding an auto cleanup based on size limit for the druid_audit metadata table. This work similar to druid.indexer.logs.kill.* which periodically clean up task entries in the druid_tasks table. Basically in my other PR, user will be able to configured the max number of entries they want to store in the druid_audit metadata table, and Druid will automatically remove from oldest entry (FIFO) when the table entries exceed this size limit. Using both of these configuration (this PR and the other PR), Druid operator will be able to control the size of the audit metadata table. For example, if you want to limit your audit table metadata to 20GB (i.e. that is how much storage you have on your machine), and say you want keep x number of last audit entries, you can then set maxPayloadSizeBytes to 20GB / x
This make sure that the audit table will not grow uncontrollably and bringing down / effecting your whole Druid cluster.

That makes sense. Do you have any plan to put these explanation in doc by your another PR ?

maytasm · 2021-04-09T02:26:02Z

I have a question. Users have no idea of the size of payload, are there any suggestions that tell users how to set this configuration ? Can we skip storing payload based on value of type ?

In another PR, I am adding an auto cleanup based on size limit for the druid_audit metadata table. This work similar to druid.indexer.logs.kill.* which periodically clean up task entries in the druid_tasks table. Basically in my other PR, user will be able to configured the max number of entries they want to store in the druid_audit metadata table, and Druid will automatically remove from oldest entry (FIFO) when the table entries exceed this size limit. Using both of these configuration (this PR and the other PR), Druid operator will be able to control the size of the audit metadata table. For example, if you want to limit your audit table metadata to 20GB (i.e. that is how much storage you have on your machine), and say you want keep x number of last audit entries, you can then set maxPayloadSizeBytes to 20GB / x
This make sure that the audit table will not grow uncontrollably and bringing down / effecting your whole Druid cluster.

That makes sense. Do you have any plan to put these explanation in doc by your another PR ?

Yep. This is the other PR #11084. Right now it only has the doc change and the code implementation is coming soon. Please let me know if you think any docs is missing. I will add a section on how both configs can be use together to control the size of the metadata tables.

jihoonson · 2021-04-09T20:37:44Z

core/src/main/java/org/apache/druid/audit/AuditManager.java

@@ -32,17 +33,21 @@
   * This String is the default message stored instead of the actual audit payload if the audit payload size
   * exceeded the maximum size limit configuration
   */
-  String PAYLOAD_SKIP_MESSAGE = "Payload was not stored as the payload size exceed limit configured by druid.audit.manager.maxPayloadSizeBytes";
+  String PAYLOAD_SKIP_MESSAGE = "Payload was not stored as its size exceeds the limit [%d] configured by druid.audit.manager.maxPayloadSizeBytes";


Suggest PAYLOAD_SKIP_MSG_FORMAT.

jihoonson · 2021-04-09T20:46:47Z

server/src/main/java/org/apache/druid/server/audit/SQLAuditManagerConfig.java

@@ -32,7 +33,10 @@
  private boolean includePayloadAsDimensionInMetric = false;

  @JsonProperty
-  private long maxPayloadSizeBytes = -1;
+  private HumanReadableBytes maxPayloadSizeBytes = HumanReadableBytes.valueOf(-1);


I think maxPayloadSizeBytes should not be a negative number than -1. It would be nice to have a sanity check that checks the lower and upper bounds (to avoid overflow) using @HumanReadableBytesRange as @FrankChen021 suggested in #11078 (comment).

Done. Also updated the docs.

jihoonson · 2021-04-09T21:02:27Z

docs/configuration/index.md

-|`druid.audit.manager.maxPayloadSizeBytes`|The maximum size of audit payload, in bytes, to store in Druid's metadata store audit table. If the size of audit payload exceed this value, the audit log would be stored with a messaging indicating that the payload was omitted instead. Setting `maxPayloadSizeBytes` to -1 (default value) disables this check, meaning Druid will always store audit payload regardless of it's size.|-1|
-
+|`druid.audit.manager.maxPayloadSizeBytes`|The maximum size of audit payload, in bytes, to store in Druid's metadata store audit table. If the size of audit payload exceed this value, the audit log would be stored with a messaging indicating that the payload was omitted instead. Setting `maxPayloadSizeBytes` to -1 (default value) disables this check, meaning Druid will always store audit payload regardless of it's size. Human-readable format is supported, see [here](human-readable-byte.md).  |-1|
+|`druid.audit.manager.skipNullField`|If true, the audit payload stored in metadata store will be exclude any field with null value. |false|


will be exclude -> will exclude?

jihoonson · 2021-04-09T21:05:08Z

server/src/main/java/org/apache/druid/server/audit/SQLAuditManagerConfig.java

@@ -31,6 +31,9 @@
  @JsonProperty
  private boolean includePayloadAsDimensionInMetric = false;

+  @JsonProperty
+  private long maxPayloadSizeBytes = -1;


@FrankChen021 oh you are right. There is a class for unit tests. Sorry, a filter that excludes all test classes might be on in my intellij when I searched last time 😓

jihoonson

Thanks for the quick update. I left a couple of trivial comments.

jihoonson

LGTM. I left a suggestion on typos in docs.

docs/configuration/index.md

maytasm · 2021-04-12T18:32:27Z

@FrankChen021 I have changed the config to HumanReadableBytes. Do you have any other concern or blocker before merging this PR? Thanks!

suneet-s

The configs, defaults and approach LGTM

I have not dug into tests, but since this design review, another committer can confirm this.

Co-authored-by: Jihoon Son <jihoonson@apache.org>

jihoonson

Thanks! +1 after CI.

maytasm · 2021-04-14T03:18:21Z

@jihoonson @suneet-s @FrankChen021 Thanks all for the review.

@FrankChen021 Let me know if you have any followup comments or concerns.

maytasm added 2 commits April 6, 2021 22:53

Add config to skip storing audit payload if exceed limit

b22ae81

fix checkstyle

fae67e1

kfaraz reviewed Apr 7, 2021

View reviewed changes

mghosh4 approved these changes Apr 7, 2021

View reviewed changes

maytasm added 3 commits April 7, 2021 10:54

change config name

ff1aab5

skip null fields for audit payload

adc3731

fix checkstyle

a3aec87

maytasm changed the title ~~Introduce a new configuration that skip storing audit payload if payload size exceed limit~~ Introduce a new configuration that skip storing audit payload if payload size exceed limit and skip storing null fields for audit payload Apr 7, 2021

jihoonson added the Design Review label Apr 7, 2021

suneet-s reviewed Apr 7, 2021

View reviewed changes

maytasm added 2 commits April 7, 2021 18:05

address comments

9fd2385

fix guice

4317700

FrankChen021 reviewed Apr 8, 2021

View reviewed changes

fix test

cdc45b7

add tests

563b1d0

maytasm added Area - Operations Release Notes labels Apr 8, 2021

jihoonson reviewed Apr 8, 2021

View reviewed changes

maytasm added 6 commits April 8, 2021 19:37

address comments

73da5da

address comments

ba77a16

address comments

f09beb0

fix checkstyle

a4ec898

address comments

48ebcb3

fix test

604ab8c

fix test

d6b53f8

jihoonson reviewed Apr 9, 2021

View reviewed changes

address comments

acaa6a4

jihoonson reviewed Apr 12, 2021

View reviewed changes

docs/configuration/index.md Outdated Show resolved Hide resolved

suneet-s approved these changes Apr 12, 2021

View reviewed changes

Address comments

34c6005

Co-authored-by: Jihoon Son <jihoonson@apache.org>

jihoonson approved these changes Apr 13, 2021

View reviewed changes

maytasm merged commit f968400 into apache:master Apr 14, 2021

maytasm deleted the IMPLY-2505 branch April 14, 2021 03:18

techdocsmith mentioned this pull request May 12, 2021

add docs for high-churn datasource cleanup #11245

Merged

1 task

maytasm mentioned this pull request May 14, 2021

Auto cleanup of metadata tables to enable high frequency creation/deletion of data sources #11173

Closed

clintropolis added this to the 0.22.0 milestone Aug 12, 2021

clintropolis mentioned this pull request Sep 3, 2021

[Draft] 0.22.0 Release Notes #11657

Closed

Introduce a new configuration that skip storing audit payload if payload size exceed limit and skip storing null fields for audit payload #11078

Introduce a new configuration that skip storing audit payload if payload size exceed limit and skip storing null fields for audit payload #11078

Conversation

maytasm commented Apr 7, 2021 • edited Loading

Description

kfaraz left a comment • edited Loading

Choose a reason for hiding this comment

maytasm commented Apr 7, 2021

jihoonson commented Apr 7, 2021

suneet-s left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suneet-s Apr 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maytasm Apr 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maytasm Apr 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FrankChen021 commented Apr 8, 2021

maytasm commented Apr 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FrankChen021 commented Apr 9, 2021

maytasm commented Apr 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jihoonson left a comment

Choose a reason for hiding this comment

jihoonson left a comment

Choose a reason for hiding this comment

maytasm commented Apr 12, 2021

suneet-s left a comment

Choose a reason for hiding this comment

jihoonson left a comment

Choose a reason for hiding this comment

maytasm commented Apr 14, 2021

maytasm commented Apr 7, 2021 •

edited

Loading

kfaraz left a comment •

edited

Loading

suneet-s Apr 7, 2021 •

edited

Loading

maytasm Apr 8, 2021 •

edited

Loading

maytasm Apr 9, 2021 •

edited

Loading

maytasm commented Apr 8, 2021 •

edited

Loading