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

Logback: don't make MDCPropertyMap of logging event immutable #12718

Merged
merged 2 commits into from
Nov 14, 2024

Conversation

laurit
Copy link
Contributor

@laurit laurit commented Nov 13, 2024

Resolves #12615

@laurit laurit requested a review from a team as a code owner November 13, 2024 08:24
import ch.qos.logback.core.spi.AppenderAttachableImpl;
import java.util.Iterator;

public class TestAppender extends UnsynchronizedAppenderBase<ILoggingEvent>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public class TestAppender extends UnsynchronizedAppenderBase<ILoggingEvent>
class TestAppender extends UnsynchronizedAppenderBase<ILoggingEvent>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this class must be public

if (eventContext == null) {
eventContext = contextData;
} else {
eventContext = new UnionMap<>(eventContext, contextData);
Copy link
Member

Choose a reason for hiding this comment

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

was there some benefit in retaining both individual maps with the use of UnionMap that we lose now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It avoided copying the original map.

runWithSpanAndBaggage("test", baggage, () -> logger.info("log message"));

assertThat(testAppender.lastEvent.getMessage()).isEqualTo("log message");
Map<String, String> map = testAppender.lastEvent.getMDCPropertyMap();
Copy link
Contributor

Choose a reason for hiding this comment

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

There are two instrumentation points:


Is there any reason to miss getMdc test here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a library instrumentation tests. It only serves as a regression test to ensure that we don't accidentally introduce something similar to the union map in the future.

@trask trask merged commit c631801 into open-telemetry:main Nov 14, 2024
56 checks passed
akats7 pushed a commit to akats7/opentelemetry-java-instrumentation that referenced this pull request Nov 21, 2024
akats7 pushed a commit to akats7/opentelemetry-java-instrumentation that referenced this pull request Nov 21, 2024
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.

Logback fails to print logs successfully
5 participants