MovingBlocks / Terasology

Terasology - open source voxel world
http://terasology.org
Apache License 2.0
3.64k stars 1.33k forks source link

remove logback RegexFilterAction, not supported any more. #5193

Closed soloturn closed 5 months ago

soloturn commented 6 months ago

RegexFilterAction blocks the upgrade of logback, and blocked logback upgrade blocks upgrade to slf4j-2.x. slf4j-2 additionally provides a fluent logging API.

the affected classes are used test logging and one real, facades/PC:

❯ git grep "include resource"
engine-tests/src/test/resources/logback-test.xml:  <include resource="org/terasology/engine/logback/setup.xml" />
engine-tests/src/test/resources/logback-test.xml:    <include resource="org/terasology/engine/logback/filter-reflections.xml" />
facades/PC/src/main/resources/logback.xml:  <include resource="org/terasology/engine/logback/setup.xml" />
facades/PC/src/main/resources/logback.xml:    <include resource="org/terasology/engine/logback/filter-reflections.xml" />
templates/module.logback-test.xml:  <include resource="org/terasology/engine/logback/setup.xml" />
templates/module.logback-test.xml:    <include resource="org/terasology/engine/logback/filter-reflections.xml" />

❯ git grep denyReg
engine/src/main/resources/org/terasology/engine/logback/filter-reflections.xml:    <denyRegex prefix="org.reflections" message="given scan urls are empty" />
engine/src/main/resources/org/terasology/engine/logback/filter-reflections.xml:    <denyRegex prefix="org.reflections"
engine/src/main/resources/org/terasology/engine/logback/setup.xml:    <!-- map <denyRegex> and <requireRegex> xml elements to RegexFilter -->
engine/src/main/resources/org/terasology/engine/logback/setup.xml:    <newRule pattern="*/appender/denyRegex" actionClass="org.terasology.logback.RegexFilterAction" />

❯ git grep requireReg
engine/src/main/resources/org/terasology/engine/logback/setup.xml:    <!-- map <denyRegex> and <requireRegex> xml elements to RegexFilter -->
engine/src/main/resources/org/terasology/engine/logback/setup.xml:    <newRule pattern="*/appender/requireRegex" actionClass="org.terasology.logback.RegexFilterAction" />
out of filter-reflections.xml :
    <!-- WONTFIX: gestalt routinely builds empty configs and then adds to them later. -->
    <denyRegex prefix="org.reflections" message="given scan urls are empty" />

    <!-- WONTFIX: these classes don't need to be reflected on,
             and they're only on the classpath during tests. -->
    <denyRegex prefix="org.reflections"
               message="could not get type for name org\.terasology\.benchmark" />

the test runs produce a sliht difference in size, but i cannot really make out any reasonable difference, on the default checkout, running like this, for branch default and [qa/remove-outdated-logback]

❯ gradle clean build --warning-mode=all -D sonar.gradle.skipCompile=true >filter.log 2>&1

❯ ls -la *.log
.rw-r--r-- 308k st  7 Dez 06:55 filter.log
.rw-r--r-- 313k st  7 Dez 06:11 nofilter.log
jdrueckert commented 6 months ago

@soloturn You can have a look at https://github.com/MovingBlocks/Terasology/pull/5022 for some context and motivation.

soloturn commented 6 months ago

ah, thank you @jdrueckert ! @keturn writes

logback 1.2 isn't structured in a way that facilitates making complete logback extensions from logback-core alone; you often have to pull in logback-classic to do anything useful.

why you opted to not use logback 1.4 in 2022, and not use something like slf4j appeders addFilter spi or logbacks filter ? i saw the nice syntax of @keturn implementation.

i updated the text of the ticket, removed the asking of the question, und put the log file sizes with this pull request and without,

soloturn commented 6 months ago

removed now the usage in the logback.xml config files. still am not able to see the difference in the output with and without. so i think we could merge it what you thnk @jdrueckert ?

unrelated to this PR, as preview, i created one branch merging all the qa stuff, including to use fluent logger interface.

jdrueckert commented 6 months ago

@soloturn This is something I'd like to discuss with the others first, they may recall more details about keturn's effort back then... I'll put it on the agenda for our next meeting this Sunday.

soloturn commented 6 months ago

what was the outcome of the meeting @jdrueckert ?

jdrueckert commented 5 months ago

what was the outcome of the meeting @jdrueckert ?

@BenjaminAmos had a closer look at the custom implementation that keturn introduced and the built-in options available. Based on this, we agreed that we can use the built-in options to achieve an equivalent configuration without the custom regex filter that currently blocks the upgrade. We also believe that the upgrade should not impact using the built-in functionality, so we decided that we're good to remove the custom implementation to unblock the upgrade and afterwards go for the upgrade.