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.

Sign up for GitHub

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

Jump to bottom

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

Open
eldwrjwt opened this issue Nov 29, 2024 · 5 comments · May be fixed by #3259 or #3260
Open

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

eldwrjwt opened this issue Nov 29, 2024 · 5 comments · May be fixed by #3259 or #3260
Labels
async Affects asynchronous loggers or appenders bug Incorrect, unexpected, or unintended behavior of existing code waiting-for-maintainer

Comments

@eldwrjwt
Copy link

eldwrjwt commented Nov 29, 2024 •
edited
Loading

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

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.

@ppkarwasz ppkarwasz added bug Incorrect, unexpected, or unintended behavior of existing code async Affects asynchronous loggers or appenders and removed waiting-for-maintainer labels Nov 29, 2024
@eldwrjwt
Copy link
Author

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

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 added a commit to eldwrjwt/logging-log4j2 that referenced this issue Nov 30, 2024
@eldwrjwt
Add requiresLocation function for FailoverAppender (apache#3257)
7ad2a70
eldwrjwt added a commit to eldwrjwt/logging-log4j2 that referenced this issue Nov 30, 2024
@eldwrjwt
Add requiresLocation function for FailoverAppender (apache#3257)
5452eaf
eldwrjwt added a commit to eldwrjwt/logging-log4j2 that referenced this issue Nov 30, 2024
@eldwrjwt
Add requiresLocation function for FailoverAppender (apache#3257)
87147db
@eldwrjwt eldwrjwt linked a pull request Nov 30, 2024 that will close this issue
Open
@eldwrjwt
Copy link
Author

PR Submitted in #3259 for FailoverAppender

eldwrjwt added a commit to eldwrjwt/logging-log4j2 that referenced this issue Dec 1, 2024
@eldwrjwt
Add requiresLocation function for AsyncAppender (apache#3257)
68380f1
@eldwrjwt eldwrjwt linked a pull request Dec 1, 2024 that will close this issue
Open
@eldwrjwt
Copy link
Author

eldwrjwt commented Dec 1, 2024

Another PR #3260 for AsyncAppender

eldwrjwt added a commit to eldwrjwt/logging-log4j2 that referenced this issue Dec 2, 2024
@eldwrjwt
Add requiresLocation function for AsyncAppender (apache#3257)
3fa5de4
eldwrjwt added a commit to eldwrjwt/logging-log4j2 that referenced this issue Dec 4, 2024
@eldwrjwt
Add requiresLocation function for FailoverAppender (apache#3257)
1b68944
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
async Affects asynchronous loggers or appenders bug Incorrect, unexpected, or unintended behavior of existing code waiting-for-maintainer
Projects
None yet
Milestone
No milestone
2 participants
@ppkarwasz @eldwrjwt