oshai / kotlin-logging

Lightweight Multiplatform logging framework for Kotlin. A convenient and performant logging facade.
Other
2.51k stars 110 forks source link

IllegalArgumentException when message includes {} #401

Open Zack-Freedman-Thoughtworks opened 3 months ago

Zack-Freedman-Thoughtworks commented 3 months ago

In the following example, the first error leads to the failure:

WARN StatusConsoleListener io.github.oshai.kotlinlogging.slf4j.internal.LocationAwareKLogger caught java.lang.IllegalArgumentException logging ReusableParameterizedMessage: Hey this fails {}
 java.lang.IllegalArgumentException: found 1 argument placeholders, but provided 0 for pattern `Hey this fails {}`
    at org.apache.logging.log4j.message.ParameterFormatter.formatMessage(ParameterFormatter.java:238)
logger.atInfo {
    message = "Hey this fails {}"
    payload = mapOf(
        "first" to "second"
    )
}

logger.atInfo {
    message = "Hey this doesn't fail {}"
}

Configuration:

    implementation("org.springframework.boot:spring-boot-starter-log4j2")
    implementation("io.github.oshai:kotlin-logging-jvm:6.0.3")
    implementation("org.springframework.boot:spring-boot-starter-actuator")

It doesn't seem like the issue happens with Spring Boots default logger spring-boot-starter-logging (vs log4j2).

I am aware this is an error arising from log4j but I'm wondering if there is something we can do for these methods to always avoid parameterized messaging. As far as I understand, we couldn't use atInfo to parameterize messages anyways.

Example can be found here. Let me know if there's a better way to provide examples.

Zack-Freedman-Thoughtworks commented 3 months ago

When I first noticed this issue, the error presented as a JsonMappingException which is likely related to using JsonLayout

oshai commented 3 months ago

Yes, this is really something related to the specific log4j impl. I don't have a good idea how to fix that (looking for {} placeholder in strings feels an overkill, as anyway we will just throw an exception).

Zack-Freedman-Thoughtworks commented 3 months ago

Is there any workaround for this or should we stay away from the version upgrade? I'd like to avoid breaking some logs for an upgrade.

oshai commented 1 month ago

I am not sure what you're trying to upgrade: from which version to which and how does the log call changes. I suspect using a payload is what causing this, so I expect that change to be breaking anyway, and need to be tested.

Can you please provide so more details?

Zack-Freedman-Thoughtworks commented 1 month ago

Hi, as you guessed, we were experimenting with a major version increment where we weren't using payloads in the past. I suppose we can still upgrade as long as we don't allow payloads, but that seems hard to enforce.