Remove unused audit log file rotation settings from docs#8771
Remove unused audit log file rotation settings from docs#8771
Conversation
From Mattermost server v11.4, the following ExperimentalAuditSettings file rotation settings were removed as unused (mattermost/mattermost#35170): - FileMaxSizeMB - FileMaxAgeDays - FileMaxBackups - FileCompress File rotation for audit logs is now configured via AdvancedLoggingJSON. Updated files: - environment-configuration-settings.rst: Remove 4 rotation setting sections - experimental-configuration-settings.rst: Remove 4 rotation setting sections; update description to direct admins to AdvancedLoggingJSON for file rotation - logging.rst: Remove rotation settings from Cloud defaults list - telemetry.rst: Remove rotation settings from telemetry data list Co-authored-by: Combs7th <[email protected]>
|
@hanzei - Tagging you for dev review. Do the changes above look accurate? If so, I'll tag Eric for editor review. |
|
Newest code from mattermost has been published to preview environment for Git SHA f141ce0 |
|
@Combs7th This is also scheduled for v11.6 (April release). |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
|
@claude - Please revise the docs update based on the PR evidence and developer feedback. Before drafting:
What to do:
|
|
Claude finished @Combs7th's task in 2m 57s —— View job Revised audit log docs — all 5 removed fields addressed
Summary of changes (commit
The previous PR commit had removed the 4 rotation settings ( |
Also remove FileMaxQueueSize from all doc locations: - environment-configuration-settings.rst: remove queue size section, add AdvancedLoggingJSON pointer note - experimental-configuration-settings.rst: fix intro text (was "file name and queue size", now "file name only"), remove queue size section - logging.rst: remove FileMaxQueueSize from cloud defaults list - telemetry.rst: remove enum FileMaxQueueSize from ExperimentalAuditSettings Aligns with mattermost/mattermost#35170 which removes 5 fields from ExperimentalAuditSettings: FileMaxSizeMB, FileMaxAgeDays, FileMaxBackups, FileCompress, FileMaxQueueSize. Co-authored-by: Combs7th <[email protected]>
|
Newest code from mattermost has been published to preview environment for Git SHA 5df6d13 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughRemoved deprecated self‑hosted audit log file rotation configuration entries from multiple admin docs and replaced specific rotation/retention guidance with a reference to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
source/administration-guide/manage/telemetry.rst (1)
1-1:⚠️ Potential issue | 🟡 MinorConsider broader telemetry documentation strategy.
A previous review comment indicates telemetry is no longer collected ("We don't do telemetry any longer"). Additionally, the changes to this file (removing individual
ServiceSettingsenumeration fields) appear outside the stated PR scope of removing audit log rotation settings documentation.Before merging, clarify whether:
- The entire telemetry file should be deprecated/removed rather than maintained
- These enum field removals are intentional or accidentally included in this PR
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/administration-guide/manage/telemetry.rst` at line 1, The telemetry docs currently state telemetry is not collected and the diff shows removals from the ServiceSettings enumeration that fall outside the audit-log-rotation scope; confirm whether the telemetry page (source/administration-guide/manage/telemetry.rst) should be fully deprecated/removed or retained, and if retained revert or remove only the unrelated enum-field edits (references to ServiceSettings enum fields) so this PR only contains audit log rotation doc changes; update the PR with either a removal commit for the telemetry file or a revert/adjustment of the ServiceSettings field deletions and add a short commit message explaining the chosen action.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@source/administration-guide/configure/environment-configuration-settings.rst`:
- Line 3592: The sentence that currently reads as an absolute requirement should
be narrowed to apply only when using the FileEnabled/file-output audit logging
path: update the phrasing around the "enable <#auditlog-fileenabled>`__ audit
logging" reference so it explicitly says the file name must be set when using
the FileEnabled (file output) audit logger; leave references to
AdvancedLoggingJSON and ExperimentalAuditSettings.FileName untouched but note
that those targets may not require ExperimentalAuditSettings.FileName. Target
the text around the "enable <#auditlog-fileenabled>" link and mention
"FileEnabled/file-output path" and "ExperimentalAuditSettings.FileName" so
readers understand the requirement is scoped to the file-output configuration
only.
---
Duplicate comments:
In `@source/administration-guide/manage/telemetry.rst`:
- Line 1: The telemetry docs currently state telemetry is not collected and the
diff shows removals from the ServiceSettings enumeration that fall outside the
audit-log-rotation scope; confirm whether the telemetry page
(source/administration-guide/manage/telemetry.rst) should be fully
deprecated/removed or retained, and if retained revert or remove only the
unrelated enum-field edits (references to ServiceSettings enum fields) so this
PR only contains audit log rotation doc changes; update the PR with either a
removal commit for the telemetry file or a revert/adjustment of the
ServiceSettings field deletions and add a short commit message explaining the
chosen action.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2372fab4-159b-42bd-94e7-6267364f6e57
📒 Files selected for processing (4)
source/administration-guide/configure/environment-configuration-settings.rstsource/administration-guide/configure/experimental-configuration-settings.rstsource/administration-guide/manage/logging.rstsource/administration-guide/manage/telemetry.rst
|
@hanzei - Is this looking more accurate now? |
|
Newest code from mattermost has been published to preview environment for Git SHA 4af6ca7 |
|
Newest code from mattermost has been published to preview environment for Git SHA 8d482b4 |
|
@esethna - I think this one should be good for merge now? |
|
Newest code from mattermost has been published to preview environment for Git SHA 403699c |
|
|
||
| .. _telemetry: | ||
|
|
||
| Telemetry |
There was a problem hiding this comment.
Is it intentional that we are removing this entire page?
I have an 11.6 server and I still see settings for "Error and Diagnostic reporting" and security alerts feature.
I understand that we don't have telemetry in the same way as before, but perhaps this page needs updates, rather than a full removal? @hanzei @Combs7th
There was a problem hiding this comment.
Good call out here, @esethna! I'm gonna try something on a new comment..
|
@claude - Please review all commits and discussion posted in this PR. Context:
Please do this in two passes:
Output:
Important constraints:
|
|
Claude finished @Combs7th's task in 5m 14s —— View job Telemetry page decision — two-pass analysis
Veteran Vince pass — conservative reviewVerdict: Do not delete Here's why: The deletion was triggered by a misread of the dev comment. @hanzei's original note — "We don't do telemetry any longer. Can we just drop the file?" — was made during review of the initial PR, which was removing 4 Eric's v11.6 evidence is conclusive. The page documents two features that demonstrably still exist in v11.6:
Both are distinct from audit logging and both remain product-valid. The only PR-justified change to
CodeRabbit also flagged this independently as "out of scope." Editor Evie pass — cleanest docs approachRecommendation: Keep the page, apply the minimal fix, no structural changes needed for this PR. The page's structure is sound for its purpose — it's a transparency reference that explains what data Mattermost collects and how to opt out. The two main sections (security alerts and error/diagnostic reporting) are clearly written and still accurate. The one thing that's slightly stale but out of scope for this PR:
No wording changes needed beyond the field removals. The page correctly describes opt-out paths and the data collection policy as it still stands. What was doneRestored Follow-up questions for @hanzei / @esethna before merge
|
…Settings fields The page documents features (Security Alerts, Error & Diagnostic Reporting) still present in v11.6. Full deletion was out of PR scope. Only removes FileMaxSizeMB, FileMaxAgeDays, FileMaxBackups, FileCompress, FileMaxQueueSize from the telemetry enumeration list, matching the server-side removal in mattermost/mattermost#35170. Co-authored-by: Combs7th <[email protected]>
|
Newest code from mattermost has been published to preview environment for Git SHA 1d0a041 |
Remove four unused audit log file rotation settings from documentation to reflect Mattermost server v11.4+ behavior.
Engineering PR mattermost/mattermost#35170 removes these settings from
ExperimentalAuditSettings:FileMaxSizeMBFileMaxAgeDaysFileMaxBackupsFileCompressFile rotation for audit logs is now configured via
AdvancedLoggingJSON.Closes #8769
Generated with Claude Code