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.

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

refactor: Parameterized logging with constant log values #2910

Merged
merged 3 commits into from
Nov 22, 2023

Conversation

timtebeek
Copy link
Contributor

Use this link to re-run the recipe: https://app.moderne.io/recipes/builder/sgdN8FTlx

As discussed on #2594 (comment)

Use this link to re-run the recipe: https://app.moderne.io/recipes/builder/sgdN8FTlx

Co-authored-by: Moderne <team@moderne.io>
@timtebeek
Copy link
Contributor Author

We now see a failure due to a behavioral change in LoggingInterceptor

-            logger.warn(format("[%s] execution failed:", message.getPayloadType().getSimpleName()), t);
+            logger.warn("[{}] execution failed:", message.getPayloadType().getSimpleName(), t);
[ERROR] Failures: 
[ERROR]   LoggingInterceptorTest.handlerInterceptorWithFailedExecution:134 
Argument(s) are different! Wanted:
extendedLogger.logIfEnabled(
    <any string>,
    WARN,
    isNull(),
    and(contains("StubMessage"), contains("failed")),
    java.lang.RuntimeException
);
-> at org.axonframework.messaging.interceptors.LoggingInterceptorTest.handlerInterceptorWithFailedExecution(LoggingInterceptorTest.java:134)
Actual invocations have different arguments:
extendedLogger.logIfEnabled(
    "org.apache.logging.slf4j.Log4jLogger",
    INFO,
    null,
    "Incoming message: [{}]",
    "StubMessage"
);
-> at org.apache.logging.slf4j.Log4jLogger.info(Log4jLogger.java:178)
extendedLogger.logIfEnabled(
    "org.apache.logging.slf4j.Log4jLogger",
    WARN,
    null,
    "[{}] execution failed:",
    "StubMessage",
    java.lang.RuntimeException
);
-> at org.apache.logging.slf4j.Log4jLogger.warn(Log4jLogger.java:243)

[INFO] 
[ERROR] Tests run: 1975, Failures: 1, Errors: 0, Skipped: 0

@smcvb smcvb added Priority 4: Would Lowest priority. Would-be-nice to include issues when time allows it. Type: Enhancement Use to signal an issue enhances an already existing feature of the project. Status: In Progress Use to signal this issue is actively worked on. labels Nov 22, 2023
@smcvb smcvb added this to the Release 4.9.1 milestone Nov 22, 2023
@smcvb smcvb requested review from a team, gklijs, CodeDrivenMitch and smcvb and removed request for a team November 22, 2023 08:35
@timtebeek
Copy link
Contributor Author

The Java 11 build fails on

[ERROR]   TrackingEventProcessorTest.resetBeforeStartingPerformsANormalRun:1168 » ConditionTimeout Condition with alias 'Handle Events' didn't complete within 2 seconds 500 milliseconds because assertion condition defined as a lambda expression in org.axonframework.integrationtests.eventhandling.TrackingEventProcessorTest that uses int, intjava.util.List Actually handled [3] instead of expected [4] ==> expected: <4> but was: <3>.

The Java 17 build fails on

[ERROR]   SpringBootDockerComposeIntegrationTest.setUp:37 » NullPointer Cannot invoke "org.springframework.boot.docker.compose.core.DockerCliInspectResponse.hostConfig()" because "inspectResponse" is null

The Java 21 build has a suspicious failure probably unrelated to the changes here.

Caused by: java.io.IOException: Error while instrumenting org/axonframework/eventsourcing/eventstore/jpa/SnapshotEventEntry$HibernateProxy$DwNKPWeP with JaCoCo 0.8.8.202204050719/5dcf34a.
	at org.jacoco.agent.rt.internal_b6258fc.core.instr.Instrumenter.instrumentError(Instrumenter.java:161)
	at org.jacoco.agent.rt.internal_b6258fc.core.instr.Instrumenter.instrument(Instrumenter.java:111)
	at org.jacoco.agent.rt.internal_b6258fc.CoverageTransformer.transform(CoverageTransformer.java:92)
	... 145 more
Caused by: java.lang.IllegalArgumentException: Unsupported class file major version 65

I'm thinking all of these changes are unrelated, especially since the Java 8 build passed. You might want to look at using Develocity (formerly Gradle Enterprise, also available for Maven) to get help identifying any flaky tests.

@timtebeek
Copy link
Contributor Author

Jacoco likely needs a bump to 0.8.11 to support Java 21.

@abuijze
Copy link
Member

abuijze commented Nov 22, 2023

Hi Tim, the Java 17 build failure is due to an issue in Spring and/or Docker Compose. There were some API changes in docker-compose that caused SpringBoot DockerCompose to not work properly. The fix is expected to be released by the Spring team tomorrow.

Copy link
Member

@smcvb smcvb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 👍

@smcvb smcvb modified the milestones: Release 4.9.1, Release 4.10.0 Nov 22, 2023
@smcvb smcvb merged commit 2bf65ab into AxonFramework:master Nov 22, 2023
5 of 7 checks passed
@smcvb
Copy link
Member

smcvb commented Nov 22, 2023

Thanks once more for the effort @timtebeek!!

@smcvb smcvb added Status: Resolved Use to signal that work on this issue is done. and removed Status: In Progress Use to signal this issue is actively worked on. labels Nov 22, 2023
@timtebeek timtebeek deleted the refactor/parameterized-logging branch November 22, 2023 12:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority 4: Would Lowest priority. Would-be-nice to include issues when time allows it. Status: Resolved Use to signal that work on this issue is done. Type: Enhancement Use to signal an issue enhances an already existing feature of the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants