openrewrite / rewrite-logging-frameworks

OpenRewrite recipes for assisting with Java logging migration tasks.
Apache License 2.0
26 stars 21 forks source link

ParameterizedLogging recipe doesn't support Log4j2 Marker #159

Closed jeffreye closed 5 months ago

jeffreye commented 5 months ago

What version of OpenRewrite are you using?

I am using

How are you running OpenRewrite?

I am using the Maven plugin, and my project is a single module project.


            <plugin>
                <groupId>org.openrewrite.maven</groupId>
                <artifactId>rewrite-maven-plugin</artifactId>
                <version>5.34.0</version>
                <configuration>
                    <activeRecipes>
                        <recipe>org.openrewrite.java.logging.log4j.ParameterizedLogging</recipe>
                    </activeRecipes>
                </configuration>
                <dependencies>
                    <dependency>
                        <groupId>org.openrewrite.recipe</groupId>
                        <artifactId>rewrite-logging-frameworks</artifactId>
                        <version>2.9.1</version>
                    </dependency>
                </dependencies>
            </plugin>

What is the smallest, simplest way to reproduce the problem?

@Log4j2
class A {
    public static final Marker                             MY_MARKER                     = MarkerManager.getMarker("my-A");
    void foo(String bar) {
       log.debug(MY_MARKER, "foo1");
       log.debug(MY_MARKER, "foo2 " + bar);
        log.debug(MY_MARKER, () -> bar);
    }
}

What did you expect to see?

@Log4j2
class A {
    public static final Marker                             MY_MARKER                     = MarkerManager.getMarker("my-A");
    void foo(String bar) {
       log.debug(MY_MARKER, "foo1");
       log.debug(MY_MARKER, "foo2 {}", bar);
        log.debug(MY_MARKER, () -> bar);
    }
}

What did you see instead?

@Log4j2
class A {
    public static final Marker                             MY_MARKER                     = MarkerManager.getMarker("my-A");
    void foo(String bar) {
       log.debug(“{}”, MY_MARKER, "foo1");
       log.debug("{}", MY_MARKER, "foo2 " + bar);
        log.debug("{}", MY_MARKER, () -> bar);
    }
}

What is the full stack trace of any errors you encountered?

stacktrace output here

Are you interested in contributing a fix to OpenRewrite?

timtebeek commented 5 months ago

Hi @jeffreye ! Thanks for the detailed report. That seems like an oversight indeed, that I think should be relatively straightforward to resolve. Right here is where we'd want to make an exception for Markers, if they're the first argument passed in: https://github.com/openrewrite/rewrite-logging-frameworks/blob/b46bf0b8572e2dbdc23a8ae827519321f244e8de/src/main/java/org/openrewrite/java/logging/ParameterizedLogging.java#L101

Simplest would be to not make any changes just yet; a little more complicated to make the correct change when markers are involved for the rest of the arguments.

Would you be willing to help out with a draft PR containing just a unit test as seen here? https://github.com/openrewrite/rewrite-logging-frameworks/blob/b46bf0b8572e2dbdc23a8ae827519321f244e8de/src/test/java/org/openrewrite/java/logging/ParameterizedLoggingTest.java#L43-L68

Then we can see from there which next steps to take to see this resolved.