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

[36] Override the ThreadContextMap in the provider. #37

Merged
merged 1 commit into from Jun 27, 2023

Conversation

jamezp
Copy link
Member

@jamezp jamezp commented Jun 8, 2023

resolves #36

@sschulze
Copy link

sschulze commented Jun 8, 2023

Looks pretty good to me. Just two remarks:

  • in org.apache.logging.log4j.spi.Provider#loadThreadContextMap() is some logic with the classloader provided in the constructor. I'm not familiar enough with classloader kungfu, but is this a problem?
  • As you already mentioned in your PR, the isEmpty() and getCopy() method is called very often in org.jboss.logmanager.log4j.JBossLogger#logMessage(...), so they are quite performance critical.

@Override
public boolean isEmpty() {
// TODO (jrp) this is not fast and should be thought about, should we add this to the log managers MDC?
return MDC.copy().isEmpty();
Copy link
Member

Choose a reason for hiding this comment

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

I would suggest that we consider adding an MDC.isEmpty query method to avoid the overhead of the copy in this case.

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense, I filed https://issues.redhat.com/browse/LOGMGR-327 and will send up a PR.

@sschulze
Copy link

sschulze commented Jun 9, 2023

I just recognized, that previously it was ok to call ThreadContext.put("foo", null) (because DefaultThreadContextMap uses a java.lang.HashMap internally) but now this fails with a NPE.
It's not declared in the JavaDoc of MdcProvider but I checked three different implementations and they all contain null-checks resulting usually in a NPE.

@dmlloyd
Copy link
Member

dmlloyd commented Jun 9, 2023

I think we can safely treat this case equivalently to MDC.remove("foo").

@Override
public Map<String, String> getImmutableMapOrNull() {
final Map<String, String> copy = MDC.copy();
return copy.isEmpty() ? null : Collections.unmodifiableMap(copy);
Copy link
Member

Choose a reason for hiding this comment

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

I would use Map.copyOf(copy). This way, if we ever change MDC.copy() to return an immutable map, this won't actually have to make a copy.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we just need to require Java 11 first. We're currently targeting Java 8.

@jamezp jamezp force-pushed the issue36 branch 2 times, most recently from 9473d68 to ccf8518 Compare June 27, 2023 17:32
@jamezp
Copy link
Member Author

jamezp commented Jun 27, 2023

I changed the approach on this since we're going to do this requiring JBoss Log Manager 3.x. I've implemented a MDCProvider and an NDCProvider which are instead backed by the ThreadContext.

@jamezp jamezp marked this pull request as ready for review June 27, 2023 17:33
@jamezp
Copy link
Member Author

jamezp commented Jun 27, 2023

Never mind, this new approach only works if the log4j2-jboss-logmanager is the only delegating log manager. We can't replace MDC and NDC like this.

resolves jboss-logging#36

Signed-off-by: James R. Perkins <jperkins@redhat.com>
@jamezp jamezp merged commit 76f1561 into jboss-logging:main Jun 27, 2023
7 checks passed
@jamezp jamezp deleted the issue36 branch June 27, 2023 18:18
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.

Adapt log4j2.ThreadContext to jboss-logging.MDC
3 participants