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

[eldwrjwt]

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]

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! 💯 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:

logging-log4j2/log4j-api/src/main/java/org/apache/logging/log4j/spi/AbstractLogger.java

Line 2861 in e1715dc

tryLogMessage(fqcn, getLocation(fqcn), level, marker, message, throwable);

logging-log4j2/log4j-api/src/main/java/org/apache/logging/log4j/spi/AbstractLogger.java

Lines 2918 to 2920 in e1715dc

	private StackTraceElement getLocation(final String fqcn) {
	return requiresLocation() ? StackLocatorUtil.calcLocation(fqcn) : null;
	}

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

logging-log4j2/log4j-async-logger/src/main/java/org/apache/logging/log4j/async/logger/RingBufferLogEventTranslator.java

Lines 61 to 87 in 7fd3a13

	public void translateTo(final RingBufferLogEvent event, final long sequence) {
	try {
	final StringMap contextData = event.getContextData();
	// Compute location if necessary
	event.setValues(
	asyncLogger,
	loggerName,
	marker,
	fqcn,
	level,
	message,
	thrown,
	// config properties are taken care of in the EventHandler thread
	// in the AsyncLogger#actualAsyncLog method
	contextDataInjector.injectContextData(null, contextData),
	contextStack,
	threadId,
	threadName,
	threadPriority,
	// compute location if necessary
	requiresLocation ? StackLocatorUtil.calcLocation(fqcn) : location,
	clock,
	nanoClock);
	} finally {
	clear(); // clear the translator
	}
	}

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]

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]

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]

PR Submitted in #3259 for FailoverAppender