[GitHub] [druid] jihoonson commented on a change in pull request #11078: Introduce a new configuration that skip storing audit payload if payload size exceed limit and skip storing null fields for audit payload

[GitBox <gi...@apache.org>]

jihoonson commented on a change in pull request #11078:
URL: https://github.com/apache/druid/pull/11078#discussion_r610144591



##########
File path: core/src/main/java/org/apache/druid/common/config/ConfigSerde.java
##########
@@ -24,6 +24,14 @@
 public interface ConfigSerde<T>
 {
   byte[] serialize(T obj);
-  String serializeToString(T obj);
+  /**
+   * Serialize object to String
+   *
+   * @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

Review comment:
       Is this true only when `skipNull` is true?

##########
File path: core/src/main/java/org/apache/druid/audit/AuditManager.java
##########
@@ -28,6 +28,12 @@
 
 public interface AuditManager
 {
+  /**
+   * 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";

Review comment:
       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`.

##########
File path: core/src/main/java/org/apache/druid/guice/annotations/JsonOnlyNonNullValueSerialization.java
##########
@@ -0,0 +1,37 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.guice.annotations;
+
+import com.google.inject.BindingAnnotation;
+
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+
+/**

Review comment:
       Javadoc would be nice.

##########
File path: 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))

Review comment:
       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.

##########
File path: core/src/main/java/org/apache/druid/guice/annotations/JsonOnlyNonNullValueSerialization.java
##########
@@ -0,0 +1,37 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.guice.annotations;
+
+import com.google.inject.BindingAnnotation;
+
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+
+/**
+ */
+@Target({ElementType.FIELD, ElementType.PARAMETER, ElementType.METHOD})
+@Retention(RetentionPolicy.RUNTIME)
+@BindingAnnotation
+@PublicApi
+public @interface JsonOnlyNonNullValueSerialization

Review comment:
       This name seems quite too long to me :sweat_smile: How about `JsonNonNull`? 

##########
File path: 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;

Review comment:
       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 :slightly_smiling_face:.
   
   > 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.




-- 
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