Add Logback's throwable-consuming semantics as an option #2363
Comments
|
@SeasonPanPan, could you provide us a minimal minimal: containing only what is necessary to reproduce the issue and nothing more |
|
@vy This is the log4j2.xml <?xml version="1.0" encoding="UTF-8"?>
<Configuration status="WARN">
<Appenders>
<Console name="Console" target="SYSTEM_OUT">
<PatternLayout pattern="%d{HH:mm:ss.SSS} [%t] [%-5level] %logger{36} - %msg%n"/>
</Console>
</Appenders>
<Loggers>
<Root level="info">
<AppenderRef ref="Console"/>
</Root>
</Loggers>
</Configuration>
pom.xml <?xml version="1.0" encoding="UTF-8"?>
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 https://maven.apache.org/xsd/maven-4.0.0.xsd">
<modelVersion>4.0.0</modelVersion>
<parent>
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-starter-parent</artifactId>
<version>2.5.0</version>
<relativePath/> <!-- lookup parent from repository -->
</parent>
<groupId>com.example</groupId>
<artifactId>demo</artifactId>
<version>0.0.1-SNAPSHOT</version>
<name>springboot demo</name>
<description>Demo project for Spring Boot</description>
<properties>
<java.version>1.8</java.version>
</properties>
<dependencies>
<dependency>
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-starter-web</artifactId>
<exclusions>
<exclusion>
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-starter-logging</artifactId>
</exclusion>
</exclusions>
</dependency>
<dependency>
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-starter-log4j2</artifactId>
</dependency>
<dependency>
<groupId>org.apache.logging.log4j</groupId>
<artifactId>log4j-api</artifactId>
<version>2.19.0</version>
</dependency>
<dependency>
<groupId>org.apache.logging.log4j</groupId>
<artifactId>log4j-core</artifactId>
<version>2.19.0</version>
</dependency>
<dependency>
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-starter-test</artifactId>
<scope>test</scope>
</dependency>
</dependencies>
</project>
|
…count of patternAnalysis
…count of patternAnalysis
In my opinion logging a log.error("Failed to process {}, the error is: {}", param, e.getMessage());
log.error("Failed to process {}.", param, e);
log.error("Failed to process {}, the error is: {}", param, e.getMessage(), e);Logback and Log4j Core differ in the way they treat the unconventional call: log.error("Failed to process {}, the error is: {}", param, e);but in both cases the result is not what you would expect:
In November last year I proposed a change of semantics for dealing with throwables in this |
|
@ppkarwasz I understand how to use log4j, but some developers don't write code properly, some old codes still in our repository. I will tell my team that we couldn't migrate smoothly from logback to log4j2. |
|
Also, I found an incompatibility issue, from version 2.19.0 to 2.23.1, for example: @Test
public void testLog() {
log.info("first param is {}, second param is {}", "Log4j2");
}It works ok with v2.19.0, but throw an exception with v2.23.1. |
|
Today there is an increasing amount of tools to check or reformat logging code:
This behavior was introduced in 2.21.0 and corrected in #2343, to provide a uniform way of handling missing log statement parameters (the same way In my personal experience, it is better to detect this kind of errors early in the release cycle. With the previous behavior I remember staring at a literal I am not strongly opinionated regarding this behavior though: we could still fill in the provided parameters, but keep the status logger error message. @vy, what do you think? |
|
@ppkarwasz I really appreciate your professional and rigorous way of coding. I'm inclined to the last method you mentioned about keep the status logger error message. For example, regarding the last log4j2 vulnerability, many companies fix it by replacing the new version of log4j2 in production without enough time to test the compatibility of the new version of log4j2, because the 2.x version is backwards compatible by default. This exception change will cause the program to be unusable in certain scenarios. I think your change could be more appropriate in version 3.x. What do you think? @vy @ppkarwasz |
|
@ppkarwasz @vy Logging a warning or error to the status log is perfectly acceptable to me for the following. As I recall we previously simply didn't replace the '{}' sequence. I would recommend allowing a configuration property to re-enable the previous behavior. I need to also add that when a Throwable is logged using LogBuilder's withThrowable() method it should NEVER be treated as an argument. Likewise when a throwable is included as an argument in the log() method it should ALWAYS be treated only as an argument. Note that DefaultLogBuilder enforces that. I would personally welcome a PR that changed the behavior to work as I have mentioned here. |
|
@SeasonPanPan, thanks so much for the report. This issue is more convoluted than it meets the eye:
Before implementing anything, I want to do some homework first. I will create a list of all corner cases, compare their behaviour across the two different Log4j versions cited above, and then decide on what shall change and in which way. This week I am swamped with other priorities. I believe I can start working on this sometime next week. |
vy
added
bug
vy
changed the title
ppkarwasz
changed the title
|
As Volkan mentioned, solving your migration issues require 3 ingredients that we will try to incorporate into |
|
@ppkarwasz @vy |
@ppkarwasz OK，assign to me |
|
Not sure how this works on Github. Maybe you need to comment on the issue, for me to be able to assign it to you. |
Description
Hello, we used to use logback in our springboot project, recently migrated to log4j2 and we found that a lot of the exception stack is gone.
For example, in the following example, using logback, you can clearly see the error stack, but log4j2 only has one sentence "java.lang.NullPointerException", I read the source code of log4j2 and feel that this should be a bug. I also test it with version:2.23.1, and the same error.
Additionally, I found that these classes are correct: MessageFormatMessage and StringFormattedMessage.
If you can confirm that this is a bug, I'd be pleasure to submit a PR to fix it.
Configuration
Version: 2.23.1
Operating system: windows/Linux
JDK: "1.8.0_291"
Logs
Reproduction
The text was updated successfully, but these errors were encountered: