Skip to content

Conversation

@mgoworko
Copy link
Contributor

@mgoworko mgoworko commented Nov 27, 2025

Summary by CodeRabbit

  • New Features

    • Expanded audit logging to support multiple output destinations (index and file)
    • Added dedicated search and indexing slowlog outputs for performance tracking
  • Chores

    • Enhanced log rotation and retention with size- and age-based cleanup (39.5GB / 7 days)
    • Increased action-level logging verbosity
    • Updated a helper script's interpreter for improved shell compatibility

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai

This comment was marked as off-topic.

coderabbitai[bot]

This comment was marked as off-topic.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
ror-demo-cluster/conf/es/log4j2.properties (1)

29-39: File pattern naming strategy differs from main rolling appender.

The readonlyrest_audit_rolling appender uses index-only naming (readonlyrest_audit-%i.log.gz), while the main rolling appender at line 53 uses date-and-index naming (${sys:es.logs.cluster_name}-%d{yyyy-MM-dd}-%i.log.gz). This inconsistency may be intentional, but consider documenting the design rationale.

Similarly, lines 79, 95, and 111 each use different naming strategies (index-only, date-only, respectively). If this reflects deliberate separation by log type, a brief comment explaining the strategy would improve maintainability.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7cff1e3 and 3ec8539.

📒 Files selected for processing (1)
  • ror-demo-cluster/conf/es/log4j2.properties (1 hunks)

Comment on lines +122 to +136
#appender.rolling.strategy.max = 50
appender.rolling.strategy.type = DefaultRolloverStrategy
appender.rolling.strategy.action.type = Delete
appender.rolling.strategy.action.basepath = ${sys:es.logs.base_path}
appender.rolling.strategy.action.PathConditions.type = IfFileName
appender.rolling.strategy.action.PathConditions.glob = ${sys:es.logs.cluster_name}*

appender.rolling.strategy.action.condition.type = IfFileName
appender.rolling.strategy.action.condition.glob = ${sys:es.logs.cluster_name}*
#delete old logs after 7 days or when exceeding 39.5 GB
appender.rolling.strategy.action.condition.nested_condition.type = IfAny
appender.rolling.strategy.action.condition.nested_condition.fileSize.type = IfAccumulatedFileSize
appender.rolling.strategy.action.condition.nested_condition.fileSize.exceeds = 39.5GB
appender.rolling.strategy.action.condition.nested_condition.lastMod.type = IfLastModified
appender.rolling.strategy.action.condition.nested_condition.lastMod.age = 7D
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🌐 Web query:

log4j2 Delete action RollingFileAppender PathConditions condition syntax

💡 Result:

Summary of Delete action PathCondition syntax for RollingFileAppender (log4j2)

  • Delete element: placed inside a RolloverStrategy (DefaultRolloverStrategy, DirectWriteRolloverStrategy, etc.). Key attrs: basePath, maxDepth, followLinks, testMode. [1][5]

  • Basic PathConditions:

    • IfFileName: filters by file name; supports attributes glob="pattern" or regex="pattern". Example: . [1][5]
    • IfLastModified: filters by modification age; use ISO-8601 period or shorthand (examples in docs show "60d" or "P60D"/"P15D" notation). Example: or . [1][3][5]
    • IfAccumulatedFileSize / IfAny / IfAll / IfNot: composite and size-based conditions are available; nested conditions are evaluated only if the outer condition accepts the file. [1][5]
  • Combining conditions:

    • Multiple non-nested conditions: all must accept the file (logical AND).
    • Use composite wrappers: (AND), (OR), (NOT) to combine conditions explicitly. Nested PathConditions inside these elements are evaluated per the composite logic. [1][5]
  • ScriptCondition: you can supply a ScriptCondition (Groovy, JavaScript, etc.) that returns the list/boolean to select files; when present, PathCondition elements are ignored. Useful for more complex rules (including deleting empty directories). [1][2][3]

  • Test mode and safety:

    • testMode="true" on Delete prints actions to status logger instead of deleting — use to validate rules. Also be careful: Delete can remove any file under basePath. [1]
  • Example (XML):

    (This deletes archived files matching the glob older than 60 days.) [1][5]

