-
Notifications
You must be signed in to change notification settings - Fork 1k
Unify Log4j2 appender tests #14859
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
base: main
Are you sure you want to change the base?
Unify Log4j2 appender tests #14859
Conversation
🔧 The result from spotlessApply was committed to the PR branch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these tests were moved to the shared testing base class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these tests were mostly redundant with the javaagent tests which were moved to the new shared base class
} | ||
|
||
@Test | ||
void logWithSpan() { // Does not work for log replay, but it is not likely to occur because |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
redundant with the new shared base class tests
<OpenTelemetry name="OpenTelemetryAppender" numLogsCapturedBeforeOtelInstall="1" captureMapMessageAttributes="true" captureMarkerAttribute="true" captureContextDataAttributes="key1,key2" captureExperimentalAttributes="true" captureCodeAttributes="true"/> | ||
</Appenders> | ||
<Loggers> | ||
<Logger name="TestLogger" level="All"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TestLogger is still used by library tests which were not redundant with common tests
The async logger condition was removed in the previous commit which caused testAsyncLogger to fail. AsyncLogger doesn't capture source location attributes by default in non-latest dependency tests, so this check is needed to return empty code location attributes in that case.
…odeAttributes Changed addCodeLocationAttributes() from static to instance method to enable overriding in subclasses. Library tests now override this method to return empty list since the log4j2.xml configuration lacks captureCodeAttributes. The async logger condition is preserved in both base implementation and override to maintain consistency across all test scenarios.
- Add captureCodeAttributes="true" to library/log4j2.xml - Remove override from library Log4j2Test, use base class method - Base class addCodeLocationAttributes() handles all scenarios: * Returns empty for AsyncLogger + older Log4j2 versions * Returns code location attributes otherwise - All tests pass (library and AsyncLogger)
The javaagent auto-instruments Log4j2 and enables code location capture, which works even with AsyncLogger. Updated addCodeLocationAttributes() to: - Always return code location for javaagent tests - Only skip for library AsyncLogger tests with older dependencies All tests now pass: - javaagent testAsync: ✓ (expects code location) - library test: ✓ (expects code location) - library testAsyncLogger: ✓ (no code location for older deps)
🔧 The result from spotlessApply was committed to the PR branch. |
679f376
to
5b21e7f
Compare
.../src/main/java/io/opentelemetry/instrumentation/log4j/appender/v2_17/AbstractLog4j2Test.java
Outdated
Show resolved
Hide resolved
…a/io/opentelemetry/instrumentation/log4j/appender/v2_17/AbstractLog4j2Test.java
} | ||
|
||
protected static List<AttributeAssertion> threadAttributesAssertions() { | ||
return Arrays.asList( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you could do static imports for asList
and stringKey
and make all of the test methods not public
3f45b8b
to
e9cf224
Compare
Resolves #4956