Improve implementations of LogEvent.toImmutable() and ReusableMessage.memento()
#3171
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The implementations of `LogEvent.toImmutable()` either use serialization or one of the `Log4jLogEvent.Builder` constructors and custom per class code. This PR: - Redirects all implementation of `LogEvent.toImmutable` to the `Builder(LogEvent)` constructor (except `Log4jLogEvent.toImmutable()`) - Improve the constructor to create really immutable and thread-safe instances. - Removes the usage of `ThrowableProxy` for purposes not related to serialization. - Fixes some implementations of `ReusableMessage.memento()`. - Uses `ReusableMessage.memento()` instead of `ImmutableMessage` where applicable.
ppkarwasz
commented
Nov 4, 2024
Comment on lines
-358
to
-366
| private void initTranslatorThreadValues(final RingBufferLogEventTranslator translator) { | ||
| // constant check should be optimized out when using default (CACHED) | ||
| if (THREAD_NAME_CACHING_STRATEGY == ThreadNameCachingStrategy.UNCACHED) { | ||
| translator.updateThreadValues(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Instances of RingBufferLogEventTranslator are always used on the same thread that created them.
…hrowable-proxy-clean-up
vy
approved these changes
May 3, 2025
| @@ -128,7 +128,9 @@ public <S> void forEachParameter(final ParameterConsumer<S> action, final S stat | |||
|
|
|||
| @Override | |||
| public Message memento() { | |||
| return new ObjectMessage(obj); | |||
| Message message = new ObjectMessage(obj); | |||
| message.getFormattedMessage(); | |||
There was a problem hiding this comment.
I presume you make this call to trigger the stack trace capture. Would you mind documenting this, please? (Also in other message/*Message.java changes.)
| assertThat(other.getTimeMillis()).isEqualTo(12345); | ||
| assertThat(other.getInstant().getNanoOfMillisecond()).isEqualTo(678); | ||
| final LogEvent other = SerialUtil.deserialize(SerialUtil.serialize(evt)); | ||
| assertThat(other.getLoggerName()).as("Logger name").isEqualTo(loggerName); |
There was a problem hiding this comment.
Nit: I wouldn't add to the complexity with extra as() calls, since the assertion failure line number precisely points to the associated field.
| * @deprecated since 2.25.0. {@link RingBufferLogEventTranslator} instances should only be used on the thread that | ||
| * created it. | ||
| */ | ||
| @Deprecated |
There was a problem hiding this comment.
Since we removed AsyncLogger::initTranslatorThreadValues, usages of this method will result in undefined behavior. Shouldn't we throw an UnsupportedOE in this method?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The implementations of
LogEvent.toImmutable()either use serialization or one of theLog4jLogEvent.Builderconstructors and custom per class code.This PR:
LogEvent.toImmutableto theBuilder(LogEvent)constructor (exceptLog4jLogEvent.toImmutable())ThrowableProxyfor purposes not related to serialization.ReusableMessage.memento().ReusableMessage.memento()instead ofImmutableMessagewhere applicable.