References
[1] Apache Log4j 2 Appenders — Delete action docs (examples & param list).
[2] Apache Log4j 2 Rolling-file manual (Delete, ScriptCondition examples).
[3] HowToDoInJava Log4j2 RollingFileAppender — Delete examples.


🌐 Web query:

log4j2 properties format Delete action configuration DefaultRolloverStrategy nested_condition

💡 Result:

Yes — you can configure the Delete action (inside DefaultRolloverStrategy) with nested PathConditions in log4j2.properties. Use the action.type = Delete, then express the outer condition(s) and any nested conditions with the nested_condition namespace (indexed where needed). Example:

  • Simple: delete files matching a glob and older than 30 days
    appender.rolling.strategy.type = DefaultRolloverStrategy
    appender.rolling.strategy.action.type = Delete
    appender.rolling.strategy.action.basePath = ${baseDir}
    appender.rolling.strategy.action.maxDepth = 2
    appender.rolling.strategy.action.condition.type = IfFileName
    appender.rolling.strategy.action.condition.glob = /app-.log.gz
    appender.rolling.strategy.action.condition.nested_condition.type = IfLastModified
    appender.rolling.strategy.action.condition.nested_condition.age = 30D

  • Example using an inner IfAny (logical OR) with two inner conditions (IfAccumulatedFileSize OR IfLastModified):
    appender.rolling.strategy.type = DefaultRolloverStrategy
    appender.rolling.strategy.action.type = Delete
    appender.rolling.strategy.action.basePath = ${baseDir}
    appender.rolling.strategy.action.maxDepth = 2
    appender.rolling.strategy.action.condition.type = IfFileName
    appender.rolling.strategy.action.condition.glob = /app-.log.gz
    appender.rolling.strategy.action.condition.nested_condition.type = IfAny
    appender.rolling.strategy.action.condition.nested_condition.0.type = IfAccumulatedFileSize
    appender.rolling.strategy.action.condition.nested_condition.0.exceeds = 100GB
    appender.rolling.strategy.action.condition.nested_condition.1.type = IfLastModified
    appender.rolling.strategy.action.condition.nested_condition.1.age = 30D

Notes:

  • Outer condition (e.g., IfFileName) is evaluated first; nestedConditions are evaluated only for files accepted by the outer condition. Nested conditions can themselves be compound (IfAny / IfAll) or specific path/accumulated conditions. See Log4j2 docs for condition parameter names and semantics. [1][2][3]

Sources:
[1] Apache Log4j2 RollingFile/Delete examples and XML/Properties guidance. [2] Log4j2 manual: nestedConditions documentation. [3] Community examples showing properties-format nested_condition indexing (StackOverflow).


Remove the non-standard PathConditions configuration at lines 126–127.

Lines 126–127 define appender.rolling.strategy.action.PathConditions.type and .glob, which are not valid log4j2 properties-format keys for the Delete action. The correct structure uses appender.rolling.strategy.action.condition (lines 129–136), which already defines the file-matching logic. Delete the redundant lines 126–127; the nested condition structure at lines 129–136 is correct and properly chains the glob pattern match with the IfAny composite condition for size and age checks.

🤖 Prompt for AI Agents
In ror-demo-cluster/conf/es/log4j2.properties around lines 122 to 136, remove
the two non-standard properties
appender.rolling.strategy.action.PathConditions.type and
appender.rolling.strategy.action.PathConditions.glob (lines 126–127) because
they are not valid for the Delete action; the correct file-matching is already
defined by appender.rolling.strategy.action.condition and its nested condition
block (lines 129–136), so delete the redundant PathConditions lines and keep the
existing condition/nested_condition structure intact.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants