Copilot commented on code in PR #11343:
URL: https://github.com/apache/gravitino/pull/11343#discussion_r3370789421
##########
docs/gravitino-server-config.md:
##########
@@ -259,7 +259,7 @@ Gravitino provides a default implementation to log basic
audit information to a
#### Audit log formatter
-The `Formatter` interface transforms an `Event` into an `AuditLog`.
`SimpleFormatterV2` is the default implementation and requires no extra
configuration. It produces a tab-separated line with the following fields:
timestamp, user, operation type, identifier, operation status, event source,
remote address, and custom info.
+The `Formatter` interface transforms an `Event` into an `AuditLog`.
`SimpleFormatterV2` is the default implementation. `JsonAuditFormatter` is also
available when structured JSON output is required. It emits one JSON object per
line, serializes `customInfo`, and formats `timestamp` as ISO 8601 with
millisecond precision and zone offset. Both simple and JSON formatters mask
sensitive values such as `Authorization`, `Cookie`, `X-Amz-Security-Token`,
`s3.access-key-id`, and `jdbc-password`.
Review Comment:
This documentation says `SimpleFormatterV2` is the default audit formatter,
but the PR description/issue state the default should switch to
`JsonAuditFormatter`. Please align the docs with the actual default behavior
(either update the default to JSON in code/config, or adjust the PR description
if the default is intentionally unchanged).
##########
dev/charts/gravitino/resources/config/gravitino.conf:
##########
@@ -173,7 +173,7 @@ gravitino.iceberg-rest.default-catalog-name = {{
.Values.icebergRest.dynamicConf
# Audit log configuration
gravitino.audit.enabled = {{ .Values.audit.enabled }}
gravitino.audit.writer.className = {{ if (and .Values.audit
.Values.audit.writer .Values.audit.writer.className) }}{{
.Values.audit.writer.className }}{{ else
}}org.apache.gravitino.audit.FileAuditWriter{{- end }}
-gravitino.audit.formatter.className = {{ if (and .Values.audit
.Values.audit.formatter .Values.audit.formatter.className) }}{{
.Values.audit.formatter.className }}{{ else
}}org.apache.gravitino.audit.SimpleFormatter{{- end }}
+gravitino.audit.formatter.className = {{ if (and .Values.audit
.Values.audit.formatter .Values.audit.formatter.className) }}{{
.Values.audit.formatter.className }}{{ else
}}org.apache.gravitino.audit.v2.SimpleFormatterV2{{- end }}
Review Comment:
The Helm chart template still defaults `gravitino.audit.formatter.className`
to `SimpleFormatterV2`, but the PR description/issue say the default should
switch to `JsonAuditFormatter`. If the intent is to make JSON the new default,
update the template default accordingly (and ensure the core config default is
also updated).
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
