ppkarwasz commented on PR #2419: URL: https://github.com/apache/logging-log4j2/pull/2419#issuecomment-2027242392
@rgoers, > It is worth repeating that resource Loggers should **NEVER** be added to the LoggerRegistry. Resource Loggers need to have the same lifetime as the resource. Loggers in the registry never go away and so would result in a memory leak. Note that them not being in the registry isn't really a problem as the underlying Logger they use is. That Logger will be reconfigured when needed. Good point, so maybe `LogManager#getLogger(String name, MessageFactory factory)` could be reimplemented to: 1. create a logger `logger` with the **default** message factory, 2. if `factory` is not `null`, return a wrapper of `logger`. > > IMHO, the ParameterizedMapMessage class that you introduce is already half way to meet @rocketraman's expectations: > > * by using a special [LogEventFactory](https://logging.apache.org/log4j/2.x/javadoc/log4j-core/org/apache/logging/log4j/core/impl/LogEventFactory) the map data can be added to the context map and the baseMessage can be used as message. Something similar already happens with [TimestampMessage](https://logging.apache.org/log4j/2.x/javadoc/log4j-api/org/apache/logging/log4j/message/TimestampMessage)s that add their own timestamp to the LogEvent, > > * certainly the specification of [ContextDataInjector#injectContextData](https://logging.apache.org/log4j/2.x/javadoc/log4j-core/org/apache/logging/log4j/core/ContextDataInjector#injectContextData(java.util.List,org.apache.logging.log4j.util.StringMap)) must change: currently it already receives the current context data of the event as reusable parameter, but most implementation ignore it. > > Sure, you _could_ do this, but I am not sure it is a good idea. There are other uses of `MapMessage` where you do not want the data merged into the context data. I would be hesitant to always merge it without some indication from the user since there are a lot of cases where that is not what you want. Sure, the **default** event factory should not merge the data, but we can propose an alternative one that does merge it. The crucial point for this to work is that `ContextDataInjector` implementations should **not** ignore the `reusable` parameter they receive, but always test if it is empty or not and act accordingly. I don't think the an `isEmpty()` test will have any impact on the performance. > Did you just answer your own question? Of course there is a difference. A Message is really just a container for the format String and its parameters. The fact that you can make more complex messages like a MapMessage is a perk. But the main difference is that the user really controls the contents of the Message while Log4j controls the LogEvent. Sure, there is a semantic difference, but a `MutableLogEvent` with all the "message" fields filled and all the "event" fields empty, looks pretty much as a `Message` to me. As I previously mentioned, for Log4j 3.x I would like to create a factory that implements both `MessageFactory` and `LogEventFactory`. The `createMessage` methods will pull `MutableLogEvent`s from a recycler and fill the "message" fields, while the `createEvent` methods will fill the remaining fields. > The bottom line here is Piotr, I am not sure what to do with your comment. You haven't actually asked for any changes. I needed some time to digest your proposed changes, :wink:. While it doesn't touch a lot files, the amount of API changes in this PR has not been seen since the introduction of `LogBuilder`. Having done some questionable "improvements" to the `o.a.l.l` package in the past (e.g. the `BridgeAware` interface that I should have put in `o.a.l.l.spi`), I had to do some research to decide which changes are required and which we'd better drop. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
