Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove logback RegexFilterAction, not supported any more. #5193

Merged
merged 4 commits into from
Jan 20, 2024

Conversation

soloturn
Copy link
Contributor

@soloturn soloturn commented Dec 6, 2023

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

@soloturn soloturn marked this pull request as draft December 6, 2023 07:23
@jdrueckert
Copy link
Member

@soloturn You can have a look at #5022 for some context and motivation.

@soloturn
Copy link
Contributor Author

soloturn commented Dec 7, 2023

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
Copy link
Contributor Author

soloturn commented Dec 9, 2023

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
Copy link
Member

@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
Copy link
Contributor Author

soloturn commented Dec 19, 2023

what was the outcome of the meeting @jdrueckert ?

@jdrueckert jdrueckert marked this pull request as ready for review January 16, 2024 21:14
@jdrueckert
Copy link
Member

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.

@BenjaminAmos BenjaminAmos merged commit 3e9b5b6 into develop Jan 20, 2024
9 checks passed
@BenjaminAmos BenjaminAmos deleted the qa/remove-outdated-logback branch January 20, 2024 23:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

3 participants