File line numbers are not printed for the failover appender when using AsyncLoggerContextSelector

eldwrjwt commented 2 days ago

Description

I was using a failover appender with an AsyncLoggerContextSelector and wanted to print the line number. I set includeLocation to true in the root logger, but there were still no line numbers in the logs. We encountered the same problem with the async appender, even with includeLocation set to true.

I believe this is because the requiresLocation function in FailoverAppender is the default implementation in AbstractAppender, which relies on the layout inside the appender. However, for FailoverAppender, we do not have a layout, and thus fail to generate the location field in the log event object. Additionally, the Async logger uses a RingBufferLogEvent, which directly reads the location field, so there's no location to print. This issue is also true for AsyncAppender.

Configuration

Version: 2.24.2

Operating system: Windows11

JDK: OpenJDK23

Reproduction

Log4j2.xml

<Configuration>
    <Appenders>
        <Console name="CONSOLE">
            <PatternLayout pattern="%m%L%n"/>
        </Console>

        <Console name="CONSOLE2">
            <PatternLayout pattern="%m%L%n"/>
        </Console>

        <Failover name="Failover" primary="CONSOLE">
            <Failovers>
                <AppenderRef ref="CONSOLE2"/>
            </Failovers>
        </Failover>
    </Appenders>
    <Loggers>
        <Root level="INFO" includeLocation="true">
            <AppenderRef ref="Failover"/>
        </Root>
    </Loggers>
</Configuration>

log4j2.component.properties

Log4jContextSelector=org.apache.logging.log4j.core.async.AsyncLoggerContextSelector

ppkarwasz commented 2 days ago

Hi @eldwrjwt,

I believe this is because the requiresLocation function in FailoverAppender is the default implementation in AbstractAppender, which relies on the layout inside the appender. However, for FailoverAppender, we do not have a layout, and thus fail to generate the location field in the log event object.

Nice catch! :100: Can you submit a PR that fixes it?

Additionally, the Async logger uses a RingBufferLogEvent, which directly reads the location field, so there's no location to print.

The shouldn't be a problem:

Log4j Core 2 uses the o.a.l.l.spi.AbstractLogger class from Log4j API, which populates the value of the location early:

https://github.com/apache/logging-log4j2/blob/e1715dc9f3132c70f1a87a6a75058d02a0b69d63/log4j-api/src/main/java/org/apache/logging/log4j/spi/AbstractLogger.java#L2861

https://github.com/apache/logging-log4j2/blob/e1715dc9f3132c70f1a87a6a75058d02a0b69d63/log4j-api/src/main/java/org/apache/logging/log4j/spi/AbstractLogger.java#L2918-L2920

Log4j Core 3 populates the location just before the asynchronous barrier to give filters a chance to discard it:

https://github.com/apache/logging-log4j2/blob/7fd3a1303dec3ede639dd573851143c5ac0890e1/log4j-async-logger/src/main/java/org/apache/logging/log4j/async/logger/RingBufferLogEventTranslator.java#L61-L87

In both cases the location field is populated if requiresLocation() returns true.

This issue is also true for AsyncAppender.

I agree, we have the same problem as in Failover here: the AsyncAppender.requiresLocation() implementation is incorrect. Could you submit a separate PR for this one?

Note that AsyncAppender has an additional twist to make things interesting: it has an includeLocation configuration attribute that must be turned on to compute location. The implementation of requiresLocation() for the Async appender should probably be: includeLocation is true and any of the downstream appenders has requiresLocation() == true.

Remark: If location information is important to you, since last year we have a Log4j Transform Maven plugin, which precomputes the location of your log statements at build time. If the location is precomputed, it will be available regardless of any includeLocation setting: these settings prevent the computation of location information at runtime, they don't delete the information that is already there.

eldwrjwt commented 2 days ago

Hi @ppkarwasz, I can submit a PR to fix the issue later on. If my understanding is correct, I think I can write a correct implementation of requiresLocation() for FailoverAppender (and AsyncAppender in another PR), and this should fix the bug.

ppkarwasz commented 2 days ago

Hi @ppkarwasz, I can submit a PR to fix the issue later on. If my understanding is correct, I think I can write a correct implementation of requiresLocation() for FailoverAppender (and AsyncAppender in another PR), and this should fix the bug.

That's great! Yes, requiresLocation() is the method that needs fixing. Please add some unit tests too.

eldwrjwt commented 1 day ago

PR Submitted in #3259 for FailoverAppender

eldwrjwt commented 19 hours ago

Another PR #3260 for AsyncAppender

apache / logging-log4j2