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

allow to override fqcn in Log4jEventBuilder #1534

Merged
merged 1 commit into from
Jul 11, 2023
Merged

Conversation

oshai
Copy link
Contributor

@oshai oshai commented Jul 3, 2023

fix #1533

[A clear and concise description of what the pull request is for along with a reference to the associated issue IDs, if they exist.]

Checklist

  • Base your changes on 2.x branch if you are targeting Log4j 2; use main otherwise
  • ./mvnw verify succeeds (if it fails due to code formatting issues reported by Spotless, simply run ./mvnw spotless:apply and retry)
  • Non-trivial changes contain an entry file in the src/changelog/.2.x.x directory
  • Tests for the changes are provided
  • Commits are signed (optional, but highly recommended)

@ppkarwasz
Copy link
Contributor

Thanks @oshai,

The fix looks good to me. Can you also:

  • provide a test to validate your change (e.g. add a test case to CallerInformationTest),
  • add a changelog file to src/changelog/.2.x.x (check the other entries for the format).

@oshai
Copy link
Contributor Author

oshai commented Jul 4, 2023

Added changelog and test.
I was unable to verify the test locally at the moment because of the following error:

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-surefire-plugin:3.0.0-M9:test (test) on project log4j-api-java9: Requested toolchain specification did not match any configured toolchain: {version=[9, )} -> [Help 1]

With the following java version (I am using sdk man):

$ java -version
openjdk version "17.0.5" 2022-10-18
OpenJDK Runtime Environment Temurin-17.0.5+8 (build 17.0.5+8)
OpenJDK 64-Bit Server VM Temurin-17.0.5+8 (build 17.0.5+8, mixed mode, sharing)

@oshai
Copy link
Contributor Author

oshai commented Jul 5, 2023

I'v fixed the tool issue (configured toolchain), and now have another exception when running tests locally looks unrelated:

ERROR StatusLogger LogManager returned an instance of org.apache.logging.slf4j.SLF4JLoggerContextFactory which does not implement org.apache.logging.log4j.core.impl.Log4jContextFactory. Unable to initialize Log4j.

java.lang.NullPointerException
	at org.apache.logging.log4j.core.test.junit.LoggerContextRule.getConfiguration(LoggerContextRule.java:177)
	at org.apache.logging.log4j.core.test.junit.LoggerContextRule.getAppender(LoggerContextRule.java:153)
	at org.apache.logging.log4j.core.test.junit.LoggerContextRule.getListAppender(LoggerContextRule.java:209)
	at org.apache.logging.slf4j.CallerInformationTest.testClassLogger(CallerInformationTest.java:43)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:56)
	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
	at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
	at org.junit.runners.BlockJUnit4ClassRunner$1.evaluate(BlockJUnit4ClassRunner.java:100)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:366)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:103)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:63)
	at org.junit.runners.ParentRunner$4.run(ParentRunner.java:331)
	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:79)
	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:329)
	at org.junit.runners.ParentRunner.access$100(ParentRunner.java:66)
	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:293)
	at org.apache.logging.log4j.core.test.junit.LoggerContextRule$1.evaluate(LoggerContextRule.java:126)
	at org.junit.rules.RunRules.evaluate(RunRules.java:20)
	at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
	at org.junit.runners.ParentRunner.run(ParentRunner.java:413)
	at org.junit.runner.JUnitCore.run(JUnitCore.java:137)
	at com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:69)
	at com.intellij.rt.junit.IdeaTestRunner$Repeater$1.execute(IdeaTestRunner.java:38)
	at com.intellij.rt.execution.junit.TestsRepeater.repeat(TestsRepeater.java:11)
	at com.intellij.rt.junit.IdeaTestRunner$Repeater.startRunnerWithArgs(IdeaTestRunner.java:35)
	at com.intellij.rt.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:232)
	at com.intellij.rt.junit.JUnitStarter.main(JUnitStarter.java:55)


@oshai
Copy link
Contributor Author

oshai commented Jul 5, 2023

I had to do another local change:

if (factoryClass != null && factoryClass.getName().equals("org.apache.logging.log4j.core.impl.Log4jContextFactory")) {...

But now the test pass locally.

@oshai
Copy link
Contributor Author

oshai commented Jul 5, 2023

@ppkarwasz I think the PR is ready if you can take a look please.

@oshai
Copy link
Contributor Author

oshai commented Jul 6, 2023

Gihub actions now fails only on windows gradle-core, which seems unrelated to this change:

Error:  Failures: 
Error:    GcFreeAsynchronousLoggingTest.testNoAllocationDuringSteadyStateLogging:35 pattern mismatch at line 1: FATAL o.a.l.l.c.GcFreeAsynchronousLoggingTest [main] value1 {aKey=value1, key2I just allocated the object com.lmax.disruptor.InsufficientCapacityException of type com/lmax/disruptor/InsufficientCapacityException whose size is 32 ==> expected: <true> but was: <false>
Error:    AsyncLoggerThreadContextDefaultTest>AbstractAsyncThreadContextTestBase.testAsyncLogWritesToLog:161->AbstractAsyncThreadContextTestBase.checkResult:187 AsyncLoggerTest.log: line 3 expected:<...igProp2=configValue2[, count=3} WEBAPP DefaultThreadContextMap AsyncLoggerContext i=3]> but was:<...igProp2=configValue2[} WEBAPP DefaultThreadContextMap AsyncLoggerContext i=128]>
[INFO] 
Error:  Tests run: 2500, Failures: 2, Errors: 0, Skipped: 34

@oshai
Copy link
Contributor Author

oshai commented Jul 10, 2023

Hi, PR is ready, please let me know if anything else is needed. If not, it would be great to know the timeline to merge / release as this is blocking: oshai/kotlin-logging#332.

Copy link
Contributor

@ppkarwasz ppkarwasz left a comment

Choose a reason for hiding this comment

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

@oshai, looks good to me.

@ppkarwasz ppkarwasz added this to the 2.20.1 milestone Jul 10, 2023
@oshai
Copy link
Contributor Author

oshai commented Jul 10, 2023

There are still some test failures, but doens't looks related to the PR.

@ppkarwasz
Copy link
Contributor

There are still some test failures, but doesn't looks related to the PR.

No, they are not. We have 2 or 3 flaky tests that appear almost exclusively on the CI.

@ppkarwasz ppkarwasz merged commit 372ea7c into apache:2.x Jul 11, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Log4jEventBuilder does not support dynamic fqcn so incompatible with slf4j
2 participants