Fixes #28770: write server audit entries to audit.log#28771
Open
harshach wants to merge 2 commits into
Open
Conversation
Audit-to-file logging silently broke in #23733: gutting AuditEventHandler removed the only emitter of the AUDIT log marker, so the audit.log appender (audit-only-filter-factory) creates the file but never writes to it. Re-emit each audit entry through the AUDIT-marked logger from AuditLogRepository.write(), gated on a successful (non-duplicate) DB insert so the publisher+consumer dual-write path does not double-log. Emit a compact one-line summary at INFO rather than the full ChangeEvent JSON. Harden conf/openmetadata.yaml: pin the audit logger to INFO so capture is independent of LOG_LEVEL, and exclude audit lines from the console appender so they land only in audit.log. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
✅ PR checks passedThe linked issue has a description and all required Shipping project fields set. Thanks! |
Contributor
🟡 Playwright Results — all passed (13 flaky)✅ 4276 passed · ❌ 0 failed · 🟡 13 flaky · ⏭️ 88 skipped
🟡 13 flaky test(s) (passed on retry)
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
Resolved the add/add conflict in AuditLogRepositoryTest.java by combining this branch's audit-marker emission + dedup-gate tests with the lineage-persistence test added in #28426. AuditLogRepository.java and CollectionDAO.java auto-merged cleanly: main's lineage event types and IntakeForm DAO coexist with the insert-gated audit-to-file logging. Verified: 16/16 audit/logging tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Code Review ✅ ApprovedRestores audit logging by re-emitting events through the AUDIT-marked logger within the AuditLogRepository, ensuring exactly-once writes by gating on successful database inserts. No issues found. OptionsDisplay: compact → Showing less information. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | Gitar |
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.



Describe your changes:
Fixes #28770
Server audit logging to
audit.logsilently broke in #23733: guttingAuditEventHandlerremoved the only emitter of theAUDITSLF4J marker, so theaudit.logappender (guarded byaudit-only-filter-factory) still creates the file at startup but never writes a line to it. I re-emit each audit entry through theAUDIT-marked logger fromAuditLogRepository.write()— the single chokepoint all audit records flow through — gated on a successful, non-duplicate DB insert so the publisher + consumer dual-write path (and multi-server replays) can't double-log. The emitted line is a compact one-line summary atINFO, not the fullChangeEventJSON (which can be 1–2 MB/event).Type of change:
High-level design:
AuditLogDAO.insertnow returns rows-affected (int); since the insert is idempotent (ON CONFLICT DO NOTHING/INSERT IGNORE), the file line is emitted only when> 0, giving exactly-once file logging that matches the DB's exactly-once semantics. Config hardening inconf/openmetadata.yaml: pin the audit logger toINFOso capture is independent of the globalLOG_LEVEL, and addaudit-exclude-filter-factoryto the console appender so audit lines land only inaudit.log. The DB-backed audit subsystem (REST API + UI) is unchanged.Tests:
Use cases covered
audit.log.Unit tests
openmetadata-service/src/test/java/org/openmetadata/service/audit/AuditLogRepositoryTest.java— asserts theAUDITmarker is emitted on insert, and suppressed when the insert is de-duplicated (gate). Pure unit tests (mocked DAO + in-memory logbackListAppender), no DB/container.Backend integration tests
AuditLogResourceITcovers the DB-backed path).Ingestion integration tests
Playwright (UI) tests
Manual testing performed
mvn -pl openmetadata-service test -Dtest=AuditLogRepositoryTest— 2/2 pass; observed theINFO+AUDITsummary line emitted on insert and suppressed on dedup.mvn -pl openmetadata-service test -Dtest=LoggingConfigurationYamlTest— 2/2 pass; confirms the editedconf/openmetadata.yamlparses in both text and JSON logging modes.mvn spotless:apply -pl openmetadata-service— clean.UI screen recording / screenshots:
Not applicable.
Checklist:
Fixes <issue-number>: <short explanation>Fixes #<issue-number>above.